Page MenuHomePhabricator

Support custom fields in diffusion commit property views.
AbandonedPublic

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

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

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 https://secure.phabricator.com/P1819 (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

Branch
redesign-2015-wmf
Lint
Lint OK
Unit
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).
src/applications/diffusion/controller/DiffusionCommitController.php
1087

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

1092

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

1095

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

$field_list->appendFieldsToPropertyList(...);

(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.