Page MenuHomePhabricator

Support custom fields in diffusion commit property views.
AbandonedPublic

Authored by 20after4 on Jul 7 2015, 5:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 1:48 PM
Unknown Object (File)
Dec 30 2023, 12:51 AM
Unknown Object (File)
Dec 20 2023, 11:22 AM
Unknown Object (File)
Dec 12 2023, 2:16 AM
Unknown Object (File)
Nov 13 2023, 6:58 AM
Unknown Object (File)
Nov 4 2023, 7:04 AM
Unknown Object (File)
Oct 30 2023, 8:03 AM
Unknown Object (File)
Sep 24 2023, 9:48 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 Passed
Unit
Tests Passed
Build Status
Buildable 7184
Build 7413: [Placeholder Plan] Wait for 30 Seconds
Build 7412: arc lint + arc unit

Event Timeline

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 edited edge metadata.

remove my debugging call to phlog().

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