D10869.id26103.diff
No OneTemporary
Actions

Size

20 KB

Referenced Files

None

Subscribers

None

D10869.id26103.diff
View Options

	diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
	--- a/src/__phutil_library_map__.php
	+++ b/src/__phutil_library_map__.php
	@@ -3313,7 +3313,7 @@
	),
	'DifferentialDiffCreateController' => 'DifferentialController',
	'DifferentialDiffCreationRejectException' => 'Exception',
	- 'DifferentialDiffEditor' => 'PhabricatorEditor',
	+ 'DifferentialDiffEditor' => 'PhabricatorApplicationTransactionEditor',
	'DifferentialDiffPHIDType' => 'PhabricatorPHIDType',
	'DifferentialDiffProperty' => 'DifferentialDAO',
	'DifferentialDiffQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
	diff --git a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php
	--- a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php
	+++ b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php
	@@ -70,19 +70,13 @@
	}

	$diff = DifferentialDiff::newFromRawChanges($changes);
	- $diff->setSourcePath($request->getValue('sourcePath'));
	- $diff->setSourceMachine($request->getValue('sourceMachine'));

	- $diff->setBranch($request->getValue('branch'));
	- $diff->setCreationMethod($request->getValue('creationMethod'));
	- $diff->setAuthorPHID($viewer->getPHID());
	- $diff->setBookmark($request->getValue('bookmark'));
	-
	- // TODO: Remove this eventually; for now continue writing the UUID. Note
	- // that we'll overwrite it below if we identify a repository, and `arc`
	- // no longer sends it. This stuff is retained for backward compatibility.
	- $diff->setRepositoryUUID($request->getValue('repositoryUUID'));
	+ // TODO: Remove repository UUID eventually; for now continue writing
	+ // the UUID. Note that we'll overwrite it below if we identify a
	+ // repository, and `arc` no longer sends it. This stuff is retained for
	+ // backward compatibility.

	+ $repository_uuid = $request->getValue('repositoryUUID');
	$repository_phid = $request->getValue('repositoryPHID');
	if ($repository_phid) {
	$repository = id(new PhabricatorRepositoryQuery())
	@@ -90,17 +84,11 @@
	->withPHIDs(array($repository_phid))
	->executeOne();
	if ($repository) {
	- $diff->setRepositoryPHID($repository->getPHID());
	- $diff->setRepositoryUUID($repository->getUUID());
	+ $repository_phid = $repository->getPHID();
	+ $repository_uuid = $repository->getUUID();
	}
	}

	- $system = $request->getValue('sourceControlSystem');
	- $diff->setSourceControlSystem($system);
	- $diff->setSourceControlPath($request->getValue('sourceControlPath'));
	- $diff->setSourceControlBaseRevision(
	- $request->getValue('sourceControlBaseRevision'));
	-
	$project_name = $request->getValue('arcanistProject');
	$project_phid = null;
	if ($project_name) {
	@@ -116,73 +104,75 @@
	$project_phid = $arcanist_project->getPHID();
	}

	- $diff->setArcanistProjectPHID($project_phid);
	-
	switch ($request->getValue('lintStatus')) {
	case 'skip':
	- $diff->setLintStatus(DifferentialLintStatus::LINT_SKIP);
	+ $lint_status = DifferentialLintStatus::LINT_SKIP;
	break;
	case 'okay':
	- $diff->setLintStatus(DifferentialLintStatus::LINT_OKAY);
	+ $lint_status = DifferentialLintStatus::LINT_OKAY;
	break;
	case 'warn':
	- $diff->setLintStatus(DifferentialLintStatus::LINT_WARN);
	+ $lint_status = DifferentialLintStatus::LINT_WARN;
	break;
	case 'fail':
	- $diff->setLintStatus(DifferentialLintStatus::LINT_FAIL);
	+ $lint_status = DifferentialLintStatus::LINT_FAIL;
	break;
	case 'postponed':
	- $diff->setLintStatus(DifferentialLintStatus::LINT_POSTPONED);
	+ $lint_status = DifferentialLintStatus::LINT_POSTPONED;
	break;
	case 'none':
	default:
	- $diff->setLintStatus(DifferentialLintStatus::LINT_NONE);
	+ $lint_status = DifferentialLintStatus::LINT_NONE;
	break;
	}

	switch ($request->getValue('unitStatus')) {
	case 'skip':
	- $diff->setUnitStatus(DifferentialUnitStatus::UNIT_SKIP);
	+ $unit_status = DifferentialUnitStatus::UNIT_SKIP;
	break;
	case 'okay':
	- $diff->setUnitStatus(DifferentialUnitStatus::UNIT_OKAY);
	+ $unit_status = DifferentialUnitStatus::UNIT_OKAY;
	break;
	case 'warn':
	- $diff->setUnitStatus(DifferentialUnitStatus::UNIT_WARN);
	+ $unit_status = DifferentialUnitStatus::UNIT_WARN;
	break;
	case 'fail':
	- $diff->setUnitStatus(DifferentialUnitStatus::UNIT_FAIL);
	+ $unit_status = DifferentialUnitStatus::UNIT_FAIL;
	break;
	case 'postponed':
	- $diff->setUnitStatus(DifferentialUnitStatus::UNIT_POSTPONED);
	+ $unit_status = DifferentialUnitStatus::UNIT_POSTPONED;
	break;
	case 'none':
	default:
	- $diff->setUnitStatus(DifferentialUnitStatus::UNIT_NONE);
	+ $unit_status = DifferentialUnitStatus::UNIT_NONE;
	break;
	}

	+ $diff_data_dict = array(
	+ 'sourcePath' => $request->getValue('sourcePath'),
	+ 'sourceMachine' => $request->getValue('sourceMachine'),
	+ 'branch' => $request->getValue('branch'),
	+ 'creationMethod' => $request->getValue('creationMethod'),
	+ 'authorPHID' => $viewer->getPHID(),
	+ 'bookmark' => $request->getValue('bookmark'),
	+ 'repositoryUUID' => $repository_uuid,
	+ 'repositoryPHID' => $repository_phid,
	+ 'sourceControlSystem' => $request->getValue('sourceControlSystem'),
	+ 'sourceControlPath' => $request->getValue('sourceControlPath'),
	+ 'sourceControlBaseRevision' =>
	+ $request->getValue('sourceControlBaseRevision'),
	+ 'arcanistProjectPHID' => $project_phid,
	+ 'lintStatus' => $lint_status,
	+ 'unitStatus' => $unit_status,);
	+
	+ $xactions = array(id(new DifferentialTransaction())
	+ ->setTransactionType(DifferentialTransaction::TYPE_DIFF)
	+ ->setNewValue($diff_data_dict),);
	+
	id(new DifferentialDiffEditor())
	->setActor($viewer)
	- ->setContentSource(
	- PhabricatorContentSource::newFromConduitRequest($request))
	- ->saveDiff($diff);
	-
	- // If we didn't get an explicit `repositoryPHID` (which means the client is
	- // old, or couldn't figure out which repository the working copy belongs
	- // to), apply heuristics to try to figure it out.
	-
	- if (!$repository_phid) {
	- $repository = id(new DifferentialRepositoryLookup())
	- ->setDiff($diff)
	- ->setViewer($viewer)
	- ->lookupRepository();
	- if ($repository) {
	- $diff->setRepositoryPHID($repository->getPHID());
	- $diff->setRepositoryUUID($repository->getUUID());
	- $diff->save();
	- }
	- }
	+ ->setContentSourceFromConduitRequest($request)
	+ ->applyTransactions($diff, $xactions);

	$path = '/differential/diff/'.$diff->getID().'/';
	$uri = PhabricatorEnv::getURI($path);
	diff --git a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php
	--- a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php
	+++ b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php
	@@ -41,29 +41,27 @@
	throw new Exception(
	pht('No such repository "%s"!', $repository_phid));
	}
	- } else {
	- $repository = null;
	}

	$parser = new ArcanistDiffParser();
	$changes = $parser->parseDiff($raw_diff);
	$diff = DifferentialDiff::newFromRawChanges($changes);

	- $diff->setLintStatus(DifferentialLintStatus::LINT_SKIP);
	- $diff->setUnitStatus(DifferentialUnitStatus::UNIT_SKIP);
	+ $diff_data_dict = array(
	+ 'creationMethod' => 'web',
	+ 'authorPHID' => $viewer->getPHID(),
	+ 'repositoryPHID' => $repository_phid,
	+ 'lintStatus' => DifferentialLintStatus::LINT_SKIP,
	+ 'unitStatus' => DifferentialUnitStatus::UNIT_SKIP,);

	- $diff->setAuthorPHID($viewer->getPHID());
	- $diff->setCreationMethod('web');
	-
	- if ($repository) {
	- $diff->setRepositoryPHID($repository->getPHID());
	- }
	+ $xactions = array(id(new DifferentialTransaction())
	+ ->setTransactionType(DifferentialTransaction::TYPE_DIFF)
	+ ->setNewValue($diff_data_dict),);

	id(new DifferentialDiffEditor())
	->setActor($viewer)
	- ->setContentSource(
	- PhabricatorContentSource::newFromConduitRequest($request))
	- ->saveDiff($diff);
	+ ->setContentSourceFromConduitRequest($request)
	+ ->applyTransactions($diff, $xactions);

	return $this->buildDiffInfoDictionary($diff);
	}
	diff --git a/src/applications/differential/controller/DifferentialDiffCreateController.php b/src/applications/differential/controller/DifferentialDiffCreateController.php
	--- a/src/applications/differential/controller/DifferentialDiffCreateController.php
	+++ b/src/applications/differential/controller/DifferentialDiffCreateController.php
	@@ -9,6 +9,7 @@
	$errors = array();
	$e_diff = null;
	$e_file = null;
	+ $validation_exception = null;
	if ($request->isFormPost()) {
	$diff = null;

	@@ -27,16 +28,19 @@
	}

	if (!$errors) {
	- $call = new ConduitCall(
	- 'differential.createrawdiff',
	- array(
	- 'diff' => $diff,
	+ try {
	+ $call = new ConduitCall(
	+ 'differential.createrawdiff',
	+ array(
	+ 'diff' => $diff,
	));
	- $call->setUser($request->getUser());
	- $result = $call->execute();
	-
	- $path = id(new PhutilURI($result['uri']))->getPath();
	- return id(new AphrontRedirectResponse())->setURI($path);
	+ $call->setUser($request->getUser());
	+ $result = $call->execute();
	+ $path = id(new PhutilURI($result['uri']))->getPath();
	+ return id(new AphrontRedirectResponse())->setURI($path);
	+ } catch (PhabricatorApplicationTransactionValidationException $ex) {
	+ $validation_exception = $ex;
	+ }
	}
	}

	@@ -83,6 +87,7 @@

	$form_box = id(new PHUIObjectBoxView())
	->setHeaderText(pht('Create New Diff'))
	+ ->setValidationException($validation_exception)
	->setForm($form)
	->setFormErrors($errors);

	diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php
	--- a/src/applications/differential/editor/DifferentialDiffEditor.php
	+++ b/src/applications/differential/editor/DifferentialDiffEditor.php
	@@ -1,80 +1,231 @@
	<?php

	-final class DifferentialDiffEditor extends PhabricatorEditor {
	+final class DifferentialDiffEditor
	+ extends PhabricatorApplicationTransactionEditor {

	- private $contentSource;
	+ private $diffDataDict;

	- public function setContentSource($content_source) {
	- $this->contentSource = $content_source;
	- return $this;
	+ public function getEditorApplicationClass() {
	+ return 'PhabricatorDifferentialApplication';
	}

	- public function getContentSource() {
	- return $this->contentSource;
	+ public function getEditorObjectsDescription() {
	+ return pht('Differential Diffs');
	}

	- public function saveDiff(DifferentialDiff $diff) {
	- $actor = $this->requireActor();
	+ public function getTransactionTypes() {
	+ $types = parent::getTransactionTypes();

	- // Generate a PHID first, so the transcript will point at the object if
	- // we deicde to preserve it.
	- $phid = $diff->generatePHID();
	- $diff->setPHID($phid);
	+ $types[] = DifferentialTransaction::TYPE_DIFF;

	- $adapter = id(new HeraldDifferentialDiffAdapter())
	- ->setDiff($diff);
	+ return $types;
	+ }

	- $adapter->setContentSource($this->getContentSource());
	- $adapter->setIsNewObject(true);
	+ protected function getCustomTransactionOldValue(
	+ PhabricatorLiskDAO $object,
	+ PhabricatorApplicationTransaction $xaction) {

	- $engine = new HeraldEngine();
	+ switch ($xaction->getTransactionType()) {
	+ case DifferentialTransaction::TYPE_DIFF:
	+ return null;
	+ }

	- $rules = $engine->loadRulesForAdapter($adapter);
	- $rules = mpull($rules, null, 'getID');
	+ return parent::getCustomTransactionOldValue($object, $xaction);
	+ }

	- $effects = $engine->applyRules($rules, $adapter);
	+ protected function getCustomTransactionNewValue(
	+ PhabricatorLiskDAO $object,
	+ PhabricatorApplicationTransaction $xaction) {

	- $blocking_effect = null;
	- foreach ($effects as $effect) {
	- if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) {
	- $blocking_effect = $effect;
	- break;
	- }
	+ switch ($xaction->getTransactionType()) {
	+ case DifferentialTransaction::TYPE_DIFF:
	+ $this->diffDataDict = $xaction->getNewValue();
	+ return true;
	}

	- if ($blocking_effect) {
	- $rule = idx($rules, $effect->getRuleID());
	- if ($rule && strlen($rule->getName())) {
	- $rule_name = $rule->getName();
	- } else {
	- $rule_name = pht('Unnamed Herald Rule');
	- }
	+ return parent::getCustomTransactionNewValue($object, $xaction);
	+ }
	+
	+ protected function applyCustomInternalTransaction(
	+ PhabricatorLiskDAO $object,
	+ PhabricatorApplicationTransaction $xaction) {
	+
	+ switch ($xaction->getTransactionType()) {
	+ case DifferentialTransaction::TYPE_DIFF:
	+ $dict = $this->diffDataDict;
	+ $this->updateDiffFromDict($object, $dict);
	+ return;
	+ }

	- $message = $effect->getTarget();
	- if (!strlen($message)) {
	- $message = pht('(None.)');
	+ return parent::applyCustomInternalTransaction($object, $xaction);
	+ }
	+
	+ protected function applyCustomExternalTransaction(
	+ PhabricatorLiskDAO $object,
	+ PhabricatorApplicationTransaction $xaction) {
	+
	+ switch ($xaction->getTransactionType()) {
	+ case DifferentialTransaction::TYPE_DIFF:
	+ return;
	+ }
	+
	+ return parent::applyCustomExternalTransaction($object, $xaction);
	+ }
	+
	+ protected function applyFinalEffects(
	+ PhabricatorLiskDAO $object,
	+ array $xactions) {
	+
	+ // If we didn't get an explicit `repositoryPHID` (which means the client
	+ // is old, or couldn't figure out which repository the working copy
	+ // belongs to), apply heuristics to try to figure it out.
	+
	+ if (!$object->getRepositoryPHID()) {
	+ $repository = id(new DifferentialRepositoryLookup())
	+ ->setDiff($object)
	+ ->setViewer($this->getActor())
	+ ->lookupRepository();
	+ if ($repository) {
	+ $object->setRepositoryPHID($repository->getPHID());
	+ $object->setRepositoryUUID($repository->getUUID());
	+ $object->save();
	}
	+ }
	+
	+ return $xactions;
	+ }

	- throw new DifferentialDiffCreationRejectException(
	- pht(
	- "Creation of this diff was rejected by Herald rule %s.\n".
	- " Rule: %s\n".
	- "Reason: %s",
	- 'H'.$effect->getRuleID(),
	- $rule_name,
	- $message));
	+ /**
	+ * We run Herald as part of transaction validation because Herald can
	+ * block diff creation for Differential diffs. Its important to do this
	+ * separately so no Herald logs are saved; these logs could expose
	+ * information the Herald rules are inteneded to block.
	+ */
	+ protected function validateTransaction(
	+ PhabricatorLiskDAO $object,
	+ $type,
	+ array $xactions) {
	+
	+ $errors = parent::validateTransaction($object, $type, $xactions);
	+
	+ foreach ($xactions as $xaction) {
	+ switch ($type) {
	+ case DifferentialTransaction::TYPE_DIFF:
	+ $diff = clone $object;
	+ $diff = $this->updateDiffFromDict($diff, $xaction->getNewValue());
	+
	+ $adapter = $this->buildHeraldAdapter($diff, $xactions);
	+ $adapter->setContentSource($this->getContentSource());
	+ $adapter->setIsNewObject($this->getIsNewObject());
	+
	+ $engine = new HeraldEngine();
	+
	+ $rules = $engine->loadRulesForAdapter($adapter);
	+ $rules = mpull($rules, null, 'getID');
	+
	+ $effects = $engine->applyRules($rules, $adapter);
	+
	+ $blocking_effect = null;
	+ foreach ($effects as $effect) {
	+ if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) {
	+ $blocking_effect = $effect;
	+ break;
	+ }
	+ }
	+
	+ if ($blocking_effect) {
	+ $rule = idx($rules, $effect->getRuleID());
	+ if ($rule && strlen($rule->getName())) {
	+ $rule_name = $rule->getName();
	+ } else {
	+ $rule_name = pht('Unnamed Herald Rule');
	+ }
	+
	+ $message = $effect->getTarget();
	+ if (!strlen($message)) {
	+ $message = pht('(None.)');
	+ }
	+
	+ $errors[] = new PhabricatorApplicationTransactionValidationError(
	+ $type,
	+ pht('Rejected by Herald'),
	+ pht(
	+ "Creation of this diff was rejected by Herald rule %s.\n".
	+ " Rule: %s\n".
	+ "Reason: %s",
	+ 'H'.$effect->getRuleID(),
	+ $rule_name,
	+ $message));
	+ }
	+ break;
	+ }
	}

	- $diff->save();
	+ return $errors;
	+ }
	+
	+
	+ protected function shouldPublishFeedStory(
	+ PhabricatorLiskDAO $object,
	+ array $xactions) {
	+ return false;
	+ }
	+
	+ protected function shouldSendMail(
	+ PhabricatorLiskDAO $object,
	+ array $xactions) {
	+ return false;
	+ }
	+
	+ protected function supportsSearch() {
	+ return false;
	+ }

	- // NOTE: We only save the transcript if we didn't block the diff.
	- // Otherwise, we might save some of the diff's content in the transcript
	- // table, which would defeat the purpose of allowing rules to block
	- // storage of key material.
	+/* -( Herald Integration )------------------------------------------------- */

	- $engine->applyEffects($effects, $adapter, $rules);
	- $xscript = $engine->getTranscript();
	+ protected function shouldApplyHeraldRules(
	+ PhabricatorLiskDAO $object,
	+ array $xactions) {

	+ return true;
	}

	+ protected function buildHeraldAdapter(
	+ PhabricatorLiskDAO $object,
	+ array $xactions) {
	+
	+ $adapter = id(new HeraldDifferentialDiffAdapter())
	+ ->setDiff($object);
	+
	+ return $adapter;
	+ }
	+
	+ protected function didApplyHeraldRules(
	+ PhabricatorLiskDAO $object,
	+ HeraldAdapter $adapter,
	+ HeraldTranscript $transcript) {
	+
	+ $xactions = array();
	+ return $xactions;
	+ }
	+
	+ private function updateDiffFromDict(DifferentialDiff $diff, $dict) {
	+ $diff
	+ ->setSourcePath(idx($dict, 'sourcePath'))
	+ ->setSourceMachine(idx($dict, 'sourceMachine'))
	+ ->setBranch(idx($dict, 'branch'))
	+ ->setCreationMethod(idx($dict, 'creationMethod'))
	+ ->setAuthorPHID(idx($dict, 'authorPHID', $this->getActor()))
	+ ->setBookmark(idx($dict, 'bookmark'))
	+ ->setRepositoryPHID(idx($dict, 'repositoryPHID'))
	+ ->setRepositoryUUID(idx($dict, 'repositoryUUID'))
	+ ->setSourceControlSystem(idx($dict, 'sourceControlSystem'))
	+ ->setSourceControlPath(idx($dict, 'sourceControlPath'))
	+ ->setSourceControlBaseRevision(idx($dict, 'sourceControlBaseRevision'))
	+ ->setLintStatus(idx($dict, 'lintStatus'))
	+ ->setUnitStatus(idx($dict, 'unitStatus'))
	+ ->setArcanistProjectPHID(idx($dict, 'arcanistProjectPHID'));
	+
	+ return $diff;
	+ }
	}
	diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
	--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
	+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
	@@ -1523,8 +1523,6 @@
	$adapter->setExplicitReviewers($reviewer_phids);
	$adapter->setForbiddenCCs($unsubscribed_phids);

	- $adapter->setIsNewObject($this->getIsNewObject());
	-
	return $adapter;
	}

	diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php
	--- a/src/applications/differential/storage/DifferentialTransaction.php
	+++ b/src/applications/differential/storage/DifferentialTransaction.php
	@@ -18,6 +18,7 @@
	const TYPE_UPDATE = 'differential:update';
	const TYPE_ACTION = 'differential:action';
	const TYPE_STATUS = 'differential:status';
	+ const TYPE_DIFF = 'differential:diff';

	public function getApplicationName() {
	return 'differential';

File Metadata

Mime Type: text/plain
Expires: Sun, Mar 23, 3:54 AM (5 d, 6 h ago)
Storage Engine: blob
Storage Format: Encrypted (AES-256-CBC)
Storage Handle: 7710637
Default Alt Text: D10869.id26103.diff (20 KB)

D10869.id26103.diffNo OneTemporaryActions

D10869.id26103.diffView Options

File Metadata

Event Timeline

D10869.id26103.diff
No OneTemporary
Actions

D10869.id26103.diff
View Options