Currently, subclasses of PhabricatorCustomField that return PhabricatorTransactions::TYPE_EDGE from getApplicationTransactionType can't apply custom validation logic using validateApplicationTransactions. This diff adds support for doing so.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers
Plonked the following classes in src/extensions and verified that attempting to modify a project resulted in bogus validation errors.
src/extensions/PhabricatorExampleEdgeType.php
<?php final class PhabricatorExampleEdgeType extends PhabricatorEdgeType { const EDGECONST = 9999; }
src/extensions/PhabricatorProjectExampleCustomField.php
<?php final class PhabricatorProjectExampleCustomField extends PhabricatorProjectCustomField { private $value = []; protected function getDatasource(): PhabricatorTypeaheadDatasource { return new PhabricatorProjectDatasource(); } protected function getApplicationTransactionEdgeType(): PhabricatorEdgeType { return new PhabricatorExampleEdgeType(); } public function getValue(): array { return $this->value; } public function setValue(array $value): void { $this->value = $value; } /* -( PhabricatorCustomField )--------------------------------------------- */ public function getFieldKey(): string { return 'example'; } public function getFieldName(): string { return pht('Example'); } public function readValueFromObject(PhabricatorCustomFieldInterface $object): void { $edges = PhabricatorEdgeQuery::loadDestinationPHIDs( $this->getObject()->getPHID(), $this->getApplicationTransactionEdgeConstant()); $this->setValue($edges); } public function getValueForStorage(): array { return ['=' => array_fuse($this->getValue())]; } public function setValueFromStorage($value): void { $this->setValue(array_values($value['='])); } public function shouldAppearInApplicationTransactions(): bool { return true; } public function getApplicationTransactionType(): string { return PhabricatorTransactions::TYPE_EDGE; } public function getApplicationTransactionMetadata(): array { return [ 'edge:type' => $this->getApplicationTransactionEdgeConstant(), ]; } public function validateApplicationTransactions( PhabricatorApplicationTransactionEditor $editor, $type, array $xactions): array { $errors = parent::validateApplicationTransactions( $editor, $type, $xactions); foreach ($xactions as $xaction) { $errors[] = new PhabricatorApplicationTransactionValidationError( $type, pht('Invalid value'), pht('This value is not allowed.'), $xaction); } return $errors; } public function shouldAppearInEditView(): bool { return true; } public function readValueFromRequest(AphrontRequest $request): void { $this->setValue($request->getArr($this->getFieldKey())); } public function getRequiredHandlePHIDsForEdit(): array { return $this->getValue(); } public function renderEditControl(array $handles): AphrontFormControl { return (new AphrontFormTokenizerControl()) ->setViewer($this->getViewer()) ->setLabel($this->getFieldName()) ->setName($this->getFieldKey()) ->setValue($this->getValue()) ->setDatasource($this->getDatasource()); } public function shouldAppearInPropertyView(): bool { return true; } public function renderPropertyViewValue(array $handles): ?PhutilSafeHTML { return $this->renderHandleList($handles); } public function getRequiredHandlePHIDsForPropertyView(): array { return $this->getValue(); } }
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 22728 Build 31158: Run Core Tests Build 31157: arc lint + arc unit
Time | Test | |
---|---|---|
55 ms | ConpherenceRoomTestCase::testNUserRoomCreate EXCEPTION (RuntimeException): Argument 1 passed to PhabricatorCustomField::getObjectFields() must implement interface PhabricatorCustomFieldInterface, instance of ConpherenceThread given, called in /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php on line 2564 and defined
#0 /core/data/drydock/workingcopy-91/repo/phabricator/src/infrastructure/customfield/field/PhabricatorCustomField.php(48): PhutilErrorHandler::handleError(4096, 'Argument 1 pass...', '/core/data/dryd...', 48, Array)
#1 /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php(2564): PhabricatorCustomField::getObjectFields(Object(ConpherenceThread), 'edit')
| |
28 ms | ConpherenceRoomTestCase::testOneUserRoomCreate EXCEPTION (RuntimeException): Argument 1 passed to PhabricatorCustomField::getObjectFields() must implement interface PhabricatorCustomFieldInterface, instance of ConpherenceThread given, called in /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php on line 2564 and defined
#0 /core/data/drydock/workingcopy-91/repo/phabricator/src/infrastructure/customfield/field/PhabricatorCustomField.php(48): PhutilErrorHandler::handleError(4096, 'Argument 1 pass...', '/core/data/dryd...', 48, Array)
#1 /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php(2564): PhabricatorCustomField::getObjectFields(Object(ConpherenceThread), 'edit')
| |
51 ms | ConpherenceRoomTestCase::testRoomParticipantAddition EXCEPTION (RuntimeException): Argument 1 passed to PhabricatorCustomField::getObjectFields() must implement interface PhabricatorCustomFieldInterface, instance of ConpherenceThread given, called in /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php on line 2564 and defined
#0 /core/data/drydock/workingcopy-91/repo/phabricator/src/infrastructure/customfield/field/PhabricatorCustomField.php(48): PhutilErrorHandler::handleError(4096, 'Argument 1 pass...', '/core/data/dryd...', 48, Array)
#1 /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php(2564): PhabricatorCustomField::getObjectFields(Object(ConpherenceThread), 'edit')
| |
53 ms | ConpherenceRoomTestCase::testRoomParticipantDeletion EXCEPTION (RuntimeException): Argument 1 passed to PhabricatorCustomField::getObjectFields() must implement interface PhabricatorCustomFieldInterface, instance of ConpherenceThread given, called in /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php on line 2564 and defined
#0 /core/data/drydock/workingcopy-91/repo/phabricator/src/infrastructure/customfield/field/PhabricatorCustomField.php(48): PhutilErrorHandler::handleError(4096, 'Argument 1 pass...', '/core/data/dryd...', 48, Array)
#1 /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php(2564): PhabricatorCustomField::getObjectFields(Object(ConpherenceThread), 'edit')
| |
24 ms | PhabricatorPhortuneTestCase::testNewPhortuneAccount EXCEPTION (RuntimeException): Argument 1 passed to PhabricatorCustomField::getObjectFields() must implement interface PhabricatorCustomFieldInterface, instance of PhortuneAccount given, called in /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php on line 2564 and defined
#0 /core/data/drydock/workingcopy-91/repo/phabricator/src/infrastructure/customfield/field/PhabricatorCustomField.php(48): PhutilErrorHandler::handleError(4096, 'Argument 1 pass...', '/core/data/dryd...', 48, Array)
#1 /core/data/drydock/workingcopy-91/repo/phabricator/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php(2564): PhabricatorCustomField::getObjectFields(Object(PhortuneAccount), 'edit')
| |
View Full Test Results (5 Failed · 360 Passed) |
Event Timeline
Comment Actions
I'm not sure if the upstream would be interested in this change, but I figured there's no harm in trying.
Comment Actions
I like this goal achieved by this change -- it's silly that edge validation isn't really extensible anywhere -- but I think this also digs us deeper into the mess of T13248.
But T13248 is likely to be somewhat disruptive to custom fields anyway, so the harm here is probably quite small, and I'm not sure when it will move forward.