Page MenuHomePhabricator

Apply custom fields implemented using edges to apply validation logic
AcceptedPublic

Authored by joshuaspence on Apr 26 2019, 3:48 AM.
Tags
None
Referenced Files
F14151035: D20481.diff
Wed, Dec 4, 6:58 PM
F14147860: D20481.id48856.diff
Wed, Dec 4, 8:20 AM
Unknown Object (File)
Tue, Dec 3, 4:00 AM
Unknown Object (File)
Mon, Nov 25, 6:00 AM
Unknown Object (File)
Thu, Nov 21, 5:48 AM
Unknown Object (File)
Sun, Nov 17, 1:17 AM
Unknown Object (File)
Oct 17 2024, 9:27 PM
Unknown Object (File)
Oct 17 2024, 9:27 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

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.

Test Plan

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

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

Unit TestsFailed

TimeTest
55 msConpherenceRoomTestCase::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 msConpherenceRoomTestCase::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 msConpherenceRoomTestCase::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 msConpherenceRoomTestCase::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 msPhabricatorPhortuneTestCase::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

I'm not sure if the upstream would be interested in this change, but I figured there's no harm in trying.

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 26 2019, 3:51 AM
Harbormaster failed remote builds in B22728: Diff 48855!

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.

This revision is now accepted and ready to land.May 14 2019, 2:14 PM