Page MenuHomePhabricator

Support "Set X to" as an action in Herald for tokenizer/datasource custom fields
ClosedPublic

Authored by epriestley on Nov 27 2017, 11:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jan 21, 9:45 AM
Unknown Object (File)
Thu, Jan 16, 12:43 AM
Unknown Object (File)
Thu, Jan 9, 10:28 AM
Unknown Object (File)
Dec 17 2024, 6:50 AM
Unknown Object (File)
Dec 13 2024, 11:25 PM
Unknown Object (File)
Dec 10 2024, 2:53 AM
Unknown Object (File)
Dec 4 2024, 5:44 AM
Unknown Object (File)
Dec 3 2024, 8:42 PM
Subscribers

Details

Summary

See PHI173. Adds custom field support for Herald actions, and implements actions for "Datasource/Tokenizer" fields.

The only action available for now is "set field to...". Other actions ("Add values", "Remove values") might make sense in the future for these fields, but there's currently no use case. For most other field types (text, select, checkbox, etc) only "Set to" makes sense.

Test Plan
  • Added a "datasource" custom field to the custom field definition in Config.
  • Added a "if field is empty, set field to default value X" rule to Herald.
  • Created a task with a nonempty field: no Herald trigger.
  • Created a task with an empty field: Herald fired.
  • Reviewed rule and transcripts for text strings.

Screen Shot 2017-11-27 at 3.46.35 PM.png (211×388 px, 25 KB)

Screen Shot 2017-11-27 at 3.46.15 PM.png (461×1 px, 98 KB)

Screen Shot 2017-11-27 at 3.46.05 PM.png (1×1 px, 149 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/infrastructure/customfield/field/PhabricatorCustomField.php
1482–1487

What do all these if ($proxy) checks accomplish, and why do we only do a real check if the object has a proxy?

This revision is now accepted and ready to land.Nov 28 2017, 9:19 PM

The CustomField stuff is built real weird.

As written, the actual classes need to extend application classes (say, DifferentialCustomField) to work. But we don't want to implement DifferentialTextCustomField, ManiphestTextCustomField, DiffusionTextCustomField, etc., for every application to have text fields everywhere, so the "solution" is that fields can "proxy" another field. So a "text" field in Differential is really a DifferentialCustomField proxying a PhabricatorStandardTextCustomField or whatever. This gives it both "Differential" behavior and "Text" behavior.

This is pretty gross and weird, and we don't do it anywhere else. CustomField generally got a lot of things mostly-reasonable but has a few things which just didn't turn out that well. Later abstractions like EditEngine + EditField feel far better factored. I've moved us a bit away from as heavy an investment in CustomField to some degree in changes like T11114, and imagine it will eventually be wrangled into something a little more modern/sensible (notably, without all the proxying stuff).

For now, the proxy stuff is all just saying "if this is proxying something, the default behavior is whatever the thing we're proxying is doing; otherwise it's return null". This is a little like a really hacky, cumbersome sort of multiple inheritance? Hopefully we'll be able to get rid of it eventually, maybe when something like T418 motivates more extensive work on CustomField.

This revision was automatically updated to reflect the committed changes.