Page MenuHomePhabricator

Upgrading: Changes to Differential Custom Fields in Commit Messages
Closed, ResolvedPublic

Description

In connection with changes in T11114, Differential custom fields which appear on commit messages have changed. This change affects you if:

  • you run custom third-party code which defines new Differential Custom Fields; and
  • those fields appear in commit messages (for example, with arc diff).

This change is part of a broader set of changes related to T11114 and T12010.


Broadly, the commit message handling parts of custom fields have been separated into a new class (DifferentialCommitMessageField). If you have a subclass of DifferentialCustomField which implements commit message methods, make these changes:

  • Create a new, corresponding subclass of DifferentialCommitMessageField for your custom field (for example, if you have DifferentialZebraCustomField, create a new DifferentialZebraCommitMessageField).
  • If you use Phabricator's custom field storage, extend DifferentialCommitMessageCustomField instead of extending DifferentialCommitMessageField directly.
  • Move these methods from the CustomField to the CommitMessageField:
Old MethodActionNew MethodNotes
getFieldName()Copy TogetFieldName()Both classes should define this method.
getFieldKeyForConduit()Move ToFIELDKEY class constantPreserves API compatibility.
shouldAppearInCommitMessage()Remove-Obsolete.
shouldAllowEditInCommitMessage()Move ToisFieldEditable()Omit if it always returns true.
getCommitMessageLabels()Move TogetFieldAliases()Move and rename method.
parseValueFromCommitMessage()Move ToparseFieldValue()Move and rename method.
getRequiredHandlePHIDsForCommitMessage()Remove-Obsolete.
renderCommitMessageValue()Move TorenderFieldValue()Move and rename method. Signature has changed.
-Create?readFieldValueFromObject()Optional, but probably desired.
-Create?getFieldOrder()Optional.
-Create?isFieldEnabled()Optional.
validateCommitMessageValue()MovevalidateFieldValue()Move and rename.
shouldAppearInCommitMessageTemplate()MoveisTemplateField()Move and rename.

In the upstream, these are the underlying changes, which included making these changes to all upstream CustomFields:

Event Timeline

epriestley updated the task description. (Show Details)Jan 10 2017, 7:09 PM
chad awarded a token.Jan 10 2017, 7:12 PM
chad added a subscriber: chad.
jmeador added a subscriber: jmeador.EditedJan 12 2017, 12:32 AM

shouldAppearInCommitMessage() is marked as obsolete, but still seems necessary for me to implement a CustomField that behaves as follows:

The field should not be editable in the commit message
The field should show up in the final commits merged to master

It also seems that shouldAllowEditInCommitMessage() is also not cleanly replaced by isFieldEditable().

@jmeador, I think I have a fix for your use case but it touches enough stuff that I'm going to hold it until after release promotion. Your use case is a bit unusual but should be possible to support.

Awesome. What do you think the time delta will be between release promotion and the fix you have being merged to master?

Hour or two if nothing's on fire -- probably today, just later this evening.

chad added a comment.Jan 13 2017, 5:05 PM

I plan to land several fires

chad added a comment.Jan 13 2017, 5:05 PM

ok, probably tomorrow

Alright, after D17207 I think these are analogous to what you're doing?

commit 30af8e9c120061b7d7883fa401febae0e3b220b7
Author: epriestley <git@epriestley.com>
Date:   Fri Jan 13 15:06:25 2017 -0800

    WIP

diff --git a/src/applications/differential/customfield/DifferentialMagicFlagField.php b/src/applications/differential/customfield/DifferentialMagicFlagField.php
new file mode 100644
index 000000000..618b18cc5
--- /dev/null
+++ b/src/applications/differential/customfield/DifferentialMagicFlagField.php
@@ -0,0 +1,100 @@
+<?php
+
+final class DifferentialMagicFlagField
+  extends DifferentialStoredCustomField {
+
+  public function getFieldKey() {
+    return 'magic.flag';
+  }
+
+  public function getFieldName() {
+    return pht('Magic Flag');
+  }
+
+  public function getFieldDescription() {
+    return pht('A magic flag.');
+  }
+
+  public function shouldAppearInPropertyView() {
+    return true;
+  }
+
+  public function renderPropertyViewLabel() {
+    return $this->getFieldName();
+  }
+
+  public function renderPropertyViewValue(array $handles) {
+    if (!strlen($this->getValue())) {
+      return null;
+    }
+
+    return $this->getValue();
+  }
+
+  public function shouldAppearInApplicationTransactions() {
+    return true;
+  }
+
+  public function getOldValueForApplicationTransactions() {
+    return $this->getValue();
+  }
+
+  public function getNewValueForApplicationTransactions() {
+    return $this->getValue();
+  }
+
+  public function shouldAppearInEditView() {
+    return true;
+  }
+
+  public function renderEditControl(array $handles) {
+    // This is not editable from the web UI.
+    return null;
+  }
+
+  public function readValueFromRequest(AphrontRequest $request) {
+    return null;
+  }
+
+  public function getApplicationTransactionTitle(
+    PhabricatorApplicationTransaction $xaction) {
+    $author_phid = $xaction->getAuthorPHID();
+    $old = $xaction->getOldValue();
+    $new = $xaction->getNewValue();
+
+    return pht(
+      '%s edited the magic flag: "%s" became "%s".',
+      $xaction->renderHandleLink($author_phid),
+      $old,
+      $new);
+  }
+
+  public function getApplicationTransactionTitleForFeed(
+    PhabricatorApplicationTransaction $xaction) {
+
+    $object_phid = $xaction->getObjectPHID();
+    $author_phid = $xaction->getAuthorPHID();
+    $old = $xaction->getOldValue();
+    $new = $xaction->getNewValue();
+
+    return pht(
+      '%s edited the magic flag for %s: "%s" became "%s".',
+      $xaction->renderHandleLink($author_phid),
+      $xaction->renderHandleLink($object_phid),
+      $old,
+      $new);
+  }
+
+  public function shouldAppearInConduitDictionary() {
+    return true;
+  }
+
+  public function shouldAppearInConduitTransactions() {
+    return true;
+  }
+
+  protected function newConduitEditParameterType() {
+    return new ConduitStringParameterType();
+  }
+
+}
diff --git a/src/applications/differential/field/MagicFlagCommitMessageField.php b/src/applications/differential/field/MagicFlagCommitMessageField.php
new file mode 100644
index 000000000..93de09449
--- /dev/null
+++ b/src/applications/differential/field/MagicFlagCommitMessageField.php
@@ -0,0 +1,36 @@
+<?php
+
+final class MagicFlagCommitMessageField
+  extends DifferentialCommitMessageCustomField {
+
+  const FIELDKEY = 'magic.flag';
+
+  public function getFieldName() {
+    return pht('MagicFlag');
+  }
+
+  public function getCustomFieldKey() {
+    return 'magic.flag';
+  }
+
+  public function getFieldOrder() {
+    return 1000000;
+  }
+
+  public function isFieldEditable() {
+    return false;
+  }
+
+  public function isTemplateField() {
+    return false;
+  }
+
+  public function renderFieldValue($value) {
+    if (strlen($value)) {
+      return "\nMagicFlag={$value}";
+    } else {
+      return null;
+    }
+  }
+
+}

And those now work for me, I believe. In particular, we'll load all field values which use storage so the values shouldn't vanish mysteriously anymore.

In particular, your code may just work as-is after updating. If not, compare them with those "MagicFlag" fields and/or let me know what you're still seeing?

so if isFieldEditable and isTemplateField are both false, will the field still show up in getcommitmessage?

Yes, if you aren't in edit mode. Here's normal mode (includes field):

Here's edit mode (no field):

Specifically, they control:

  • isFieldEditable(): Does this appear in edit mode?
  • isTemplateField(): Does this appear in create mode, even if it's empty?

Fields like "Reviewers:" are included in the empty template so you can fill them in, while lesser-used fields like "Tasks" and "Projects" are not.

Awesome. Things appear to be working now. I suppose I can strip out all the overrides that the most recent commit stripped out. Sweetness.

donaldguy added a comment.EditedJan 16 2017, 11:11 PM

For the record, D17207 is needed for any custom field implemented in accordance to this ticket (without shouldAppearInCommitMessage) not just ones in the not-editable special case.

I spent 2.5 hours debugging why differential.getcommitmessage was ignoring my field, for it all to come down to the difference between DifferentialCustomField::ROLE_COMMITMESSAGE and PhabricatorCustomField::ROLE_STORAGE (or PhabricatorCustomField::ROLE_CONDUIT in the nearly identical-to-loadCustomFieldStorage workaround I wrote in my field's readFieldValueFromObject)

I don't think things are especially usable in stable atm. People should consider a git cherry-pick 62cf4e6b95abbebc8ae2b1591039967651188c6e if they are adding their first custom field this week (or have completed the migration from the description of this task)

If they need to bridge the difference as some fields are migrated and some aren't consider something like

public function readFieldValueFromObject(DifferentialRevision $revision) {
    $cf = PhabricatorCustomField::getObjectFields(
               $revision,
               PhabricatorCustomField::ROLE_STORAGE);
    $cf->readFieldsFromObject($revision);

    id(new PhabricatorCustomFieldStorageQuery())
      ->addField(idx($cf->getFields(), $this::FIELDKEY))
      ->execute();

    return idx($cf->getFields(), $this::FIELDKEY)
      ->getConduitDictionaryValue();
  }

being aware that this is viewer-un-aware

epriestley closed this task as Resolved.Feb 18 2017, 2:30 AM
epriestley claimed this task.

I think we mostly managed to get installs through this.

alexmv added a subscriber: alexmv.Mar 6 2017, 7:15 AM

The table in the description references a getConduitFieldKey method – which I can't find reference to ever existing in the source. Was that supposed to be getFieldKeyForConduit?

I think that's right, yes.

alexmv updated the task description. (Show Details)Mar 6 2017, 7:23 PM