Page MenuHomePhabricator

Support custom fields in diffusion commit property views.

Authored by 20after4 on Jul 7 2015, 5:42 PM.


Group Reviewers
Blessed Reviewers

This adds support for custom fields in diffusion property views.
Use case: Used by Wikimedia to parse the gerrit change-id field from
commits and display a link to gerrit in the commit property view.

Test Plan

I tested as follows

  1. Installed (dropped the file into phabricator/extensions/)
  2. viewed a commit with a Change-Id: field in the commit message.
  3. Saw the Change-Id: in the property view, complete with a link to gerrit.

Note: P1819 is pretty messy but the code in this change should be
pretty solid. Some of it was lifted from the differential conduit api.

Diff Detail

Lint OK
Unit Tests OK
Build Status
Buildable 7184
Build 7413: [Placeholder Plan] Wait for 30 Seconds
Build 7412: arc lint + arc unit

Event Timeline

20after4 updated this revision to Diff 32823.Jul 7 2015, 5:42 PM
20after4 retitled this revision from to Support custom fields in diffusion commit property views..
20after4 updated this object.
20after4 edited the test plan for this revision. (Show Details)
20after4 added a reviewer: epriestley.
20after4 added a subscriber: qgil.
20after4 updated this revision to Diff 32824.Jul 7 2015, 5:53 PM
20after4 edited edge metadata.

remove my debugging call to phlog().

epriestley requested changes to this revision.Jul 8 2015, 8:36 AM
epriestley edited edge metadata.

Details inline, but I think the shortest path to getting what you want here is:

  • Do vanilla field rendering in Diffusion (appendFieldsToPropertyList(...)).
  • Have your field reach into the commit object when reading from storage (or just when rendering) and just parse out Change-Id. e.g., your renderValueForPropertyDetailView() or whatever method can just do $commit->getData()->getCommitMessage(), parse that, then throw most if away and just use your field.
  • Some day, we'll have a real parse phase in MessageParserWorker and you can parse + store there, which will enable search/sort, etc (these aren't too useful for this field, anyway).

Can we typehint $commit and $data? They're PhabricatorRepositoryCommit and PhabricatorRepositoryCommitData, I think?


This needs to use the actual viewer (to respect policies), not the omnipotent viewer. You can get it with $this->getViewer().


I'd expect that the rest of this method is basically just:


(Although the caller would need to be rearranged a bit.)

That won't work in your use case (where you want to read directly from the commit message), but I think your use case should be supported by a new "read commit message into fields" phase during commit parsing.

For example, it would be desirable for Diffusion to let you search for commits with a given "Change-Id", but this won't work (and can never work) if we re-parse the message at display time, since we won't have any database value that we can run a WHERE customfield_123_value = 'some-change-id' query against. Instead, we'd need to parse the message at import time and write the fields to database storage (so they become searchable), then read them from storage here.

I think adding this phase is conceptually fine, but likely quite complicated to develop/test.

An alternative would be to make your custom field fake its read-from-storage and have it go fish around in the commit message instead. This isn't "right" -- and won't work with the "search" use case -- but should work fine until we have an import phase.

This revision now requires changes to proceed.Jul 8 2015, 8:36 AM
20after4 abandoned this revision.May 22 2017, 3:46 AM

not upstream-able.