Page MenuHomePhabricator

Apply custom fields implemented using edges to apply validation logic
AcceptedPublic

Authored by joshuaspence on Apr 26 2019, 3:48 AM.

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 OK
Unit
Unit Tests OK
Build Status
Buildable 22729
Build 31160: Run Core Tests
Build 31159: arc lint + arc unit

Event Timeline

joshuaspence created this revision.Apr 26 2019, 3:48 AM

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!

Fix failing unit tests

joshuaspence requested review of this revision.Apr 26 2019, 4:19 AM
epriestley accepted this revision.May 14 2019, 2:14 PM

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