Add a "preconfigured field options" concept in Field UI

This is blocked on #2373491: Categorize field type plugins.

Yay!

Here's an initial review.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -203,6 +203,16 @@ class EntityType implements EntityTypeInterface {
      /**
   +   * Indicates whether this entity type is often used as a reference target.
   +   *
   +   * This is used by the Entity reference field to promote an entity type in the
   +   * add new field select list in Field UI.
   +   *
   +   * @var bool
   +   */
   +  protected $frequent_reference = FALSE;
   

   This is of course going to depend on the site, and on the perspective of the site builder. But sane defaults are surely possible, and acceptable.

2. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -674,4 +674,12 @@ public function getListCacheTags();
   +   * Indicates whether this entity type is commonly used as a reference target.
   

   • Here: "commonly".
   • Above: "often".
   • Function name: "frequent".

   Let's make those consistent :)

3. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
   @@ -131,9 +131,31 @@ public function getDefaultFieldSettings($type) {
   +        // Remove the original definition from the list.
   +        unset($definitions[$id]);
   

   s/original/original (unconfigured)/

4. +++ b/core/lib/Drupal/Core/Field/PreconfiguredFieldUiOptionsInterface.php
   @@ -0,0 +1,52 @@
   + * These field definitions will be exposed as additional options in the
   + * 'Add field' form in Field UI, together with individual field types.
   

   80 cols

5. +++ b/core/lib/Drupal/Core/Field/PreconfiguredFieldUiOptionsInterface.php
   @@ -0,0 +1,52 @@
   + * Note that field types implementing this interface will no longer have the
   + * usual $field_type['id'] => $field_type['label'] option exposed in Field UI's
   + * 'Add field' form, so they need to take care of including an 'unconfigured'
   + * definition as part of the return value of getPreconfiguredDefinitions().
   

   Hrm, why does it have to be this way? Why can't the usual still be present? Because we currently explicitly remove the original definition. What's wrong with just not doing that anymore?

   Some ordering problem?

6. +++ b/core/lib/Drupal/Core/Field/PreconfiguredFieldUiOptionsInterface.php
   @@ -0,0 +1,52 @@
   +   *   A multi-dimensional array with non-numeric keys and the following
   

   s/non-numeric/string/, if I have to believe what's written just below.

   Or do you mean "non-numeric keys at all levels"?

7. +++ b/core/modules/entity_reference/entity_reference.module
   @@ -101,6 +101,16 @@ function entity_reference_field_storage_config_update(FieldStorageConfigInterfac
   + * Implements hook_form_FORM_ID_alter() for field_ui_field_storage_add_form.
   

   Is this "for …" suffix common? Never seen it before.

8. +++ b/core/modules/entity_reference/entity_reference.module
   @@ -101,6 +101,16 @@ function entity_reference_field_storage_config_update(FieldStorageConfigInterfac
   +  // Move the 'Other' reference option at the end of the list.
   

   s/at/to/

9. +++ b/core/modules/entity_reference/entity_reference.module
   @@ -101,6 +101,16 @@ function entity_reference_field_storage_config_update(FieldStorageConfigInterfac
   +  $form['add']['new_storage_type']['#options'][t('Reference')]['field_ui:entity_reference:_other'] = $other;
   

   Wow, t()inside an array key. Never seen that before :D

   Probably due to optgroups?

10. +++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
    @@ -216,4 +218,37 @@ public static function fieldSettingsFormValidate(array $form, FormStateInterface
    +    // Also include an 'unconfigured' option.
    +    $options['_other'] = [
    

    Unconfigured or other?

    I understood from the documentation I quoted above that the key had to be 'unconfigured'. But apparently that's not the case.

    That's a bit confusing. Perhaps use double quotes to distinguish between strings and "airquotes"?

11. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -396,20 +431,34 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +   * @param bool $widget_id
    +   *   (optional) The plugin ID of the widget. Defaults to NULL.
    ...
    +   * @param bool $formatter_id
    +   *   (optional) The plugin ID of the formatter. Defaults to NULL.
    

    bool|null

The patch in general looks really great. Here is just one nitpick, but I just want to say that this is great functionality!

+++ b/core/lib/Drupal/Core/Field/PreconfiguredFieldUiOptionsInterface.php
@@ -0,0 +1,46 @@
+ * Defines an interface for exposing "preconfigured" Field UI definitions.

What is a field UI definition, I guess its a field definition?

Manually tested the patch and it looked really great!

The patch looks great, but I don't think I have enough experience in these areas to be able to RTBC it. Have not found anything to be concerned about.

Also tested manually, works great. IMO RTBC. Pinged Bojhan for a usability review.

Didn't mean to do that.

What other labels than "Other.." (hah), did we explore? That's not really a very descriptive label - not sure if I have an alternative atm.

I also tested this manually and it behaves great IMHO.

If none of us can think of a better label, then let's just do this. Changing this label would be utterly trivial. Progress over perfection.

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -203,6 +203,16 @@ class EntityType implements EntityTypeInterface {
+  protected $common_reference = FALSE;

How about IsReferenceable? I find this name confusing.

 *
 * @ViewsDisplay(
 *   id = "entity_reference",
 *   title = @Translation("Entity Reference"),
 *   admin = @Translation("Entity Reference Source"),
 *   help = @Translation("Selects referenceable entities for an entity reference field."),
 *   theme = "views_view",
 *   register_theme = FALSE,
 *   uses_menu_links = FALSE,
 *   entity_reference_display = TRUE

'referenceable' is already exposed to user in entity_reference display plugin so why not here. But anyway it is a boolean so IMO the @var name should start with 'Is'.
Can we please wait for @yched's input here?

#25 + #26:

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -203,6 +203,16 @@ class EntityType implements EntityTypeInterface {
+  protected $common_reference = FALSE;

@@ -709,4 +719,11 @@ public function getConfigDependencyKey() {
+  public function isCommonReference() {

is_common_reference_target/isCommonReferenceTarget() would probably be clearer.

NR per #27.

Trying to formulate why I have a gut reluctancy to silently blend "field types" with "pre-packaged field configurations of the same field type" in that same list :

- Well, that's a lie :-), the UI label (actually the hint text in the 'none' option) says "Select a field type", hinting that this lists, well, field types...
Field types, and how they cluster all the subsequent field functionnality (storage schema, widgets, formatters, behavior in views...), are a very structuring keystone in the overall mental model. I'm a bit wary to blur the lines in the screen where users have to make that choice.

- The newly added "grouping" is awesome, but we're suddenly using that UI formulation for two very different kind of groupings :
a) related but *different* field types (e.g int / float), those are not variants of the same,
b) prepackaged variants of the same type (reference configured to point to nodes or to terms), you can turn one into the other after the fact, or compose it yourself from the raw, unpackaged underlying field type and get to the same result.

- In the end, you create a field of a given field type, and after that initial "Add new field" form, the various UIs (Field UI, Views UI...) will show you *that* field type. So,
On the "Add new field" screen, if I pick "Field type : Taxo Term", when I'm back on the "Manage fields" screen, the UI tells me "Field type : Entity reference".
But if I picked "Field type : Image" just one line above in that same select group, the "Manage fields" screen would tell me "Field type : Image".
That seems like a WTF ?

That's for the -1's :-)
(Hopefully) more constructive thoughts in the next comment.

So, I'm very much +1 on getting rid of taxo field type, and thus to finding a way to mitigate the extra legwork in the UI.

I just think we should try to keep a distinction between "different field types" and "packaged configurations of a given field type" in the "Add new field" list.

Ideally, a hierarchical select would be nice here :

Group label
- field type A
- field type B
- field type C [ is selectable on its own, but has an arrow that opens a list of prepackaged variants: ]
  - variant C.1
  - variant C.2

But well, I'm afraid we don't have the corresponding UI tools in core atm :-)

So if we're stuck with native selects and optgroups with one flat list below each grouping, we should try to make it clearer whether an option is an actual field type or a pre-configured variant of one of the other options.

What about :

Reference (grouping label, not selectable)
  Entity Reference
    (pre-configured) Content
    (pre-configured) Taxonomy Term
    (pre-configured) User
  File
  Image

?

Also (sorry, haven't looked at the patch yet :-/) : not sure how we formulate and transfer what lies in a "preconfigured package" yet : storage level settings + instance level settings ?

if I pick of the "pre-configured entity_ref to nodes" option in "Add new field", I still get to visit the storage and field config forms to setup non-packaged options (cardinality, etc...), right ? Are the options that are part of the package greyed out / disabled, or are they simply used to prepopulate default values ?

Yched: I'm curious why you feel it's important to expose this implementation detail in the UI to site builders. We don't do this elsewhere in the UI. For example, in standard profile, "Page" and "Story" are effectively "pre-configured" variants of node module, whereas Book and Forum are provided by their own respective modules. Yet in the node/add page they are all presented as equals (as they should be).

Not a lot of time for d.o these days :-/, sorry for the delay and the answer that might not be very articulate.

I'm curious why you feel it's important to expose this implementation detail in the UI to site builders

So yeah, what i'm trying to say is that "the field type" is very much not an implementation detail to my eyes :-), it's a very structuring concept, grasping it moves you upward as a drupal dev or site builder.

People will get exposed to contrib modules building on / adding functionality on "entity_reference fields".
It's IMO very weird to have the UI obscure the fact that "Node" and "Taxo term" are in fact just entity_ref fields, while "File" and "Image" are their own separate field type.

My feeling is that , as is, the proposal does help the very beginners (which is absolutely something we need to do if we get rid of taxo field), but gets in the way and adds confusion for everyone else, once the pattern is out in the wild and used for other field types than entity_ref (like - why not include a pre-configured "positive integer" field ?). It means I can't know for sure which is what in that "field types" option list, unless I somehow know or remember "so this one is a real field type, but this one is just the former, prepackaged".

I'm totally fine with pre-packaged shortcuts if they are clearly identified as such, but I do feel that blurring the line between field types and shortcuts is not a good move. Trying to clarify "I'm a shortcut for the field type above" was what i was trying in #31.

"Page" and "Story" are effectively "pre-configured" variants of node module, whereas Book and Forum are provided by their own respective modules

I don't see the connection ? They are all separate "content types", and i don't think we call them differently anywhere in the UI ?
UIs that let you pick a node type list them side-by-side as "peers", and don't mix them with other things that are not node types ?

Also, for that same "limited time in yched's hands atm" reason - don't let me stall this :-p
I do have concerns with the proposed UI as is, if I'm the only one it bothers, then well it surprises me a bit ;-), but feel free to ignore me.

I also feel that these concerns are mostly a question of wording / UI adjustments. At worst, we can discuss and adjust in a followup after taxo field is dead :-)

Well, no other entity type annotation boolean start with 'is_' fair enough but I still think referenceable make more sense. It's one word. is_common_reference_target is way too long INMO.

#35: I think your explanation makes a lot of sense. So what if we do something like this:

  Entity Reference
  … or a pre-configured Entity Reference:
    Content
    Taxonomy Term
    User

i.e. we list the field type as per usual, but in addition to that, any field type with pre configured field options gets an option group as well, containing the preconfigured field options.

Except that doesn't work, because that'd imply 3 levels, but a <select> with option groups only supports 2 levels. Sigh. I guess we really do want something like https://www.drupal.org/project/hierarchical_select… But since this is very limited, and non-dynamic, perhaps we can do something with #states to build what we need?

#37: what about commonly_referenced? Every entity type is referenceable, it's the fact that an entity type is *commonly* referenced that matters.

re : referenceable / is_common_reference_target / commonly_referenced

That property optionally appears in Entity type definitions, and
- is given a meaning by entity_ref fields,
- only impacts what is shown in the "field type" select in Field UI's "Add new field",
right ?

Maybe the property name should try to make that scope and intent clearer ? field_reference_preconfigured_option ?
(or something else, but IMO clarity wins over brevity here)

IMO clarity wins over brevity here

+1

re @Wim #38:
Yeah, another drawback with hierarchical select or #states is that they require some action (click, hover...) before you see the list of available "preconfigured options".

If I get things right, one requirement for "make it obvious that the way to add taxo to a content type is to create an entity_ref field" is that the option is visible straight ahead without needing to explore / open stuff.

Which is why it seems that, short of a complete UI reformulation to move away from a select, which doesn't feel like a reasonable prerequisite if we want to move forward, our only margin is how we visually present this "single flat list of options under the optgroup" :
- prefix with "(pre-configured)" as suggested in #31 ?
- just visually indent with a couple spaces ?
- ... ?

Agreed that referenceable would not be good. Are the others not referenceable?

Why wouldn't this work then ?

- Number
    Integer
    Float
    [... snip...]
- Text
    [... snip...]
- Reference
    Entity Reference
      Content
      Taxonomy term
      User
      [... snip...]
    File
    Image

Prefixing with "(pre-configured)" will make it look scarier than it is IMO.

Visually indenting might work. The accessibility consequences of that should be minimal.

So this issue goes right to the heart of what Drupal is: a data modeling system. To structure content.

I think we can actually group all field types in either of two groups:
- primitives/self-contained data
- references (to other data in Drupal)

Would it make sense to change the "Add field" UI to something like this?

HEAD:

Add a new field
<select containing *all* field types>

Idea:

Add a new field

… that points to other data in this site: <select with all field types in the "reference" category>

… that contains data: <select with all other field types>

We currently present all field types as equals. One could argue that that is imposing developer thinking on the user of this UI. But when a user creates a new field type, he goes there with a specific goal in mind, for a specific data modeling purpose. And in data modeling, I think you're always modeling either data to be stored or data to be pointed at.

Just one small note on the patch: If you update UI text, you also need to update hook_help() that references that UI text. The place I noticed is that in the Taxonomy module, the latest patch changes the displayed field name for the taxonomy field to "Taxonomy term" instead of "Term reference". The function taxonomy_help() in taxonomy.module refers to this field by name, and needs an update. Thanks!

I think the patch also needs a hook_help() update for the Entity Reference module help, since that UI is also quite different?

I took a look at the patch... skipped looking at the tests and mostly I was looking at API docs. Found one other thing that needs fixing: in core/modules/field_ui/src/Form/FieldStorageAddForm.php

+   * @param bool|null $widget_id
+   *   (optional) The plugin ID of the widget. Defaults to NULL.
    */
-  protected function configureEntityDisplays($field_name) {
+  protected function configureEntityFormDisplay($field_name, $widget_id = NULL) {

I don't think it's "bool" on widget_id? isn't it a string?

Some before and after screen shots would be helpful in reviewing this patch, for anything changed in the UI. It looks like mostly it's the Add New Field process that changes -- both the form that lets you choose what field to add, and then... I'd assume the process afterwards? I don't have time to test it today but I think that screen shots would help in getting usability reviews.

Thanks for #35 and other comments. I'm curious if simply moving Image/File out of "Reference" would resolve it? And either put those in a different category such as "Media" or leaving them just in "uncategorized" for now. Then the only things under "Reference" are direct descendents of the Entity Reference field, and we don't need multiple levels.

As far as the property name goes, in my special rainbow unicorn pony happy land, this would not be a one-off property only for Entity Reference fields (which is what it is now), but could instead be a standard way to limit "sub-field" choices in this UI. For example, I could create a "Crypto" field type with 150 different kinds of text mangling, but only expose ROT-13 and SHA-1 by default (or whatever).

Not sure if that's actually doable, but that was my original thinking in suggesting it.

However, -1 to "(pre-configured)" (that's very geek-speak and as Wim said makes them look much scarier when the point is to expose them to make them easier to find) and -1 to * (that already means "required" elsewhere).

I tinkered with the idea of moving File and Image to "Media" as well.

However:
- From what I grabbed from the "plan for Media D8" meetings, it seems that Media D8 (or other contrib alternatives like scald) will be based on a simple entity_ref field on a "Media" entity type. In which case we'd have:

Reference
  Content
  Taxonomy Term
  Media
  Other
Media
  File
  Image

which would be... weird :-)

- That just sidesteps the issue, by only solving the UI case of pre-configured entity_ref ("easy, there are only pre-configured options + the native field type left in that optgroup, so no real need to tell them apart").

But what if, say, we want to add a pre-configured option for a "positive int" - then we're back to

Number
  Integer
  Positive Integer
  Float
  Decimal

Which puts us back into "we don't want people to think "Integer" and "Positive integer" are two separate field types"

"we don't want people to think "Integer" and "Positive integer" are two separate field types"

I'm still really, really struggling with why that is so bad. We don't make that distinction anywhere else in the UI:

- Both module-provided, "pre-configured" Blocks and custom Blocks are both listed in the Blocks interface.
- Both module-provided, "pre-configured" Content types and custom Content types are both listed in the Content types interface.
- Both distribution-provided, "pre-configured" Roles and custom Roles are listed in the Roles interface.
etc.

When I'm wearing my site builder hat, I literally do not care where something comes from. That's an implementation detail, far under the hood. I (may) care when I'm wearing my developer hat, if I'm doing something around this area, but then I can use developer-y tools like grep and api.d.o and devel inspectors and whatnot to figure those implementation details out when they are relevant to what I'm doing (which will only be when I'm dealing with field types, not when i'm dealing with routes or permissions or any of the other 100 things I could be doing as a developer).

I'll try reading #35/56 again later when I'm not about to leave and be supermom but it's not quite gelling for me currently. Field types, content types, etc. are all "domains" from a site builder perspective and we do not differentiate pre-configured from otherwise anywhere else, so I'm struggling with understanding why we would do something special here.

+100 to what @webchick says in #57. I don't think it matters to a site builder whether something is a special case of something else, or what type of special case it is. Anyway, site builders may have different notions of what field is a "special case" of what other field (logically) than developers do.

The thing is, UIs should always be task-oriented and user-oriented. The task here is "I want to add a field that works like this and stores this type of data to my entity". Let's make sure that the categories and labels for the "fields" (wherever they come from and whatever they are to developers) make sense to the site builder, independent of what they "really" are behind the scenes.

Given that idea, what is the current patch doing? And does it make sense to the site builder?

Thanks for the screen shot! To me, the proposed UI looks very sensible.

The green patch in #23 has now been discussed for a week. We haven't been able to formulate a better alternative. webchick strongly feels the current patch is the right approach (forgive me if I'm wrong), jhogdon is on board, as is Bojhan. I'm sorry yched, but we can still improve this later, and this is still an incremental step forward. So let's do it :)

I'd RTBC but…

1. #50:

   Just one small note on the patch: If you update UI text, you also need to update hook_help() that references that UI text. The place I noticed is that in the Taxonomy module, the latest patch changes the displayed field name for the taxonomy field to "Taxonomy term" instead of "Term reference". The function taxonomy_help() in taxonomy.module refers to this field by name, and needs an update. Thanks!

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -203,6 +203,16 @@ class EntityType implements EntityTypeInterface {
   +  protected $common_reference = FALSE;
   

   Let's change this to "common_reference_target" or "commmonly_referenced", per #39.

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -709,4 +719,11 @@ public function getConfigDependencyKey() {
   +  public function isCommonReference() {
   

   This should match whatever we end up choosing above.

Once those are fixed, I think we can RTBC this.

Added a bat signal.

#64: as I already said in #61, we already have usability approval. See #18. Bojhan is +1, but would prefer a better label, but can't think of one. He thinks this is a step forward regardless though.

EDIT: ROFL @ "batsignal" though :D :D

Let's do this!

Wanted to leave this a bit to see if yched made a resurgence, but since it's been a couple of days now...

Committed and pushed to 8.0.x. Last blocker bites the dust. :)

yched, I'm totally open to a follow-up to enumerate your concerns more and see if we can make some adjustments to address them. This might or might not be easier after the entity reference conversion happens, so we can see the "full picture" of what we're dealing with.

Yeah, sorry, hard for me to stay on top of d.o issues these days. As I said in #36, no problem with committing this.

I mean, I do hate with a passion that the UI smushes apples and oranges in that select list :-), and still intend to argue for #44 (just a matter of two spaces :-p) if I get a chance, but that can totally be debated in a followup.
Getting this in was the right thing to do, sorry for slowing things here.

@amateescu++. Enjoy you last moments on earth, taxonomy field...

Np — happy that you're +1 on the concept, just -1 on easy-to-amend-later details. :)