From de6c68b91e57208b7ccb454d53916553d79c667c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 3 Jan 2018 13:40:44 -0800 Subject: [PATCH 01/14] Always show "X requested review" in mail to stop some undraft mail from being dropped Summary: Ref T13035. See that task for a description of the issue. Test Plan: - Enabled prototypes. - Disabled all Herald rules that trigger Harbormaster builds. - Created a new revision. - Before patch: initial review request email was dropped. - After patch: initial review request email is sent. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13035 Differential Revision: https://secure.phabricator.com/D18851 --- .../differential/storage/DifferentialTransaction.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index ea0d7789cb..18edd3c625 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -111,6 +111,12 @@ final class DifferentialTransaction // Don't hide the initial "X added reviewers: ..." transaction during // object creation from mail. See T12118 and PHI54. return false; + case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE: + // Don't hide the initial "X requested review: ..." transaction from + // mail even when it occurs during creation. We need this transaction + // to survive so we'll generate mail when revisions immediately leave + // the draft state. See T13035 for discussion. + return false; } return parent::shouldHideForMail($xactions); From 129e3a120849ada69ecda9f7a475cb9522080eb5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 3 Jan 2018 13:47:56 -0800 Subject: [PATCH 02/14] Fix a minor/harmless race with feed publishers in certain draft states Summary: Depends on D18851. Ref T13035. After D18819, revision creation transactions may be split into two groups (if prototypes are enabled). This split means we have two workers. The first worker doesn't publish feed stories or mail; the second one does. Currently, both workers call `shouldPublishFeedStory()` before they queue, and then again after the daemons pull them out of the queue. However, the answer to this question can change. Specifically, this happens: - `arc` creates a revision. - The first transaction group applies, creating the revision as a draft, and returns `false` from `shouldPublishFeedStory()`, and does not generate related PHIDs. It queues a daemon to send mail, expecting it not to publish a feed story. - The second transaction group applies, promoting the revision to "needs review". Since the revision has promoted, `shouldPublishFeedStory()` now returns true. This editor generates related PHIDs and queues a daemon task, expecting it to send mail / publish feed. - A few milliseconds pass. - The first job gets pulled out of the daemon queue and worked on. It does not have any feed metadata because the object wasn't publishable when the job was queued -- but `shouldPublishFeedStory()` now returns true, so it tries to publish a story without any metadata available. Slightly bad stuff happens (see below). - The second job gets pulled out of the daemon queue and worked on. This one has metadata and works fine. The "slightly bad stuff" is that we publish an empty feed story with no references to any objects, then try to push it to hooks and other listeners. Since it doesn't have any references, it fails to load during the "push to external listeners" phase. This is harmless but clutters the log and doesn't help anything. Instead, cache the state of "are we publishing a feed story for this object?" when we queue the worker so it can't race. Test Plan: - Enabled prototypes. - Disabled all Herald triggers for Harbormaster build plans. - Ran `bin/phd debug task` in one window. - Created a revision in a separate window. - Before patch: saw "unable to load feed story" errors in the daemon log. - After patch: no more "unable to load feed story" errors. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13035 Differential Revision: https://secure.phabricator.com/D18852 --- src/applications/feed/worker/FeedPushWorker.php | 4 +++- .../editor/PhabricatorApplicationTransactionEditor.php | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/applications/feed/worker/FeedPushWorker.php b/src/applications/feed/worker/FeedPushWorker.php index 90407f6a75..3eff676e99 100644 --- a/src/applications/feed/worker/FeedPushWorker.php +++ b/src/applications/feed/worker/FeedPushWorker.php @@ -13,7 +13,9 @@ abstract class FeedPushWorker extends PhabricatorWorker { if (!$story) { throw new PhabricatorWorkerPermanentFailureException( - pht('Feed story does not exist.')); + pht( + 'Feed story (with key "%s") does not exist or could not be loaded.', + $key)); } return $story; diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 8e6d816769..c08451624d 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -68,6 +68,7 @@ abstract class PhabricatorApplicationTransactionEditor private $mailCCPHIDs = array(); private $feedNotifyPHIDs = array(); private $feedRelatedPHIDs = array(); + private $feedShouldPublish = false; private $modularTypes; private $transactionQueue = array(); @@ -1159,6 +1160,7 @@ abstract class PhabricatorApplicationTransactionEditor } if ($this->shouldPublishFeedStory($object, $xactions)) { + $this->feedShouldPublish = true; $this->feedRelatedPHIDs = $this->getFeedRelatedPHIDs($object, $xactions); $this->feedNotifyPHIDs = $this->getFeedNotifyPHIDs($object, $xactions); } @@ -1216,8 +1218,7 @@ abstract class PhabricatorApplicationTransactionEditor )); } - if ($this->shouldPublishFeedStory($object, $xactions)) { - + if ($this->feedShouldPublish) { $mailed = array(); foreach ($messages as $mail) { foreach ($mail->buildRecipientList() as $phid) { @@ -3512,6 +3513,7 @@ abstract class PhabricatorApplicationTransactionEditor 'mailCCPHIDs', 'feedNotifyPHIDs', 'feedRelatedPHIDs', + 'feedShouldPublish', ); } From cb957f8d62424ace76a06331d9859d92dfb9f5db Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 09:34:40 -0800 Subject: [PATCH 03/14] Pile more atrocities onto the Maniphest burnup report Summary: See PHI273. Ref T13020. After D18777, tasks created directly into the default status (which is common) via the web UI no longer write a "status" transaction. This is consistent with other applications, and consistent with the API/email behavior for tasks since early 2016. It also improves the consistency of //reading// tasks via the API. However, it impacted the "Burnup Report" which relies on directly reading these rows to detect task creation. Until this is fixed properly (T1562), synthetically generate the "missing" transactions which this page expects by looking at task creation dates instead. Specifically, we: - Generate a fake `status: null -> "open"` transaction for every task by looking at the Task table. - Go through the transaction list and remove all the legacy `status: null -> "any open status"` transactions. These will only exist for older tasks. - Merge all our new fake transactions into the list of transactions. - Continue on as though nothing happened, letting the rendering code continue to operate on legacy-looking data. I think this will slightly miscount tasks which were created directly into a closed status, but this is very rare, and does not significantly impact the accuracy of this report relative to other known issues (notably, merging closed tasks). This will also get the wrong result if the default status has changed from an "open" status to a "closed" status at any point, but this is exceptionally bizarre/rare. Ultimately, T1562 will let us delete all this stuff and disavow its existence. Test Plan: - Created some tasks, loaded burnup before/after this patch. - My local chart looks more accurate afterwards, but the data is super weird (I used `bin/lipsum` to create a huge number of tasks a couple months ago). I'll vet this on `secure`, which has more reasonable data. Here's my local chart: {F5356499} That's what it //should// look like, it's just hard to be confident that nothing else is hiding there. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13020 Differential Revision: https://secure.phabricator.com/D18853 --- .../controller/ManiphestReportController.php | 57 ++++++++++++++++++- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php index 66aa154e8f..821c96a2aa 100644 --- a/src/applications/maniphest/controller/ManiphestReportController.php +++ b/src/applications/maniphest/controller/ManiphestReportController.php @@ -99,13 +99,66 @@ final class ManiphestReportController extends ManiphestController { ManiphestTaskMergedIntoTransaction::TRANSACTIONTYPE, )); + // See PHI273. After the move to EditEngine, we no longer create a + // "status" transaction if a task is created directly into the default + // status. This likely impacted API/email tasks after 2016 and all other + // tasks after late 2017. Until Facts can fix this properly, use the + // task creation dates to generate synthetic transactions which look like + // the older transactions that this page expects. + + $default_status = ManiphestTaskStatus::getDefaultStatus(); + $duplicate_status = ManiphestTaskStatus::getDuplicateStatus(); + + // Build synthetic transactions which take status from `null` to the + // default value. + $create_rows = queryfx_all( + $conn, + 'SELECT dateCreated FROM %T', + id(new ManiphestTask())->getTableName()); + foreach ($create_rows as $key => $create_row) { + $create_rows[$key] = array( + 'transactionType' => 'status', + 'oldValue' => null, + 'newValue' => $default_status, + 'dateCreated' => $create_row['dateCreated'], + ); + } + + // Remove any actual legacy status transactions which take status from + // `null` to any open status. + foreach ($data as $key => $row) { + if ($row['transactionType'] != 'status') { + continue; + } + + $oldv = trim($row['oldValue'], '"'); + $newv = trim($row['oldValue'], '"'); + + // If this is a status change, preserve it. + if ($oldv != 'null') { + continue; + } + + // If this task was created directly into a closed status, preserve + // the transaction. + if (!ManiphestTaskStatus::isOpenStatus($newv)) { + continue; + } + + // If this is a legacy "create" transaction, discard it in favor of the + // synthetic one. + unset($data[$key]); + } + + // Merge the synthetic rows into the real transactions. + $data = array_merge($create_rows, $data); + $data = array_values($data); + $stats = array(); $day_buckets = array(); $open_tasks = array(); - $default_status = ManiphestTaskStatus::getDefaultStatus(); - $duplicate_status = ManiphestTaskStatus::getDuplicateStatus(); foreach ($data as $key => $row) { switch ($row['transactionType']) { case ManiphestTaskStatusTransaction::TRANSACTIONTYPE: From 53b25db9185606a93f46cfd5d8911af644d63c31 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Dec 2017 10:38:45 -0800 Subject: [PATCH 04/14] Prevent enormous changes from being pushed to repositoires by default Summary: Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes". If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe. Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default. Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See . Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI. Reviewers: amckinley Reviewed By: amckinley Subscribers: joshuaspence Maniphest Tasks: T13031 Differential Revision: https://secure.phabricator.com/D18850 --- src/__phutil_library_map__.php | 2 + .../PhabricatorDiffusionApplication.php | 1 + .../controller/DiffusionCommitController.php | 4 +- ...fusionRepositoryEditEnormousController.php | 90 +++++++++++++++++++ .../editor/DiffusionRepositoryEditEngine.php | 13 +++ .../engine/DiffusionCommitHookEngine.php | 63 ++++++++++++- .../herald/HeraldPreCommitContentAdapter.php | 2 +- ...ffusionRepositoryBasicsManagementPanel.php | 31 +++++++ .../config/PhabricatorFilesConfigOptions.php | 2 +- .../editor/PhabricatorRepositoryEditor.php | 8 ++ .../storage/PhabricatorRepository.php | 12 +++ .../storage/PhabricatorRepositoryPushLog.php | 2 + .../PhabricatorRepositoryTransaction.php | 11 +++ 13 files changed, 233 insertions(+), 8 deletions(-) create mode 100644 src/applications/diffusion/controller/DiffusionRepositoryEditEnormousController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 849d4d4702..1a8de4e42b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -860,6 +860,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryEditDangerousController' => 'applications/diffusion/controller/DiffusionRepositoryEditDangerousController.php', 'DiffusionRepositoryEditDeleteController' => 'applications/diffusion/controller/DiffusionRepositoryEditDeleteController.php', 'DiffusionRepositoryEditEngine' => 'applications/diffusion/editor/DiffusionRepositoryEditEngine.php', + 'DiffusionRepositoryEditEnormousController' => 'applications/diffusion/controller/DiffusionRepositoryEditEnormousController.php', 'DiffusionRepositoryEditUpdateController' => 'applications/diffusion/controller/DiffusionRepositoryEditUpdateController.php', 'DiffusionRepositoryFunctionDatasource' => 'applications/diffusion/typeahead/DiffusionRepositoryFunctionDatasource.php', 'DiffusionRepositoryHistoryManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php', @@ -5923,6 +5924,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryEditDangerousController' => 'DiffusionRepositoryManageController', 'DiffusionRepositoryEditDeleteController' => 'DiffusionRepositoryManageController', 'DiffusionRepositoryEditEngine' => 'PhabricatorEditEngine', + 'DiffusionRepositoryEditEnormousController' => 'DiffusionRepositoryManageController', 'DiffusionRepositoryEditUpdateController' => 'DiffusionRepositoryManageController', 'DiffusionRepositoryFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DiffusionRepositoryHistoryManagementPanel' => 'DiffusionRepositoryManagementPanel', diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index 5a426ec299..5797b8ba7c 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -89,6 +89,7 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { 'edit/' => array( 'activate/' => 'DiffusionRepositoryEditActivateController', 'dangerous/' => 'DiffusionRepositoryEditDangerousController', + 'enormous/' => 'DiffusionRepositoryEditEnormousController', 'delete/' => 'DiffusionRepositoryEditDeleteController', 'update/' => 'DiffusionRepositoryEditUpdateController', 'testautomation/' => 'DiffusionRepositoryTestAutomationController', diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 11463ef97d..b4d06989c2 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -277,9 +277,9 @@ final class DiffusionCommitController extends DiffusionController { 'This commit is empty and does not affect any paths.')); } else if ($was_limited) { $info_panel = $this->renderStatusMessage( - pht('Enormous Commit'), + pht('Very Large Commit'), pht( - 'This commit is enormous, and affects more than %d files. '. + 'This commit is very large, and affects more than %d files. '. 'Changes are not shown.', $hard_limit)); } else if (!$this->getCommitExists()) { diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditEnormousController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditEnormousController.php new file mode 100644 index 0000000000..d4eeb118d7 --- /dev/null +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditEnormousController.php @@ -0,0 +1,90 @@ +loadDiffusionContextForEdit(); + if ($response) { + return $response; + } + + $viewer = $this->getViewer(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + $panel_uri = id(new DiffusionRepositoryBasicsManagementPanel()) + ->setRepository($repository) + ->getPanelURI(); + + if (!$repository->canAllowEnormousChanges()) { + return $this->newDialog() + ->setTitle(pht('Unprotectable Repository')) + ->appendParagraph( + pht( + 'This repository can not be protected from enormous changes '. + 'because Phabricator does not control what users are allowed '. + 'to push to it.')) + ->addCancelButton($panel_uri); + } + + if ($request->isFormPost()) { + $xaction = id(new PhabricatorRepositoryTransaction()) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_ENORMOUS) + ->setNewValue(!$repository->shouldAllowEnormousChanges()); + + $editor = id(new PhabricatorRepositoryEditor()) + ->setContinueOnNoEffect(true) + ->setContentSourceFromRequest($request) + ->setActor($viewer) + ->applyTransactions($repository, array($xaction)); + + return id(new AphrontReloadResponse())->setURI($panel_uri); + } + + if ($repository->shouldAllowEnormousChanges()) { + $title = pht('Prevent Enormous Changes'); + + $body = pht( + 'It will no longer be possible to push enormous changes to this '. + 'repository.'); + + $submit = pht('Prevent Enormous Changes'); + } else { + $title = pht('Allow Enormous Changes'); + + $body = array( + pht( + 'If you allow enormous changes, users can push commits which are '. + 'too large for Herald to process content rules for. This can allow '. + 'users to evade content rules implemented in Herald.'), + pht( + 'You can selectively configure Herald by adding rules to prevent a '. + 'subset of enormous changes (for example, based on who is trying '. + 'to push the change).'), + ); + + $submit = pht('Allow Enormous Changes'); + } + + $more_help = pht( + 'Enormous changes are commits which are too large to process with '. + 'content rules because: the diff text for the change is larger than '. + '%s bytes; or the diff text takes more than %s seconds to extract.', + new PhutilNumber(HeraldCommitAdapter::getEnormousByteLimit()), + new PhutilNumber(HeraldCommitAdapter::getEnormousTimeLimit())); + + $response = $this->newDialog(); + + foreach ((array)$body as $paragraph) { + $response->appendParagraph($paragraph); + } + + return $response + ->setTitle($title) + ->appendParagraph($more_help) + ->addSubmitButton($submit) + ->addCancelButton($panel_uri); + } + +} diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index b3baafbd06..4eebc09b52 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -309,6 +309,19 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Allow or prevent dangerous changes.')) ->setConduitTypeDescription(pht('New protection setting.')) ->setValue($object->shouldAllowDangerousChanges()), + id(new PhabricatorBoolEditField()) + ->setKey('allowEnormousChanges') + ->setLabel(pht('Allow Enormous Changes')) + ->setIsCopyable(true) + ->setIsConduitOnly(true) + ->setOptions( + pht('Prevent Enormous Changes'), + pht('Allow Enormous Changes')) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_ENORMOUS) + ->setDescription(pht('Permit enomrous changes to be made.')) + ->setConduitDescription(pht('Allow or prevent enormous changes.')) + ->setConduitTypeDescription(pht('New protection setting.')) + ->setValue($object->shouldAllowEnormousChanges()), id(new PhabricatorSelectEditField()) ->setKey('status') ->setLabel(pht('Status')) diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 7fe45834b3..99df0e54af 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -34,6 +34,7 @@ final class DiffusionCommitHookEngine extends Phobject { private $rejectCode = PhabricatorRepositoryPushLog::REJECT_BROKEN; private $rejectDetails; private $emailPHIDs = array(); + private $changesets = array(); /* -( Config )------------------------------------------------------------- */ @@ -131,6 +132,15 @@ final class DiffusionCommitHookEngine extends Phobject { $this->applyHeraldRefRules($ref_updates, $all_updates); $content_updates = $this->findContentUpdates($ref_updates); + + try { + $this->rejectEnormousChanges($content_updates); + } catch (DiffusionCommitHookRejectException $ex) { + // If we're rejecting enormous changes, flag everything. + $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_ENORMOUS; + throw $ex; + } + $all_updates = array_merge($all_updates, $content_updates); $this->applyHeraldContentRules($content_updates, $all_updates); @@ -1079,7 +1089,37 @@ final class DiffusionCommitHookEngine extends Phobject { ->setEpoch(time()); } - public function loadChangesetsForCommit($identifier) { + private function rejectEnormousChanges(array $content_updates) { + $repository = $this->getRepository(); + if ($repository->shouldAllowEnormousChanges()) { + return; + } + + foreach ($content_updates as $update) { + $identifier = $update->getRefNew(); + try { + $changesets = $this->loadChangesetsForCommit($identifier); + $this->changesets[$identifier] = $changesets; + } catch (Exception $ex) { + $this->changesets[$identifier] = $ex; + + $message = pht( + 'ENORMOUS CHANGE'. + "\n". + 'Enormous change protection is enabled for this repository, but '. + 'you are pushing an enormous change ("%s"). Edit the repository '. + 'configuration before making enormous changes.'. + "\n\n". + "Content Exception: %s", + $identifier, + $ex->getMessage()); + + throw new DiffusionCommitHookRejectException($message); + } + } + } + + private function loadChangesetsForCommit($identifier) { $byte_limit = HeraldCommitAdapter::getEnormousByteLimit(); $time_limit = HeraldCommitAdapter::getEnormousTimeLimit(); @@ -1126,9 +1166,10 @@ final class DiffusionCommitHookEngine extends Phobject { if (strlen($raw_diff) >= $byte_limit) { throw new Exception( pht( - 'The raw text of this change is enormous (larger than %d '. - 'bytes). Herald can not process it.', - $byte_limit)); + 'The raw text of this change ("%s") is enormous (larger than %s '. + 'bytes).', + $identifier, + new PhutilNumber($byte_limit))); } if (!strlen($raw_diff)) { @@ -1143,6 +1184,20 @@ final class DiffusionCommitHookEngine extends Phobject { return $diff->getChangesets(); } + public function getChangesetsForCommit($identifier) { + if (isset($this->changesets[$identifier])) { + $cached = $this->changesets[$identifier]; + + if ($cached instanceof Exception) { + throw $cached; + } + + return $cached; + } + + return $this->loadChangesetsForCommit($identifier); + } + public function loadCommitRefForCommit($identifier) { $repository = $this->getRepository(); $vcs = $repository->getVersionControlSystem(); diff --git a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php index f4d7e794a2..c93ba32345 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php @@ -37,7 +37,7 @@ final class HeraldPreCommitContentAdapter extends HeraldPreCommitAdapter { public function getDiffContent($type) { if ($this->changesets === null) { try { - $this->changesets = $this->getHookEngine()->loadChangesetsForCommit( + $this->changesets = $this->getHookEngine()->getChangesetsForCommit( $this->getObject()->getRefNew()); } catch (Exception $ex) { $this->changesets = $ex; diff --git a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php index 6e527e81b5..af5e9b2881 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php @@ -43,6 +43,7 @@ final class DiffusionRepositoryBasicsManagementPanel $delete_uri = $repository->getPathURI('edit/delete/'); $encoding_uri = $this->getEditPageURI('encoding'); $dangerous_uri = $repository->getPathURI('edit/dangerous/'); + $enormous_uri = $repository->getPathURI('edit/enormous/'); if ($repository->isTracked()) { $activate_label = pht('Deactivate Repository'); @@ -59,6 +60,15 @@ final class DiffusionRepositoryBasicsManagementPanel $can_dangerous = ($can_edit && $repository->canAllowDangerousChanges()); } + $should_enormous = $repository->shouldAllowEnormousChanges(); + if ($should_enormous) { + $enormous_name = pht('Prevent Enormous Changes'); + $can_enormous = $can_edit; + } else { + $enormous_name = pht('Allow Enormous Changes'); + $can_enormous = ($can_edit && $repository->canAllowEnormousChanges()); + } + $action_list->addAction( id(new PhabricatorActionView()) ->setName(pht('Edit Basic Information')) @@ -80,6 +90,13 @@ final class DiffusionRepositoryBasicsManagementPanel ->setDisabled(!$can_dangerous) ->setWorkflow(true)); + $action_list->addAction( + id(new PhabricatorActionView()) + ->setName($enormous_name) + ->setHref($enormous_uri) + ->setDisabled(!$can_enormous) + ->setWorkflow(true)); + $action_list->addAction( id(new PhabricatorActionView()) ->setHref($activate_uri) @@ -198,6 +215,20 @@ final class DiffusionRepositoryBasicsManagementPanel $view->addProperty(pht('Dangerous Changes'), $dangerous); + $can_enormous = $repository->canAllowEnormousChanges(); + if (!$can_enormous) { + $enormous = phutil_tag('em', array(), pht('Not Preventable')); + } else { + $should_enormous = $repository->shouldAllowEnormousChanges(); + if ($should_enormous) { + $enormous = pht('Allowed'); + } else { + $enormous = pht('Not Allowed'); + } + } + + $view->addProperty(pht('Enormous Changes'), $enormous); + return $view; } diff --git a/src/applications/files/config/PhabricatorFilesConfigOptions.php b/src/applications/files/config/PhabricatorFilesConfigOptions.php index 0f13a2fb4f..1493c54f59 100644 --- a/src/applications/files/config/PhabricatorFilesConfigOptions.php +++ b/src/applications/files/config/PhabricatorFilesConfigOptions.php @@ -134,7 +134,7 @@ final class PhabricatorFilesConfigOptions "Configure which uploaded file types may be viewed directly ". "in the browser. Other file types will be downloaded instead ". "of displayed. This is mainly a usability consideration, since ". - "browsers tend to freak out when viewing enormous binary files.". + "browsers tend to freak out when viewing very large binary files.". "\n\n". "The keys in this map are viewable MIME types; the values are ". "the MIME types they are delivered as when they are viewed in ". diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 401fca3668..768121de5a 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -28,6 +28,7 @@ final class PhabricatorRepositoryEditor $types[] = PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE; $types[] = PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY; $types[] = PhabricatorRepositoryTransaction::TYPE_DANGEROUS; + $types[] = PhabricatorRepositoryTransaction::TYPE_ENORMOUS; $types[] = PhabricatorRepositoryTransaction::TYPE_SLUG; $types[] = PhabricatorRepositoryTransaction::TYPE_SERVICE; $types[] = PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE; @@ -76,6 +77,8 @@ final class PhabricatorRepositoryEditor return $object->getPushPolicy(); case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: return $object->shouldAllowDangerousChanges(); + case PhabricatorRepositoryTransaction::TYPE_ENORMOUS: + return $object->shouldAllowEnormousChanges(); case PhabricatorRepositoryTransaction::TYPE_SLUG: return $object->getRepositorySlug(); case PhabricatorRepositoryTransaction::TYPE_SERVICE: @@ -110,6 +113,7 @@ final class PhabricatorRepositoryEditor case PhabricatorRepositoryTransaction::TYPE_VCS: case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY: case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: + case PhabricatorRepositoryTransaction::TYPE_ENORMOUS: case PhabricatorRepositoryTransaction::TYPE_SERVICE: case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE: case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: @@ -184,6 +188,9 @@ final class PhabricatorRepositoryEditor case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: $object->setDetail('allow-dangerous-changes', $xaction->getNewValue()); return; + case PhabricatorRepositoryTransaction::TYPE_ENORMOUS: + $object->setDetail('allow-enormous-changes', $xaction->getNewValue()); + return; case PhabricatorRepositoryTransaction::TYPE_SLUG: $object->setRepositorySlug($xaction->getNewValue()); return; @@ -248,6 +255,7 @@ final class PhabricatorRepositoryEditor case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE: case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY: case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: + case PhabricatorRepositoryTransaction::TYPE_ENORMOUS: case PhabricatorRepositoryTransaction::TYPE_SLUG: case PhabricatorRepositoryTransaction::TYPE_SERVICE: case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 702bb42780..435e5749e6 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1672,6 +1672,18 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return (bool)$this->getDetail('allow-dangerous-changes'); } + public function canAllowEnormousChanges() { + if (!$this->isHosted()) { + return false; + } + + return true; + } + + public function shouldAllowEnormousChanges() { + return (bool)$this->getDetail('allow-enormous-changes'); + } + public function writeStatusMessage( $status_type, $status_code, diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php index 4c449b4a84..4e099209c6 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php @@ -23,12 +23,14 @@ final class PhabricatorRepositoryPushLog const CHANGEFLAG_APPEND = 4; const CHANGEFLAG_REWRITE = 8; const CHANGEFLAG_DANGEROUS = 16; + const CHANGEFLAG_ENORMOUS = 32; const REJECT_ACCEPT = 0; const REJECT_DANGEROUS = 1; const REJECT_HERALD = 2; const REJECT_EXTERNAL = 3; const REJECT_BROKEN = 4; + const REJECT_ENORMOUS = 5; protected $repositoryPHID; protected $epoch; diff --git a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php index e02b670f75..866800161e 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php +++ b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php @@ -16,6 +16,7 @@ final class PhabricatorRepositoryTransaction const TYPE_AUTOCLOSE = 'repo:autoclose'; const TYPE_PUSH_POLICY = 'repo:push-policy'; const TYPE_DANGEROUS = 'repo:dangerous'; + const TYPE_ENORMOUS = 'repo:enormous'; const TYPE_SLUG = 'repo:slug'; const TYPE_SERVICE = 'repo:service'; const TYPE_SYMBOLS_SOURCES = 'repo:symbol-source'; @@ -376,6 +377,16 @@ final class PhabricatorRepositoryTransaction '%s enabled protection against dangerous changes.', $this->renderHandleLink($author_phid)); } + case self::TYPE_ENORMOUS: + if ($new) { + return pht( + '%s disabled protection against enormous changes.', + $this->renderHandleLink($author_phid)); + } else { + return pht( + '%s enabled protection against enormous changes.', + $this->renderHandleLink($author_phid)); + } case self::TYPE_SLUG: if (strlen($old) && !strlen($new)) { return pht( From 83c528c46435e25f929c1a359bf3b3929783db00 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Dec 2017 08:21:20 -0800 Subject: [PATCH 05/14] Modularize transactions for Drydock Blueprints Summary: Ref PHI243. This is a followup to D18822, which added an edit-only `drydock.blueprint.edit`. By modularizing transactions (here) and then adding a "type" transaction (next change) I intend to remove the "edit-only" limitation and make this API method fully functional. Test Plan: Created and edited blueprints via the web UI. Edited blueprints via the API. Disabled/enabled blueprints (currently web UI only). Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D18845 --- src/__phutil_library_map__.php | 8 +- .../DrydockBlueprintDisableController.php | 3 +- .../editor/DrydockBlueprintEditEngine.php | 2 +- .../drydock/editor/DrydockBlueprintEditor.php | 100 ++---------------- .../storage/DrydockBlueprintTransaction.php | 35 +----- .../DrydockBlueprintDisableTransaction.php | 48 +++++++++ .../DrydockBlueprintNameTransaction.php | 60 +++++++++++ .../DrydockBlueprintTransactionType.php | 4 + 8 files changed, 133 insertions(+), 127 deletions(-) create mode 100644 src/applications/drydock/xaction/DrydockBlueprintDisableTransaction.php create mode 100644 src/applications/drydock/xaction/DrydockBlueprintNameTransaction.php create mode 100644 src/applications/drydock/xaction/DrydockBlueprintTransactionType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1a8de4e42b..0a005eef4a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1008,6 +1008,7 @@ phutil_register_library_map(array( 'DrydockBlueprintCustomField' => 'applications/drydock/customfield/DrydockBlueprintCustomField.php', 'DrydockBlueprintDatasource' => 'applications/drydock/typeahead/DrydockBlueprintDatasource.php', 'DrydockBlueprintDisableController' => 'applications/drydock/controller/DrydockBlueprintDisableController.php', + 'DrydockBlueprintDisableTransaction' => 'applications/drydock/xaction/DrydockBlueprintDisableTransaction.php', 'DrydockBlueprintEditConduitAPIMethod' => 'applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php', 'DrydockBlueprintEditController' => 'applications/drydock/controller/DrydockBlueprintEditController.php', 'DrydockBlueprintEditEngine' => 'applications/drydock/editor/DrydockBlueprintEditEngine.php', @@ -1016,12 +1017,14 @@ phutil_register_library_map(array( 'DrydockBlueprintImplementationTestCase' => 'applications/drydock/blueprint/__tests__/DrydockBlueprintImplementationTestCase.php', 'DrydockBlueprintListController' => 'applications/drydock/controller/DrydockBlueprintListController.php', 'DrydockBlueprintNameNgrams' => 'applications/drydock/storage/DrydockBlueprintNameNgrams.php', + 'DrydockBlueprintNameTransaction' => 'applications/drydock/xaction/DrydockBlueprintNameTransaction.php', 'DrydockBlueprintPHIDType' => 'applications/drydock/phid/DrydockBlueprintPHIDType.php', 'DrydockBlueprintQuery' => 'applications/drydock/query/DrydockBlueprintQuery.php', 'DrydockBlueprintSearchConduitAPIMethod' => 'applications/drydock/conduit/DrydockBlueprintSearchConduitAPIMethod.php', 'DrydockBlueprintSearchEngine' => 'applications/drydock/query/DrydockBlueprintSearchEngine.php', 'DrydockBlueprintTransaction' => 'applications/drydock/storage/DrydockBlueprintTransaction.php', 'DrydockBlueprintTransactionQuery' => 'applications/drydock/query/DrydockBlueprintTransactionQuery.php', + 'DrydockBlueprintTransactionType' => 'applications/drydock/xaction/DrydockBlueprintTransactionType.php', 'DrydockBlueprintViewController' => 'applications/drydock/controller/DrydockBlueprintViewController.php', 'DrydockCommand' => 'applications/drydock/storage/DrydockCommand.php', 'DrydockCommandError' => 'applications/drydock/exception/DrydockCommandError.php', @@ -6102,6 +6105,7 @@ phutil_register_library_map(array( 'DrydockBlueprintCustomField' => 'PhabricatorCustomField', 'DrydockBlueprintDatasource' => 'PhabricatorTypeaheadDatasource', 'DrydockBlueprintDisableController' => 'DrydockBlueprintController', + 'DrydockBlueprintDisableTransaction' => 'DrydockBlueprintTransactionType', 'DrydockBlueprintEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'DrydockBlueprintEditController' => 'DrydockBlueprintController', 'DrydockBlueprintEditEngine' => 'PhabricatorEditEngine', @@ -6110,12 +6114,14 @@ phutil_register_library_map(array( 'DrydockBlueprintImplementationTestCase' => 'PhabricatorTestCase', 'DrydockBlueprintListController' => 'DrydockBlueprintController', 'DrydockBlueprintNameNgrams' => 'PhabricatorSearchNgrams', + 'DrydockBlueprintNameTransaction' => 'DrydockBlueprintTransactionType', 'DrydockBlueprintPHIDType' => 'PhabricatorPHIDType', 'DrydockBlueprintQuery' => 'DrydockQuery', 'DrydockBlueprintSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'DrydockBlueprintSearchEngine' => 'PhabricatorApplicationSearchEngine', - 'DrydockBlueprintTransaction' => 'PhabricatorApplicationTransaction', + 'DrydockBlueprintTransaction' => 'PhabricatorModularTransaction', 'DrydockBlueprintTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'DrydockBlueprintTransactionType' => 'PhabricatorModularTransactionType', 'DrydockBlueprintViewController' => 'DrydockBlueprintController', 'DrydockCommand' => array( 'DrydockDAO', diff --git a/src/applications/drydock/controller/DrydockBlueprintDisableController.php b/src/applications/drydock/controller/DrydockBlueprintDisableController.php index 525e55228b..60fc8b0d74 100644 --- a/src/applications/drydock/controller/DrydockBlueprintDisableController.php +++ b/src/applications/drydock/controller/DrydockBlueprintDisableController.php @@ -28,7 +28,8 @@ final class DrydockBlueprintDisableController $xactions = array(); $xactions[] = id(new DrydockBlueprintTransaction()) - ->setTransactionType(DrydockBlueprintTransaction::TYPE_DISABLED) + ->setTransactionType( + DrydockBlueprintDisableTransaction::TRANSACTIONTYPE) ->setNewValue($is_disable ? 1 : 0); $editor = id(new DrydockBlueprintEditor()) diff --git a/src/applications/drydock/editor/DrydockBlueprintEditEngine.php b/src/applications/drydock/editor/DrydockBlueprintEditEngine.php index 020fe7514c..bd91aa3350 100644 --- a/src/applications/drydock/editor/DrydockBlueprintEditEngine.php +++ b/src/applications/drydock/editor/DrydockBlueprintEditEngine.php @@ -121,7 +121,7 @@ final class DrydockBlueprintEditEngine ->setKey('name') ->setLabel(pht('Name')) ->setDescription(pht('Name of the blueprint.')) - ->setTransactionType(DrydockBlueprintTransaction::TYPE_NAME) + ->setTransactionType(DrydockBlueprintNameTransaction::TRANSACTIONTYPE) ->setIsRequired(true) ->setValue($object->getBlueprintName()), ); diff --git a/src/applications/drydock/editor/DrydockBlueprintEditor.php b/src/applications/drydock/editor/DrydockBlueprintEditor.php index 125c36811a..aac3e7a020 100644 --- a/src/applications/drydock/editor/DrydockBlueprintEditor.php +++ b/src/applications/drydock/editor/DrydockBlueprintEditor.php @@ -11,6 +11,14 @@ final class DrydockBlueprintEditor return pht('Drydock Blueprints'); } + public function getCreateObjectTitle($author, $object) { + return pht('%s created this blueprint.', $author); + } + + public function getCreateObjectTitleForFeed($author, $object) { + return pht('%s created %s.', $author, $object); + } + protected function supportsSearch() { return true; } @@ -21,99 +29,7 @@ final class DrydockBlueprintEditor $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; - $types[] = DrydockBlueprintTransaction::TYPE_NAME; - $types[] = DrydockBlueprintTransaction::TYPE_DISABLED; - return $types; } - protected function getCustomTransactionOldValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case DrydockBlueprintTransaction::TYPE_NAME: - return $object->getBlueprintName(); - case DrydockBlueprintTransaction::TYPE_DISABLED: - return (int)$object->getIsDisabled(); - } - - return parent::getCustomTransactionOldValue($object, $xaction); - } - - protected function getCustomTransactionNewValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case DrydockBlueprintTransaction::TYPE_NAME: - return $xaction->getNewValue(); - case DrydockBlueprintTransaction::TYPE_DISABLED: - return (int)$xaction->getNewValue(); - } - - return parent::getCustomTransactionNewValue($object, $xaction); - } - - protected function applyCustomInternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case DrydockBlueprintTransaction::TYPE_NAME: - $object->setBlueprintName($xaction->getNewValue()); - return; - case DrydockBlueprintTransaction::TYPE_DISABLED: - $object->setIsDisabled((int)$xaction->getNewValue()); - return; - } - - return parent::applyCustomInternalTransaction($object, $xaction); - } - - protected function applyCustomExternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case DrydockBlueprintTransaction::TYPE_NAME: - case DrydockBlueprintTransaction::TYPE_DISABLED: - return; - } - - return parent::applyCustomExternalTransaction($object, $xaction); - } - - - protected function validateTransaction( - PhabricatorLiskDAO $object, - $type, - array $xactions) { - - $errors = parent::validateTransaction($object, $type, $xactions); - - switch ($type) { - case DrydockBlueprintTransaction::TYPE_NAME: - $missing = $this->validateIsEmptyTextField( - $object->getBlueprintName(), - $xactions); - - if ($missing) { - $error = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Required'), - pht('You must choose a name for this blueprint.'), - nonempty(last($xactions), null)); - - $error->setIsMissingFieldError(true); - $errors[] = $error; - continue; - } - - break; - } - - return $errors; - } - } diff --git a/src/applications/drydock/storage/DrydockBlueprintTransaction.php b/src/applications/drydock/storage/DrydockBlueprintTransaction.php index 0856f01fdb..68eebe8c1d 100644 --- a/src/applications/drydock/storage/DrydockBlueprintTransaction.php +++ b/src/applications/drydock/storage/DrydockBlueprintTransaction.php @@ -1,7 +1,7 @@ getOldValue(); - $new = $this->getNewValue(); - $author_handle = $this->renderHandleLink($this->getAuthorPHID()); - - switch ($this->getTransactionType()) { - case self::TYPE_NAME: - if (!strlen($old)) { - return pht( - '%s created this blueprint.', - $author_handle); - } else { - return pht( - '%s renamed this blueprint from "%s" to "%s".', - $author_handle, - $old, - $new); - } - case self::TYPE_DISABLED: - if ($new) { - return pht( - '%s disabled this blueprint.', - $author_handle); - } else { - return pht( - '%s enabled this blueprint.', - $author_handle); - } - } - - return parent::getTitle(); + public function getBaseTransactionClass() { + return 'DrydockBlueprintTransactionType'; } } diff --git a/src/applications/drydock/xaction/DrydockBlueprintDisableTransaction.php b/src/applications/drydock/xaction/DrydockBlueprintDisableTransaction.php new file mode 100644 index 0000000000..216cea35f4 --- /dev/null +++ b/src/applications/drydock/xaction/DrydockBlueprintDisableTransaction.php @@ -0,0 +1,48 @@ +getIsDisabled(); + } + + public function generateNewValue($object, $value) { + return (bool)$value; + } + + public function applyInternalEffects($object, $value) { + $object->setIsDisabled((int)$value); + } + + public function getTitle() { + $new = $this->getNewValue(); + if ($new) { + return pht( + '%s disabled this blueprint.', + $this->renderAuthor()); + } else { + return pht( + '%s enabled this blueprint.', + $this->renderAuthor()); + } + } + + public function getTitleForFeed() { + $new = $this->getNewValue(); + if ($new) { + return pht( + '%s disabled %s.', + $this->renderAuthor(), + $this->renderObject()); + } else { + return pht( + '%s enabled %s.', + $this->renderAuthor(), + $this->renderObject()); + } + } + +} diff --git a/src/applications/drydock/xaction/DrydockBlueprintNameTransaction.php b/src/applications/drydock/xaction/DrydockBlueprintNameTransaction.php new file mode 100644 index 0000000000..c3c17d0413 --- /dev/null +++ b/src/applications/drydock/xaction/DrydockBlueprintNameTransaction.php @@ -0,0 +1,60 @@ +getBlueprintName(); + } + + public function applyInternalEffects($object, $value) { + $object->setBlueprintName($value); + } + + public function getTitle() { + return pht( + '%s renamed this blueprint from %s to %s.', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function getTitleForFeed() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + return pht( + '%s renamed %s from %s to %s.', + $this->renderAuthor(), + $this->renderObject(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + $name = $object->getBlueprintName(); + if ($this->isEmptyTextTransaction($name, $xactions)) { + $errors[] = $this->newRequiredError( + pht('Blueprints must have a name.')); + } + + $max_length = $object->getColumnMaximumByteLength('blueprintName'); + foreach ($xactions as $xaction) { + $new_value = $xaction->getNewValue(); + + $new_length = strlen($new_value); + if ($new_length > $max_length) { + $errors[] = $this->newInvalidError( + pht('Blueprint names can be no longer than %s characters.', + new PhutilNumber($max_length))); + } + } + + return $errors; + } + +} diff --git a/src/applications/drydock/xaction/DrydockBlueprintTransactionType.php b/src/applications/drydock/xaction/DrydockBlueprintTransactionType.php new file mode 100644 index 0000000000..ecf021b3ad --- /dev/null +++ b/src/applications/drydock/xaction/DrydockBlueprintTransactionType.php @@ -0,0 +1,4 @@ + Date: Wed, 13 Dec 2017 07:47:18 -0800 Subject: [PATCH 06/14] Give EditEngine a Conduit-specific initialization pathway for objects Summary: Depends on D18845. See PHI243 for context and more details. Briefly, some objects need a "type" transaction (or something similar) very early on, and we can't generate their fields until we know the object type. Drydock blueprints are an example: a blueprint's fields depend on the blueprint's type. In web interfaces, the workflow just forces the user to select a type first. For Conduit workflows, I think the cleanest approach is to proactively extract and apply type information before processing the request. This will make the implementation a little messier, but the API cleaner. An alternative is to add more fields to the API, like a "type" field. This makes the implementation cleaner, but the API messier. I think we're better off favoring a cleaner API here. This change just makes it possible for `DrydockBlueprintEditEngine` to look at the incoming transactions and extract a "type"; it doesn't actually change any behavior. Test Plan: Performed edits via API, but this change doesn't alter any behavior. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18847 --- .../editengine/PhabricatorEditEngine.php | 67 +++++++++++++------ 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 8ba49f4d77..1e9d936929 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -630,6 +630,17 @@ abstract class PhabricatorEditEngine return $this->isCreate; } + /** + * Initialize a new object for object creation via Conduit. + * + * @return object Newly initialized object. + * @param list Raw transactions. + * @task load + */ + protected function newEditableObjectFromConduit(array $raw_xactions) { + return $this->newEditableObject(); + } + /** * Initialize a new object for documentation creation. * @@ -2031,6 +2042,8 @@ abstract class PhabricatorEditEngine get_class($this))); } + $raw_xactions = $this->getRawConduitTransactions($request); + $identifier = $request->getValue('objectIdentifier'); if ($identifier) { $this->setIsCreate(false); @@ -2039,7 +2052,7 @@ abstract class PhabricatorEditEngine $this->requireCreateCapability(); $this->setIsCreate(true); - $object = $this->newEditableObject(); + $object = $this->newEditableObjectFromConduit($raw_xactions); } $this->validateObject($object); @@ -2049,7 +2062,11 @@ abstract class PhabricatorEditEngine $types = $this->getConduitEditTypesFromFields($fields); $template = $object->getApplicationTransactionTemplate(); - $xactions = $this->getConduitTransactions($request, $types, $template); + $xactions = $this->getConduitTransactions( + $request, + $raw_xactions, + $types, + $template); $editor = $object->getApplicationTransactionEditor() ->setActor($viewer) @@ -2078,23 +2095,7 @@ abstract class PhabricatorEditEngine ); } - - /** - * Generate transactions which can be applied from edit actions in a Conduit - * request. - * - * @param ConduitAPIRequest The request. - * @param list Supported edit types. - * @param PhabricatorApplicationTransaction Template transaction. - * @return list Generated transactions. - * @task conduit - */ - private function getConduitTransactions( - ConduitAPIRequest $request, - array $types, - PhabricatorApplicationTransaction $template) { - - $viewer = $request->getUser(); + private function getRawConduitTransactions(ConduitAPIRequest $request) { $transactions_key = 'transactions'; $xactions = $request->getValue($transactions_key); @@ -2124,7 +2125,33 @@ abstract class PhabricatorEditEngine $transactions_key, $key)); } + } + return $xactions; + } + + + /** + * Generate transactions which can be applied from edit actions in a Conduit + * request. + * + * @param ConduitAPIRequest The request. + * @param list Raw conduit transactions. + * @param list Supported edit types. + * @param PhabricatorApplicationTransaction Template transaction. + * @return list Generated transactions. + * @task conduit + */ + private function getConduitTransactions( + ConduitAPIRequest $request, + array $xactions, + array $types, + PhabricatorApplicationTransaction $template) { + + $viewer = $request->getUser(); + $results = array(); + + foreach ($xactions as $key => $xaction) { $type = $xaction['type']; if (empty($types[$type])) { throw new Exception( @@ -2137,8 +2164,6 @@ abstract class PhabricatorEditEngine } } - $results = array(); - if ($this->getIsCreate()) { $results[] = id(clone $template) ->setTransactionType(PhabricatorTransactions::TYPE_CREATE); From f3f1f9dc577ac9675accae0f5870e55a08a3f258 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Dec 2017 10:17:27 -0800 Subject: [PATCH 07/14] Allow "drydock.blueprint.edit" to create blueprints Summary: Depends on D18848. Ref PHI243. This puts a bit of logic up front to figure out the blueprint type before we actually start editing it. This implementation is a little messy but it keeps the API clean. Eventually, the implementation could probably go in the TransactionTypes so more code is shared, but I'd like to wait for a couple more of these first. This capability probably isn't too useful, but just pays down a bit of technical debt from the caveat introduced in D18822. Test Plan: - Created a new blueprint with the API. - Tried to create a blueprint without a "type" (got a helpful error). - Created and edited blueprints via the web UI. - Tried to change the "type" of an existing blueprint (got a helpful error). Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D18849 --- src/__phutil_library_map__.php | 2 + .../DrydockBlueprintEditConduitAPIMethod.php | 3 +- .../editor/DrydockBlueprintEditEngine.php | 45 ++++++++++++++- .../DrydockBlueprintTypeTransaction.php | 57 +++++++++++++++++++ 4 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 src/applications/drydock/xaction/DrydockBlueprintTypeTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0a005eef4a..f4a410ad2e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1025,6 +1025,7 @@ phutil_register_library_map(array( 'DrydockBlueprintTransaction' => 'applications/drydock/storage/DrydockBlueprintTransaction.php', 'DrydockBlueprintTransactionQuery' => 'applications/drydock/query/DrydockBlueprintTransactionQuery.php', 'DrydockBlueprintTransactionType' => 'applications/drydock/xaction/DrydockBlueprintTransactionType.php', + 'DrydockBlueprintTypeTransaction' => 'applications/drydock/xaction/DrydockBlueprintTypeTransaction.php', 'DrydockBlueprintViewController' => 'applications/drydock/controller/DrydockBlueprintViewController.php', 'DrydockCommand' => 'applications/drydock/storage/DrydockCommand.php', 'DrydockCommandError' => 'applications/drydock/exception/DrydockCommandError.php', @@ -6122,6 +6123,7 @@ phutil_register_library_map(array( 'DrydockBlueprintTransaction' => 'PhabricatorModularTransaction', 'DrydockBlueprintTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'DrydockBlueprintTransactionType' => 'PhabricatorModularTransactionType', + 'DrydockBlueprintTypeTransaction' => 'DrydockBlueprintTransactionType', 'DrydockBlueprintViewController' => 'DrydockBlueprintController', 'DrydockCommand' => array( 'DrydockDAO', diff --git a/src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php b/src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php index 764c077d87..7a120f7c5b 100644 --- a/src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php +++ b/src/applications/drydock/conduit/DrydockBlueprintEditConduitAPIMethod.php @@ -13,8 +13,7 @@ final class DrydockBlueprintEditConduitAPIMethod public function getMethodSummary() { return pht( - 'WARNING: Apply transactions to edit an existing blueprint. This method '. - 'can not create new blueprints.'); + 'Apply transactions to create or edit a blueprint.'); } } diff --git a/src/applications/drydock/editor/DrydockBlueprintEditEngine.php b/src/applications/drydock/editor/DrydockBlueprintEditEngine.php index bd91aa3350..6bd1366ef0 100644 --- a/src/applications/drydock/editor/DrydockBlueprintEditEngine.php +++ b/src/applications/drydock/editor/DrydockBlueprintEditEngine.php @@ -51,6 +51,38 @@ final class DrydockBlueprintEditEngine return $blueprint; } + protected function newEditableObjectFromConduit(array $raw_xactions) { + $type = null; + foreach ($raw_xactions as $raw_xaction) { + if ($raw_xaction['type'] !== 'type') { + continue; + } + + $type = $raw_xaction['value']; + } + + if ($type === null) { + throw new Exception( + pht( + 'When creating a new Drydock blueprint via the Conduit API, you '. + 'must provide a "type" transaction to select a type.')); + } + + $map = DrydockBlueprintImplementation::getAllBlueprintImplementations(); + if (!isset($map[$type])) { + throw new Exception( + pht( + 'Blueprint type "%s" is unrecognized. Valid types are: %s.', + $type, + implode(', ', array_keys($map)))); + } + + $impl = clone $map[$type]; + $this->setBlueprintImplementation($impl); + + return $this->newEditableObject(); + } + protected function newEditableObjectForDocumentation() { // In order to generate the proper list of fields/transactions for a // blueprint, a blueprint's type needs to be known upfront, and there's @@ -112,11 +144,22 @@ final class DrydockBlueprintEditEngine $impl = $object->getImplementation(); return array( + // This field appears in the web UI id(new PhabricatorStaticEditField()) - ->setKey('type') + ->setKey('displayType') ->setLabel(pht('Blueprint Type')) ->setDescription(pht('Type of blueprint.')) ->setValue($impl->getBlueprintName()), + id(new PhabricatorTextEditField()) + ->setKey('type') + ->setLabel(pht('Type')) + ->setIsConduitOnly(true) + ->setTransactionType( + DrydockBlueprintTypeTransaction::TRANSACTIONTYPE) + ->setDescription(pht('When creating a blueprint, set the type.')) + ->setConduitDescription(pht('Set the blueprint type.')) + ->setConduitTypeDescription(pht('Blueprint type.')) + ->setValue($object->getClassName()), id(new PhabricatorTextEditField()) ->setKey('name') ->setLabel(pht('Name')) diff --git a/src/applications/drydock/xaction/DrydockBlueprintTypeTransaction.php b/src/applications/drydock/xaction/DrydockBlueprintTypeTransaction.php new file mode 100644 index 0000000000..024e6092fa --- /dev/null +++ b/src/applications/drydock/xaction/DrydockBlueprintTypeTransaction.php @@ -0,0 +1,57 @@ +getClassName(); + } + + public function applyInternalEffects($object, $value) { + $object->setClassName($value); + } + + public function getTitle() { + // These transactions can only be applied during object creation and never + // generate a timeline event. + return null; + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + $name = $object->getClassName(); + if ($this->isEmptyTextTransaction($name, $xactions)) { + $errors[] = $this->newRequiredError( + pht('You must select a blueprint type when creating a blueprint.')); + } + + $map = DrydockBlueprintImplementation::getAllBlueprintImplementations(); + + foreach ($xactions as $xaction) { + if (!$this->isNewObject()) { + $errors[] = $this->newInvalidError( + pht( + 'The type of a blueprint can not be changed once it has '. + 'been created.'), + $xaction); + continue; + } + + $new = $xaction->getNewValue(); + if (!isset($map[$new])) { + $errors[] = $this->newInvalidError( + pht( + 'Blueprint type "%s" is not valid. Valid types are: %s.', + $new, + implode(', ', array_keys($map)))); + continue; + } + } + + return $errors; + } + +} From 0f02d79ffa9d5f20d3687953f7276076cf0a82cf Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 12:22:27 -0800 Subject: [PATCH 08/14] Remove nonfunctional Mercurial "bundle2" capability filtering from SSH pathway Summary: Ref T13036. This code attempts to filter the "capabilities" message to remove "bundle2", but I think this has never worked. Specifically, the //write// pathway is hooked, and "write" here means "client is writing a message to the server". However, the "capabilities" frame is part of the response, not part of the request. Thus, this code never fires, at least on recent versions of Mercurial. Since I plan to support bundle2 and don't want to decode response frames, just get rid of this, assuming we'll achieve those goals. I think this was just overlooked in D14241, which probably focused on the HTTP version. This code does (at least, potentially) do something for HTTP. I'm leaving the actual "strip stuff" code in place for now since I think it's still used on the HTTP pathway. Test Plan: - Added debug logging, saw this code never hit even though `hg push --debug` shows the client believing bundle2 is supported. - Logged both halves of the wire protocol and saw this come from the server, not the client. - Ran the failing `hg push` of a 4MB file under hg 4.4.1, got the same error as before. Reviewers: amckinley Reviewed By: amckinley Subscribers: cspeckmim Maniphest Tasks: T13036 Differential Revision: https://secure.phabricator.com/D18855 --- .../diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php index 4508ae53da..da877b7314 100644 --- a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php @@ -109,14 +109,7 @@ final class DiffusionMercurialServeSSHWorkflow $this->didSeeWrite = true; } - $raw_message = $message['raw']; - if ($command == 'capabilities') { - $raw_message = DiffusionMercurialWireProtocol::filterBundle2Capability( - $raw_message); - } - - // If we're good, return the raw message data. - return $raw_message; + return $message['raw']; } protected function raiseWrongVCSException( From 3a4e14431fce5b80c57c1f62ed6003ee93575ac6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 12:46:38 -0800 Subject: [PATCH 09/14] Remove an obsolete comment about Mercurial SSH error behavior Summary: Depends on D18855. Ref T13036. This comment no longer seems to be accurate: anything we send over `stderr` is faithfully shown to the user with recent clients. From [[ https://www.mercurial-scm.org/repo/hg/file/default/mercurial/help/internals/wireprotocol.txt | this document ]], the missing sauce may have been: ``` A generic error response type is also supported. It consists of a an error message written to ``stderr`` followed by ``\n-\n``. In addition, ``\n`` is written to ``stdout``. ``` That is, writing "\n" to stdout in addition to writing the error to stderr. However, this no longer appears to be necessary. I think the modern client behavior is generally sensible (and consistent with the behavior of Git and Subversion) so this //probably// isn't a bug or me making a mistake. Test Plan: With a modern client, threw some arbitrary exception during execution. Observed a helpful message on the client with no additional steps. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13036 Differential Revision: https://secure.phabricator.com/D18856 --- .../ssh/DiffusionMercurialServeSSHWorkflow.php | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php index da877b7314..5701a4bac1 100644 --- a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php @@ -66,17 +66,6 @@ final class DiffusionMercurialServeSSHWorkflow ->setWillWriteCallback(array($this, 'willWriteMessageCallback')) ->execute(); - // TODO: It's apparently technically possible to communicate errors to - // Mercurial over SSH by writing a special "\n\n-\n" string. However, - // my attempt to implement that resulted in Mercurial closing the socket and - // then hanging, without showing the error. This might be an issue on our - // side (we need to close our half of the socket?), or maybe the code - // for this in Mercurial doesn't actually work, or maybe something else - // is afoot. At some point, we should look into doing this more cleanly. - // For now, when we, e.g., reject writes for policy reasons, the user will - // see "abort: unexpected response: empty string" after the diagnostically - // useful, e.g., "remote: This repository is read-only over SSH." message. - if (!$err && $this->didSeeWrite) { $repository->writeStatusMessage( PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE, From 13c8963dab27b5ee24a87c5f4ebbf18be585dc19 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 13:41:20 -0800 Subject: [PATCH 10/14] Fix a Mercurial wire protocol parser issue when we receive a length frame before any data Summary: Depends on D18856. Ref T13036. See PHI275. When we receive a length frame but the buffer doesn't have any data yet, we currently emit a pointless 0-length data frame on the channel. For normal chatter this is harmless/valid, but it causes problems when a channel has transitioned into bundle2 mode (probably it indicates "end of stream")? In any case, it's never helpful, so if we're about to read a data block and don't have any data, just bail out until we see some more data. Note that we can't end up here //expecting// a 0-length data block: both the `data-length` and `data-bytes` states already handle that properly. Test Plan: Pushed 4MB changes to a Mercurial repository with Mercurial 4.1.1, was no longer able to hit channel errors. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13036 Differential Revision: https://secure.phabricator.com/D18857 --- .../ssh/DiffusionMercurialWireClientSSHProtocolChannel.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php b/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php index 5150ef9352..4c16fb9f0f 100644 --- a/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php +++ b/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php @@ -192,6 +192,13 @@ final class DiffusionMercurialWireClientSSHProtocolChannel $this->state = 'data-bytes'; } } else if ($this->state == 'data-bytes') { + // If we don't have any more bytes on the buffer yet, just bail: + // otherwise, we'll emit a pointless and possibly harmful 0-byte data + // frame. See T13036 for discussion. + if (!strlen($this->buffer)) { + break; + } + $bytes = substr($this->buffer, 0, $this->expectBytes); $this->buffer = substr($this->buffer, strlen($bytes)); $this->expectBytes -= strlen($bytes); From 94db95a165e710e30366c52d99ef1f5f289fbac3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 10:18:45 -0800 Subject: [PATCH 11/14] Sort burnup data chronologically after merging synthetic and "real" data Summary: Ref T13020. See PHI273. See D18853. On `secure`, the chart looks less promising than it did locally, and is full of discontinuities: {F5356544} I think this is a sorting issue. But if I can't fake my way through this soon I'll maybe get the Fact engine running and use it to provide the data here, as a sort of half-step toward T1562? Test Plan: Chart looks the same locally, will push and see if `secure` improves? Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13020 Differential Revision: https://secure.phabricator.com/D18854 --- .../maniphest/controller/ManiphestReportController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php index 821c96a2aa..5633091450 100644 --- a/src/applications/maniphest/controller/ManiphestReportController.php +++ b/src/applications/maniphest/controller/ManiphestReportController.php @@ -153,6 +153,7 @@ final class ManiphestReportController extends ManiphestController { // Merge the synthetic rows into the real transactions. $data = array_merge($create_rows, $data); $data = array_values($data); + $data = isort($data, 'dateCreated'); $stats = array(); $day_buckets = array(); From 554359203467986d2873f274364f80e2bc640239 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 14:11:54 -0800 Subject: [PATCH 12/14] Add a couple of clarifying comments to the Mercurial protocol parser Summary: See D18857. Ref T13036. See PHI275. Explain what's going on here a little better since it isn't entirely obvious and debugging these stream parsers is a gigantic pain. Test Plan: Read text. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13036 Differential Revision: https://secure.phabricator.com/D18859 --- ...nMercurialWireClientSSHProtocolChannel.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php b/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php index 4c16fb9f0f..8eb026d8fe 100644 --- a/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php +++ b/src/applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php @@ -180,6 +180,15 @@ final class DiffusionMercurialWireClientSSHProtocolChannel $this->state = 'arguments'; } } else if ($this->state == 'data-length') { + + // We're reading the length of a chunk of raw data. It looks like + // this: + // + // \n + // + // The length is human-readable text (for example, "4096"), and + // may be 0. + $line = $this->readProtocolLine(); if ($line === null) { break; @@ -192,6 +201,9 @@ final class DiffusionMercurialWireClientSSHProtocolChannel $this->state = 'data-bytes'; } } else if ($this->state == 'data-bytes') { + + // We're reading some known, nonzero number of raw bytes of data. + // If we don't have any more bytes on the buffer yet, just bail: // otherwise, we'll emit a pointless and possibly harmful 0-byte data // frame. See T13036 for discussion. @@ -203,6 +215,13 @@ final class DiffusionMercurialWireClientSSHProtocolChannel $this->buffer = substr($this->buffer, strlen($bytes)); $this->expectBytes -= strlen($bytes); + // NOTE: We emit a data frame as soon as we read some data. This can + // cause us to repackage frames: for example, if we receive one large + // frame slowly, we may emit it as several smaller frames. In theory + // this is good; in practice, Mercurial never seems to select a frame + // size larger than 4096 bytes naturally and this may be more + // complexity and trouble than it is worth. See T13036. + $messages[] = $this->newDataMessage($bytes); if (!$this->expectBytes) { From adbd7d4fd8a2c80b7f27c10c1432b6eef0196ae4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Jan 2018 14:44:01 -0800 Subject: [PATCH 13/14] Make the new synthetic burnup chart data respect the "Project" filter Summary: See PHI273. Third time's the charm? This page has a "Project" filter which lets you view data for only one project, but the synthetic data currently ignores it. Test Plan: Filtered burnup chart by various projects, saw sensible-looking data. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18860 --- .../controller/ManiphestReportController.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php index 5633091450..3ba597fb92 100644 --- a/src/applications/maniphest/controller/ManiphestReportController.php +++ b/src/applications/maniphest/controller/ManiphestReportController.php @@ -75,6 +75,7 @@ final class ManiphestReportController extends ManiphestController { $conn = $table->establishConnection('r'); $joins = ''; + $create_joins = ''; if ($project_phid) { $joins = qsprintf( $conn, @@ -84,6 +85,12 @@ final class ManiphestReportController extends ManiphestController { PhabricatorEdgeConfig::TABLE_NAME_EDGE, PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, $project_phid); + $create_joins = qsprintf( + $conn, + 'JOIN %T p ON p.src = t.phid AND p.type = %d AND p.dst = %s', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + $project_phid); } $data = queryfx_all( @@ -113,8 +120,9 @@ final class ManiphestReportController extends ManiphestController { // default value. $create_rows = queryfx_all( $conn, - 'SELECT dateCreated FROM %T', - id(new ManiphestTask())->getTableName()); + 'SELECT t.dateCreated FROM %T t %Q', + id(new ManiphestTask())->getTableName(), + $create_joins); foreach ($create_rows as $key => $create_row) { $create_rows[$key] = array( 'transactionType' => 'status', From 2140741e25288b05671e60c8954ffc0c2be7b624 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Sat, 6 Jan 2018 07:25:26 -0800 Subject: [PATCH 14/14] Fix typo in new setting description Summary: Noticed by @amckinley in https://secure.phabricator.com/D18850#inline-57246 but not fixed before landing. Test Plan: ispell Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, amckinley, epriestley Differential Revision: https://secure.phabricator.com/D18861 --- .../diffusion/editor/DiffusionRepositoryEditEngine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 4eebc09b52..dcb79a6c86 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -318,7 +318,7 @@ final class DiffusionRepositoryEditEngine pht('Prevent Enormous Changes'), pht('Allow Enormous Changes')) ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_ENORMOUS) - ->setDescription(pht('Permit enomrous changes to be made.')) + ->setDescription(pht('Permit enormous changes to be made.')) ->setConduitDescription(pht('Allow or prevent enormous changes.')) ->setConduitTypeDescription(pht('New protection setting.')) ->setValue($object->shouldAllowEnormousChanges()),