Page MenuHomePhabricator

Support enabling and disabling (and making other adjustments to) custom fields based on object subtypes
Open, NormalPublic

Description

See also PHI1051. Previously, see D17593, and some discussion in T12314.

Various reasonable use cases exist for having some fields present only on objects of certain subtypes. For example, a "Carapace Color" field might only be relevant on "Bug" subtasks, not "Fish" subtasks.

I want to implement this as "fields": { ... } override on the subtype (as suggested in D17593), not a "subtypes": [ ... ] configuration on the field (as originally proposed in D17593), because I think this gives us greater flexibility to customize more behavior based on the subtype (for example, to change the label, caption, etc).

At least until bigger problems arise, I think we can cheat our way through this in getCustomFieldSpecificationForRole(). This doesn't feel great, but is a relatively small patch, and more-or-less the right place to put things to pursue the broader goals of letting subtypes override a significant amount of field behavior.

The only immediately obvious challenge is that some roles are generic and evaluated against a default object. In these cases, any overrides on the default subtype should not be used. For example, if you view /maniphest/ and get the task search view, you should be able to use all available fields to search -- not just the fields present on the default subtype. In getCustomFieldSpecificationForRole(), we can't examine the object itself ($this) to make a determination about whether the subtype matters or not, but should be able to examine the $role.


Future work:

  • When you search for objects with value "X" in custom field "Y", we'll naively return objects of subtypes which hide the Y field. They may have a value in Y if they were previously an object of a different subtype and changed subtypes, or possibly because of default field value behavior. We could fix this by testing each subtype for presence of each custom field constraint, and including an AND subtype NOT IN (...) clause in the final query.
  • There's currently no way to disable a field by default. It would probably be useful to let a field specify that is is disabled by default, then enable it only on specific subtypes. Currently, you would need to disable it on all subtypes where you don't want it.
  • When you export tasks (e.g., to a spreadsheet), the subtype filtering currently does not apply. It probably can and should: we'll still export the column named "Carapace Color", but leave it empty for tasks of a subtype which does not use the field.

Event Timeline

epriestley triaged this task as Normal priority.
epriestley renamed this task from Support enabling and disabling custom fields based on object subtypes to Support enabling and disabling (and making other adjustments to) custom fields based on object subtypes.Feb 8 2019, 3:59 PM
epriestley claimed this task.

When we reach getCustomFieldSpecificationForRole($role), via EditEngine, the $this object currently does not have a subtype set, so it can't make any decisions about which fields to expose. This appears to have a fairly surgical fix in EditEngine, but if there are like 10 more of these coming it might merit another look.

CustomField generally has two parts: one part configures builtin primitive custom fields (maniphest.custom-field-definitions) and one part configures all custom fields (like maniphest.fields, including both configured primitive fields, and more sophisticated subclassed fields). The second configuration layer allows you to impose a global ordering on fields. I suspect the subclassed fields are broadly an architectural mistake (instead: subclasses should provide a new field type, then all fields should be configured primitives, some just less primitive than others) but fixing that is leagues out of scope. See also T6030. I'm intending to add the new behavior as either part of maniphest.fields or as a complement to it. Making it complimentary is looking more and more attractive.

PhabricatorCustomField->getObjectFields() currently caches field definitions on the object, based on the $role. This cache is slightly weird, and would be invalid if an object changes subtype during a request. They currently don't, or at least don't in a meaningful way.

There isn't an especially good layer here for injecting against the custom-field-definitions only. Also, in theory, you should be able to configure anything, whether the field is defined in configuration or in a subclass -- if you want to rename Title to Species on "Bug" subtypes, great. So I'm going to let you override anything.

This will create some amount of mess, since it will let you attempt to override behavior for some objects which don't support behavioral overrides. For example, actually renaming Title to something else might cause things to fatal. I'll mitigate this as well as I can, but the guidance is probably "don't do that" until T6030.

This would be incredibly useful for the things we are trying to do at WMF. I've gone so far as to duplicate lots of forms for each subtype and remove fields as needed. This results in N * N forms and it's not at all manageable beyond a very small number of sub-types.

epriestley updated the task description. (Show Details)Feb 13 2019, 5:20 PM
hskiba added a subscriber: hskiba.Feb 25 2019, 2:13 AM

Making a way to set fields to default: disabled would make this feature even better ;)

(This is likely to be very long, very rambling, and not particularly enlightening or useful.)

To make some sort of effort to chart a course forward here:

Configuration

Custom fields are currently configured by an option which defines "configured custom fields", like maniphest.custom-field-definitions.

Layered on top of that is an option like maniphest.fields, which allows you to reorder and disable fields. However, this applies to both builtin fields (like "Title") and custom fields (like "Carapace Thickness").

The ability to reorder fields with *.fields config is probably (?) more or less obsolete at this point. I don't think there was ever an especially strong reason for it, and you can reorder fields on custom EditEngine forms anyway. So I think it only really impacts the ability to do stuff like putting "Carapace Thickness" above "Title" in Search? I'm not even sure Search respects this ordering. I think a big chunk of the motivation for this was originally based on compatibility with some weird legacy Facebook stuff.

A possible approach to simplifying this is to remove *.fields. We lose:

  • Reordering across all fields.
  • Ability to disable builtin fields.

We can actually get both of these back by doing this:

  • *.custom-field-definitions may define fields as disabled by default.
  • *.custom-field-definitions may now "define" builtin fields. Not all keys will work -- you can't change "Title" into a "date" field. But you could change the name to "Species", or disable it.

Then we can get rid of all the *.fields configuration options.

Are All Fields Custom Fields? The answer is probably: no?

Long ago, mostly in Differential, there was an effort to define all builtin fields as custom fields. This didn't really work well.

The idea was that every field could be a "custom field", and then you could use the same set of configuration options and UIs everywhere to define behavior. In Differential, there's a mild amount of additional motivation for this because some of the fields (like "Test Plan") are truly optional, and it made sense to imagine that we'd have more of that kind of field, and disabling or reordering things would be useful. But this isn't really true of other applications, and subsequent work on EditEngine has mostly done a better job of dealing with this anyway.

This also infected Drydock and Harbormaster to some degree.

I'd like to move away from this as much as possible, and we've been headed away from it (albeit slowly) for some time.

The Implementation is a Huge Mess

The custom field hierarchy currently does two different things:

  • Defines which custom fields exist, e.g. a revision has a "Summary" because the class DifferentialSummaryField exists.
  • Defines how those fields work, e.g. you store the summary on a revision by calling ->setSummary(...) and DifferentialSummaryField knows how to do that.

That seems fine at first glance. The problem is that we only want to use Differential fields in Differential, and the class tree alone determines which fields are Differential fields, so all Differential fields must extend a common DifferentialCustomField parent. So far: okay.

But we have lots of generic fields like TextField and SelectField and whatever. These obviously can't extend DifferentialCustomField. Now, we're in trouble.

This is solved by having every custom field be able to proxy other custom fields, so a DifferentialConfiguredCustomField wraps around a TextField and you get an object which behaves mostly like a TextField but correctly extends from DifferentialCustomField. The problem is solved, but at huge cost: all the proxy logic in PhabricatorCustomField is a big complicated mess. The whole thing is hard to understand and hard to implement and hard to get right.

The way this should actually work is that the determination of which fields an objects has is made by some separate DifferentialCustomFieldEngine sort of class, not by the class tree. This Engine can add whatever Text or Select or DifferentialSummary fields it wants to the list of supported fields, without any concern about what they extend from. Then all the proxy garbage can be removed. This is how basically all other modern things work, particularly EditEngine.

This change alone would make custom fields far more palatable, and do a lot to dig us out of the hole of technical debt and well-intended-but-ultimately-very-bad technical decisions they currently live in.

Harbormaster and Drydock

Harbormaster build steps and Drydock blueprints both need some kind of reasonable access to custom fields. The current implementations leave much to be desired.

If these implementations can move away from the existing custom field infrastructure, that probably simplifies everything else substantially, since all the other use cases are "normal", or "Differential", which isn't "normal" but also isn't too weird.

I wholeheartedly agree that all that proxy field mess needs to go away. With that gone the only moderately confusing part remaining would be the way in which custom fields interact with herald, email and notifications.

pasik added a subscriber: pasik.Mar 24 2019, 9:24 PM