Problem/Motivation

Followup to #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title, which made it possible to have base fields be rendered using formatters / widgets when rendering an entity / entity form, if they are listed in the EntityDisplay used for rendering.

Base fields still don't have a real opportunity to get in the EntityDisplay though. The patch linked above hacked node.title to be added to the EntityDisplay at runtime through hook_entity_display_alter(). This hack is convoluted, makes #1875974: Abstract 'component type' specific code out of EntityDisplay impossible, and was only meant as a temporary step.

Proposed resolution

We now need to provide a real, usable API to specify widget / formatter display options for base fields, and optionnally expose those in "Manage display" / "Manage form display" UI screens.

We can't do that for all base fields, though, since this would overcrowd the list with 10s of fields ('nid', 'uid', 'status', 'created', 'changed', 'revision_timestamp'...). The approach discussed in DrupalCamp Vienna with @fago and @Gabor is to let base fields specify whether they want to appear on those pages.

Patch summary:

- Adds methods to FieldDefinition (definition class used for base fields), allowing base fields to opt-in into the "generic formatter / widget rendering" pipeline.

-- setDisplayOptions('form' | 'view', $options);
Specifies display options (widget / formatter settings + weight) for the field.
$options are the same than the ones already accepted by EntityDisplay::setComponent().

-- setDisplayConfigurable('form' | 'view', $bool);
Allows configurability in Field UI "Manage display" screens (if set, the $options above are treated as defaults - if setDisplayOptions() wasn't specified, those defaults are: 'hidden')

-- plus corresponding getters.

Example:

$fields['foo'] = FieldDefinition::create('text')
  ->setLabel(t('Some text'))
  ->setDescription(t('Some description.'))
  ->setRequired(TRUE)
  // Display in entity views using the 'text_default' formatter, 
  // allow overriding by configuration in the UI. 
  ->setDisplayOptions('view', array(
    'label' => 'hidden',
    'type' => 'text_default',
    'weight' => -5,
  ))
  ->setDisplayConfigurable('view', TRUE);
  // Display in forms using the 'text_textfield' widget,
  // harcoded, not overridable through config, not shown in Field UI.
  ->setDisplayOptions('form', array(
    'type' => 'text_textfield',
    'weight' => -5,    
  ));

Fields that don't specify anything stay out of the pipeline, and keep their current behavior: they are not displayed, or possibly displayed by their own custom ad-hoc render API / Form API code, and possibly exposed as "extra fields" to allow reorderability.

Progressively moving more base fields to the generic widget / formatter rendering pipeline can happen once this API is in place, and will require progress on #2150511: [meta] Deduplicate the set of available field types. It is out of scope here, and very likely won't be fully done within D8 - which is perfectly OK.

- The specified settings are merged into the EntityDisplay / EntityFormDisplay objects when they are loaded at runtime, so the rest of rendering pipeline stays untouched: widgets / formatters get generated according to what's specified in the EntityDisplay, regardless of whether it's base fields or configurable fields.
--> See EntityDipslayBase::init()

- On save and load, we take care of discarding display options for base fields for which isDisplayConfigurable() is FALSE, so that they do stay out of reach of config changes.
--> See EntityDisplayBase::init() and ::getExportProperties().

- This means slightly reorganizing the internal structure of EntityDisplay objects:
So far they didn't store any record when a field was set to 'hidden'. Now we need to keep track of what's "known to be hidden" vs what's "unknown to the Display, fetch defaults from the field definition". External facing API is unchanged.
--> See EntityDipslayBase::$hidden.

- The code generating Field UI "manage display" screens is adapted to handle base fields too, instead of only configurable fields so far.
Code moves from manipulating $fields / $instances to manipulating $field_definitions.
This changes the params for a couple field_ui hooks (see "API changes" section below)

Remaining tasks

None

User interface changes

"Manage Display" screens now have "widget / formatter" rows for base fields that opt-in for that. They behave just the same the ones for configurable fields.

API changes

hook_field_formatter_settings_form_alter()
hook_field_formatter_settings_summary_alter()
hook_field_widget_settings_form_alter()
hook_field_widget_settings_summary_alter()
--> $context['field'] / $context['instance'] are replaced by $context['field_definition']

The rest are API additions (methods on FieldDefinition / FieldDefinitionInterface)

CommentFileSizeAuthor
#78 base_fields_in_field_ui-2144919-78.patch56.15 KByched
#73 d8_entity_display.interdiff.txt897 bytesfago
#73 d8_entity_display.patch55.65 KBfago
#72 interdiff.txt2.03 KByched
#72 base_fields_in_field_ui-2144919-72.patch56.22 KByched
#69 interdiff.txt4.32 KByched
#69 base_fields_in_field_ui-2144919-68.patch55.93 KByched
#64 interdiff.txt759 bytesyched
#64 base_fields_in_field_ui-2144919-fago-64.patch55.19 KByched
#62 interdiff.txt859 bytesyched
#62 base_fields_in_field_ui-2144919-fago-62.patch55.2 KByched
#60 interdiff.txt3.31 KByched
#60 base_fields_in_field_ui-2144919-fago-60.patch55.14 KByched
#58 d8_display_options.patch56.21 KBfago
#58 d8_display_options.interdiff.txt4.91 KBfago
#56 d8_display_options.interdiff.txt9 KBfago
#56 d8_display_options.patch54.14 KBfago
#54 interdiff.txt2.56 KBchx
#54 2144919_54.patch53.87 KBchx
#52 widgetaroo-2144919.weights.patch53.84 KBlarowlan
#52 interdiff.txt601 byteslarowlan
#46 base_fields_in_field_ui-2144919-46.patch53.75 KByched
#43 base_fields_in_field_ui-2144919-43.patch53.75 KByched
#42 interdiff.txt3.92 KByched
#42 base_fields_in_field_ui-2144919-42.patch53.73 KByched
#34 interdiff.txt7.7 KByched
#34 base_fields_in_field_ui-2144919-34.patch53.89 KByched
#31 interdiff.txt1.34 KBswentel
#31 base_fields_in_field_ui-2144919-31.patch50.32 KBswentel
#28 base_fields_in_field_ui-2144919-28.patch49.81 KByched
#27 interdiff.txt999 bytesyched
#27 base_fields_in_field_ui-2144919-27.patch50.38 KByched
#23 interdiff.txt8.54 KByched
#23 base_fields_in_field_ui-2144919-23.patch50.37 KByched
#20 interdiff.txt1.5 KByched
#20 base_fields_in_field_ui-2144919-20.patch44.61 KByched
#19 interdiff.txt12.57 KByched
#19 base_fields_in_field_ui-2144919-19.patch44.32 KByched
#6 base_fields_in_field_ui-2144919-6.patch41.4 KByched
#115 base_fields_in_field_ui-2144919-115.patch68.27 KBeffulgentsia
#108 base_fields_in_field_ui-2144919-108.patch68.37 KByched
#97 interdiff.txt1.88 KBeffulgentsia
#97 base_fields_in_field_ui-2144919-97.patch67.9 KBeffulgentsia
#95 interdiff.txt691 bytesyched
#95 base_fields_in_field_ui-2144919-95.patch68.21 KByched
#94 interdiff.txt6.66 KByched
#94 base_fields_in_field_ui-2144919-93.patch68.08 KByched
#92 interdiff.txt8.45 KByched
#92 base_fields_in_field_ui-2144919-92.patch64.15 KByched
#90 interdiff.txt811 bytesyched
#90 base_fields_in_field_ui-2144919-90.patch57.42 KByched
#85 interdiff.txt956 bytesyched
#85 base_fields_in_field_ui-2144919-84.patch57.07 KByched
#80 2144919-80.patch55.64 KBtstoeckler
#80 2144919-78-80-interdiff.txt985 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Title: Allow widgets and formatters for base fields to configured in Field UI » Allow widgets and formatters for base fields to be configured in Field UI
yched’s picture

Priority: Normal » Major
Status: Postponed » Active

#1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title is in, unpostponing now.

As I posted in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), it would be best to solve this and thus define a "clean, non-hackish way" for base fields to leverage widgets/formatters, before we start enabling widgets/formatters on other base fields besides node.title.

andypost’s picture

OTOH #1875974: Abstract 'component type' specific code out of EntityDisplay could be done first and on top of it a field ui form could be updated

yched’s picture

Hm... yeah, tricky dependencies here. Ideally we would try to limit waterfall blocking, but that might not be easy.

This issue here is going to introduce the "default settings written in the field definition in code" pattern, which is an important design point that #1875974: Abstract 'component type' specific code out of EntityDisplay will need to account for - see my #33 over there.

So it looks like we need at least to get things rolling here first, and then - hopefully in parallel if possible at all - check that #1875974: Abstract 'component type' specific code out of EntityDisplay fills the needs.

yched’s picture

Assigned: Unassigned » yched

Started working on this

yched’s picture

Initial patch.

Still some polishing & API refining needed, but this should be something like it.

Patch summary:

- adds FieldDefinition::setDefaultDisplayOptions()
This lets base fields specify that they want to be rendered with a widget/formatter.
This also makes them configurable in Field UI (Field UI "Manage display" screens list configurable fields and base fields that have "DefaultDisplayOptions").

Example:

      $definition->setDefaultDisplayOptions('view', array(
        'label' => 'hidden',
        'type' => 'text_default',
        'weight' => -5,
      ))
      ->setDefaultDisplayOptions('form', array(
        'type' => 'text_textfield',
        'weight' => -5,
      ));

- Leverages that for node.title, and thus removes the hook_entity_display_alter() tricks that #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title had to use.

- Changes how EntityDiplsayBase manages those "components that have defaults written in code" (extra fields so far, and now base fields too). This is done as an init() step when the object is created, rather than on each indicidual getComponent() access. The components that are set to "hidden" are all placed in a separate property.
Once this init() step is done, the rest of the runtime methods (getComponent(), setComponent(), removeComponent()) become dead simple. This should simplify things a lot for #1875974: Abstract 'component type' specific code out of EntityDisplay :-)

Question:
Do we need to allow base fields to separately specify "render me using this widget / formatter" and "show me in Field UI to let site admins change those defaults" ? Currently setDefaultDisplayOptions() triggers both.

yched’s picture

Do we need to allow base fields to separately specify "render me using this widget / formatter" and "show me in Field UI to let site admins change those defaults" ?

Example: node title in entity view.
- In HEAD, Title appears on "Manage form display", but not in "Manage display".
- With the current patch, it appears in both. You can change the formatter (not super interesting with the core formatters), but reordering it has no effect since its location is hardcoded in the default tpl's.

swentel’s picture

but reordering it has no effect since its location is hardcoded in the default tpl's.

So does it mean we're going to/should kill the $title in the node.tpl for teaser ?

yched’s picture

Discussed with @amateescu on IRC, we agreed on
FieldDefinition::setDefaultDisplayOptions($display_context, $options, $field_ui = FALSE)

Rationale:
- 1st level is "do you want to be rendered using a widget / formatter (and which), or do you have your custom rendering code ?"
- Only if the former, then 2nd level is "do you want Field UI configurability for that widget / formatter ?"

So this is better as one single setter method, separate methods would allow ionvalid combinations.

I'll update the patch tomorrow.

yched’s picture

[edit: double submit]

effulgentsia’s picture

So does it mean we're going to/should kill the $title in the node.tpl for teaser ?

Not without Fences or something similar in core :) Something needs to provide the <h2> wrapper (as well as the <a>, though that could be made the formatter's or theme_field__node__title()'s responsibility if need be).

but reordering it has no effect since its location is hardcoded in the default tpl's

That's nothing new. That's been a problem in D7 as well, any time a template prints anything from content individually. I guess what's new is that we now have a core template that does that. If we can provide some kind of hook (or whatever) that lets code say "hey, this field won't respect its weight", and then Field UI puts those fields in a separate part of the table without draggers, that would be cool, but perhaps best for a separate issue?

amateescu’s picture

Small late night review for some obvious things that jump out:

  1. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -141,15 +152,19 @@ public function id() {
    +  public function PreSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
    ...
    +  public function PostSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
    

    Lowercase 'p' :)

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/FormDisplayOverview.php
    @@ -18,6 +17,11 @@
    +  var $displayContext = 'view';
    

    Should be 'form'.

yched’s picture

Sure, elements singled out in templates breaking "Manage display" reorder UI is nothing new, the novelty here is that it would involves vanilla core stuff, while so far it only happened if the site builders took explicit action (e.g. writing custom templates).

Flagging those fields so that Field UI presents them non-draggable is difficult, because what lies in tpls is varies for each theme and each tpl override. I don't really see us solving that soon, that's kind of the short-lived "Layout builder" business.

So yeah, for now I think we'll have to live with "stuff that appears reorderable but in reality isn't". Author in node forms, creation date in node views...

Node title is a bit tricky:
- We probably don't want to show it in "Manage display" (not reorderable, and not much value in changing the formatter). It just gets rendered using a hardcoded formatter and thus benefits from IPE.
But note that it then *always* get rendered, even for "full pages" where the tpl eventually choses not to use it. Small overhead, probably no biggie.
- We *do* want to show it in "Manage form display" (we did so far, as an "extra field" - reorder is valuable here)
But then the case of "$node_type->has_title == FALSE" is tricky. The patch alters 'title' out of the display at render time, but Field UI still presents it in "Manage form display", because we don't run alter hooks there (we don't alter stuff that gets saved back).
#2114707: Allow per-bundle overrides of field definitions would probably solve that, since it would allow us to do different setDefaultDisplayOptions() calls by node type.

yched’s picture

To summarize, base fields would have three options (they can choose one on forms and views independently, and on each bundle independently once #2114707: Allow per-bundle overrides of field definitions is solved).

The examples are proposals, up for discussion of course.

1. (By default) No automatic rendering & IPE support
Rendering, if any, has to be made by custom explicit code (for example in the theme preprocess / tpl).
Examples for nodes:
- forms: nid, vid, node_type, translatable, changed date, ...
- views: nid, vid, node_type, translatable, ...

2. Automatic rendering through a widget / formatter, IPE support
The field definition needs to specify a widget/formatter & weight.
The rendered result is placed in $content[$field_name] / $form[$field_name].

2.a No UI configurability - not listed in Field UI to reorder / change widget.
Customizations are possible but require runtime code (hook_entity_display_alter(), theme preprocess / tpl)
This is only suitable for fields that get explicitely located out of $content and into a fixed position by tpls / form_alter()s, because fixed-weight elements in the middle of tabledrag-reordered ones just result in impredictable order (other than -100 for "towards the top" / +100 for "towards the bottom").
Examples for nodes:
- forms: (?)
- views: title, author (?)...

2.b Configurable / reorderable in Field UI "Manage display" screen
The settings specified by the field definition just act as defaults.
Fields that get explicitely located to a fixed position by tpls / form_alter()s can use this if "change the widget in the UI" is deemed valuable, but then UI reordering has no effect.
Examples for nodes:
- forms: title, creation date, author
- views: (?)

Does that seem a suitable feature set ?

andypost’s picture

about node.title - I dont think there's a way to detect that title should not be used in entity render. When node used in 'full' mode it does not really means that its title should be excluded

The same applies to any other entity that could be rendered in 'full' or 'page' context, so do we really want to implement "form-display-alter" for each entity type?

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
@@ -141,15 +152,19 @@ public function id() {
+  public function PreSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {
+    // Sort elements by weight before saving.
+    uasort($this->content, 'Drupal\Component\Utility\SortArray::sortByWeightElement');

I dont think it's a good idea to sort on save.
Because it brings 2 troubles:
- it makes hard to track difference in VCS for config files
- sorting should be done anyway onLoad to allow hand-edit of yml files

EDIT I think we should sort in init() or prepare() because we are planing to separate hidden components out of content key

yched’s picture

Ordering on save is *especially* handy to compare VCS versions and to hand edit IMO. And the internal code relies on the stored weights, not the order, so we don't need to reorder on load or init().
I added that here because, aside from making sense generally, it really helps checking the results of "Manage display" form submits when developping/debugging this patch.
But if that's controversial I can remove it from the patch, no biggie.

swentel’s picture

But if that's controversial I can remove it from the patch, no biggie.

I really don't consider this controversial, Field UI is the place where most of the time the ordering will happen anyway.

Re: removing the title from the templates - so yeah, it probably doesn't make sense to do that unless we also start swapping out template files, but that's probably really to late .. :)

Really wild idea - could we write a (tag) formatter that uses drupal_pre_render_html_tag() then to render the h2 for the title ? I'm not fully sure whether this idea makes sense, I might try and explore this for fun over the weekend.

yched’s picture

re node.title / h2:
I'd tend to think the h2 is better off in the templates ?

yched’s picture

Status: Active » Needs review
FileSize
44.32 KB
12.57 KB

Updated patch:
- adds a flag for "should the display be configurable in the UI", as per #9 above
(doing so, renames the methods to setDisplayOptions() / getDisplayOptions(), since those are not necessarily about "defaults")
- node.title display is made configurable on forms, but not on views
- fixes entity.display yml files shipped in default config for the new "hidden" entry
- fixes @amateescu's remarks from #12

Let's have a test run.

yched’s picture

Fixes warning if the display options are set to "hidden" (& adds back a @todo that was wrongly removed in the previous patch)

The last submitted patch, 19: base_fields_in_field_ui-2144919-19.patch, failed testing.

The last submitted patch, 20: base_fields_in_field_ui-2144919-20.patch, failed testing.

yched’s picture

Should fix most of the fails.

- EntityDisplayBase::getFieldDefinition($field_name) now has to be "only within fields whose display is configurable", and so is based on ::getFieldDefinitions(). That one statically caches the definitions instead of the "fake entity" used to generate them, so ::$targetEntity disappears.
- ::getFieldDefinitions() now checks for ContentEntityInterface *before* instanciating the fake entity.
That one was funny, but long story short: EntityFormController::init(), which runs for all entity forms including ConfigEntities, tries to retrieve an EntityFormDisplay for the entity (a *config entity*). Bad things ensued when that entity is a FieldInstance - e.g instance delete forms.
Fixed our hack here, and opened #2151775: EntityFormController does things that only make sense for ContentEntities.
- Display objects in tests need to be created *after* the fields/instances the test is going to put in it, since $display->init() builds and caches the list of field definitions.
- Moved the "extra fields" used by entity_test to a separate bundle, so that they don't kick in when we don't want them.

The last submitted patch, 23: base_fields_in_field_ui-2144919-23.patch, failed testing.

dsnopek’s picture

The last submitted patch, 23: base_fields_in_field_ui-2144919-23.patch, failed testing.

yched’s picture

Er, oops...

yched’s picture

yched’s picture

I only have a couple documentation @todos left to complete.

Meanwhile, this is ready for review - along with approval of the featureset logic described in #14.

aspilicious’s picture

+++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
@@ -135,6 +135,17 @@ public function isTranslatable();
+   * Determines whether the disolay for the field is configurable.

display :)

And triple yes for the hidden section in the yml files :)

swentel’s picture

Played around by setting the title on my local codebase to be configurable in the UI and then changing the format to 'trimmed'. The formatter setting works, so good stuff.
But then moving the title to hidden (I know, not the best test, maybe, but still) triggered notices and the field was completely gone :)

Notice: Undefined index: parent_wrapper in Drupal\field_ui\OverviewBase->tablePreRender() (line 158 of core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.php).
Notice: Undefined index: #region_callback in Drupal\field_ui\OverviewBase->tablePreRender() (line 166 of core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.php).
Warning: call_user_func() expects parameter 1 to be a valid callback, no array or string given in Drupal\field_ui\OverviewBase->tablePreRender() (line 166 of core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.php).
Notice: Undefined index: weight in Drupal\field_ui\OverviewBase->tablePreRender() (line 173 of core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.php).

Two questions/remarks

  1. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -163,16 +175,55 @@ public function getExportProperties() {
    +        if ($definition instanceof FieldDefinition && $options = $definition->getDisplayOptions($this->displayContext)) {
    +          if (empty($options['type']) || $options['type'] != 'hidden') {
    +            $this->content[$name] = $this->pluginManager->prepareConfiguration($definition->getType(), $options);
    +          }
    +          else {
    +            $this->hidden[$name] = TRUE;
    +          }
    +        }
    +        else {
    +          $this->hidden[$name] = TRUE;
    

    Can't we simplify this a little ? Although I'm not sure whether this can be a bit more elegant, but it felt weird to read at first.

  2. +++ b/core/profiles/standard/config/entity.display.user.user.compact.yml
    @@ -11,6 +11,6 @@ content:
    +  member_for: true
    

    It feels weird to confirm that a field is hidden, but we probably can't get around it

Also - re-roll only after #1991292: Output of labels from hook_field_extra_fields() should not use check_plain()

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,38 @@ public function setPropertyConstraints($name, array $constraints) {
    +  public function setDisplayOptions($context, array $options, $configurable = FALSE) {
    ...
    +  public function getDisplayOptions($context) {
    ...
    +  public function isDisplayConfigurable($context) {
    

    We should rename this param to $display_context, I was really confused about what $context refers to until I scrolled down and saw the interface..

  2. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -45,15 +48,11 @@
    +   * @var \Drupal\Core\Field\FieldDefinitionInterface][
    

    This doesn't look right :)

  3. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -163,16 +175,55 @@ public function getExportProperties() {
    +    $extra_fields = field_info_extra_fields($this->targetEntityType, $this->bundle, ($this->displayContext == 'view' ? 'display' : $this->displayContext));
    

    Maybe not in this issue, but how about changing extra fields to accept 'view' instead of 'display'?

  4. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -163,16 +175,55 @@ public function getExportProperties() {
    +      if (!isset($this->content[$name]) && !isset($this->hidden[$name])) {
    ...
    +      if (!isset($this->content[$name]) && !isset($this->hidden[$name])) {
    

    One possible way to reduce the indentation level here is to reverse the conditions and use continue, with a helpful comment above the condition.

  5. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php
    @@ -18,6 +17,11 @@
    +   * @{inheritdoc}
    

    {@ instead on @{

  6. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,38 @@ public function setPropertyConstraints($name, array $constraints) {
    +    $this->definition['display'][$context] = array(
    +      'options' => $options,
    +      'configurable' => $configurable,
    +    );
    
    +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -320,14 +302,37 @@ public function getHighestWeight() {
    +        // @todo getDisplayOptions() is not part of FieldDefinitionInterface, we
    +        // should check for instanceof FieldDefinition
    +        return $field_definition->isConfigurable() || $field_definition->getDisplayOptions($context);
    

    I see that FieldDefiniton::$definition['display'] becomes the goto place for base field display information. I'm not yet sure how I feel about that, but shouldn't we at least clear that static cache in set/removeComponent()?

    Also, we need to fix that @todo in this patch :)

aspilicious’s picture

And we probably need some extra tests, to catch those notices.

yched’s picture

Filled-in missing docs and @todos, and adressed the remarks above.
Will look into the notices mentioned by @swentel.

@swentel #31:
1. Slightly reordered, and tried to clarify with code comments
2. Yes, that's the long-standing tricky thing with EntityDisplays and components that are defined in code. We need to be able to differ between "I know nothing about this component, apply the defaults" & "I know that this component is hidden" - and this now applies to base fields in addition to just "extra fields" so far. So we need to list a couple more things than before in EntityDisplay, I see no other way, and the 'hidden' section solution results in way simpler logic than in HEAD.

@amateescu #33:
1. Fixed
2. Lol, fixed
3. Fully agreed, but I prefered not derailing that issue
4. Matter of taste, but I personally find "continue;" logic harder to parse, actually. As mentioned above, I tried to clarify that area a bit.
5. Gee, guess i was tired that night...
6. Hmmm, not sure about that. A (base) FieldDefinition is defined in code, collected and cached, it doesn't change at runtime.
Anyway, this static cache of "filtered" definitions in EntityDisplay is currently mostly here because EntityManager::getFieldDefinitions() cannot hand us the right $instances, so we need to collect them ourselves by other ways (the _field_create_entity_from_ids() part), and that is costly so we cache it. When that is fixed, the static cache in EntityDisplay might be removed.
I added a note in the existing @todo about that.

amateescu’s picture

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
@@ -163,16 +174,67 @@ public function getExportProperties() {
+        // Note: base fields that do not specify display options are not
+        // tracked in the display at all, in order to avoid cluttering the
+        // configuration that gets saved back.

Maybe move this to the doxygen block instead of hiding it at the end of the method?

I added a note in the existing @todo about that.

I'm okay with the explanation for #33.6 but I don't see that note in the interdiff :-s Oh, nvm, it's in the patch.

After you check those notices I'd say this is RTBC :)

yched’s picture

@swentel, I can't seem to reproduce the errors you mention ?

If I change the code to make node.title be "configurable" in entity views, all I get is "undefined index in template_preprocess_node()" if I drag Title to be hidden in teasers - which is normal, our templates currently hardcode the existence of $content['title'].

Can you provide steps to reproduce ?

swentel’s picture

@yched hmmm, I can't seem to trigger it anymore, weird, scrap that thing then.

amateescu’s picture

I've gone through the entire patch once more and it looks great! Going into ultra-nitpicky mode :)

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,68 @@ public function setPropertyConstraints($name, array $constraints) {
    +   *   - weight: (float) The weight of the element.
    

    It's kind of weird for a weight to be float instead of integer.. are we doing this anywhere else? And there's an extra empty line below.

  2. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,68 @@ public function setPropertyConstraints($name, array $constraints) {
    +   *   Whether the display options can be configured in the "Manage display" /
    

    We should slap an '(optional)' in front of this :)

  3. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,68 @@ public function setPropertyConstraints($name, array $constraints) {
    +   * @return array|null
    +   *   The array of display options for the field, or NULL if the field is not
    +   *   displayed.
    

    We're returning an empty array, not NULL, so either the code or the text needs to be updated.

  4. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,68 @@ public function setPropertyConstraints($name, array $constraints) {
    +   * Return the display options for the field.
    

    Returns ...

  5. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
    @@ -135,6 +135,17 @@ public function isTranslatable();
    +   *   TRUE if the display for this field is configurable.
    

    , FALSE otherwise?

  6. +++ b/core/modules/forum/config/entity.display.taxonomy_term.forums.default.yml
    @@ -3,8 +3,7 @@ uuid: adaef6a9-8dc0-4f2e-9858-88daac440aa9
    -status: true
    +status: 1
    
    +++ b/core/modules/forum/config/entity.form_display.taxonomy_term.forums.default.yml
    @@ -3,11 +3,9 @@ uuid: c8eab085-8fd3-4545-8600-e13b7d8bb9c4
    -status: true
    +status: 1
    

    Is this accidental or intended?

Wim Leers’s picture

Also a nitpicks review.

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,68 @@ public function setPropertyConstraints($name, array $constraints) {
    +   *     unknown, the  'default_widget' / 'default_formatter' for the field type
    

    Double space after "the".

  2. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,68 @@ public function setPropertyConstraints($name, array $constraints) {
    +   *
    

    This newline shouldn't be here.

  3. This was a duplicate.
  4. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -363,7 +365,16 @@ public static function baseFieldDefinitions($entity_type) {
    +      ), TRUE);
    ...
    +      ))
    +      ->setDisplayOptions('view', array(
    +        'label' => 'hidden',
    +        'type' => 'text_default',
    +        'weight' => -5,
    +      ))
    +      ->setDisplayOptions('form', array(
    +        'type' => 'text_textfield',
    +        'weight' => -5,
    

    Nice :) Now it's all in one place!

amateescu’s picture

2. and 3. are also in #38...

fago’s picture

  1. edit: Removed needlessly duplicated @var comments.
  2. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -163,16 +174,67 @@ public function getExportProperties() {
    +        elseif ($definition instanceof FieldDefinition && $options = $definition->getDisplayOptions($this->displayContext)) {
    

    The check on a specific implementation class feels wrong here, it should check for an optional interface instead (if it's optional). E.g. DisplayableInterface ? I'd be fine with having the method in the main interface though along with the meaning of - if it's configurable, it's the default. If there is no display, the return value of NULL seems fine to me.

yched’s picture

Thanks for the reviews folks :-)

@amateescu #38
1. Well, stricly speaking, what you specify is a FAPI #weight here, so well, we do accept floats. If configurable, Field UI's tabledrag will renumber to ints, but that's not really relevant here...
2. Fixed
3. Right, we should be returning NULL, that's consistent with EntityDisplay::getComponent()
4. Fixed
5. The other isXxx() methods in the class cut the "FALSE otherwise" verbosity, so this stays consistent.
6. Right, merge snafu I guess. Fixed.

@Wim #39
1. Fixed
2. 3. Dupes with above
4. :-)

@fago #41
2. Been trying to avoid that too, but couldn't find a satisfying solution.

getDisplayOptions() has no business in FieldDefinitionInterface, I can't provide an implementation that makes sense for configurable Field / FieldInstance, they don't specify their display options, only base fields do. And we named the definition classes for base fields "FieldDefinition", so what would the interface be for those specifically ? Not FieldDefinitionInterface... :-)

What could be done is adding a FieldDefinitionInterface::isDisplayed() method. Then
EntityDisplayBase::getFieldDefinitions() could use that instead of "if ($definition instanceof FieldDefinition && $definition->getDisplayOptions()", since all we need is a bool there.

But EntityDisplayBase::init() would still need to call $definition->getDisplayOptions(), and thus check for the FieldDefinition class, because there we do need the $options result.
So in the end, not sure adding that isDisplayed() method is really worth it.

Is the insteanceof check really problematic ?

yched’s picture

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Is the insteanceof check really problematic ?

This can be discussed in a followup. I, for one, don't think it is that problematic.

Let's get this sucker in!

The last submitted patch, 6: base_fields_in_field_ui-2144919-6.patch, failed testing.

yched’s picture

Er, patch #6 failed testing, really bot ? ;-)

Anyway, reroll just in case.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: base_fields_in_field_ui-2144919-46.patch, failed testing.

dsnopek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: base_fields_in_field_ui-2144919-46.patch, failed testing.

yched’s picture

No time to investigate tonight, but apparently this breaks the comment paging tests introduced in #2155387: Multiple comment fields per entity are supported, but break horribly when they need pagers.

Hints welcome :-)

larowlan’s picture

/me takes a look

larowlan’s picture

Status: Needs work » Needs review
FileSize
601 bytes
53.84 KB

Something to do with weights.
In HEAD two fields with weight 20 on display settings, named comment and comment_2 => comment rendered first.
With this patch, comment_2 rendered first.
This fixes it but might mask a deeper 'what happens when two equal weights' issue, although that's not possible via the ui.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per @amateescu comment

chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
53.87 KB
2.56 KB

var is a PHP4 construct and while it still works, we don't use it any more. I will put this back to RTBC but the bot only picks up from NR.

chx’s picture

Status: Needs review » Reviewed & tested by the community
fago’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
54.14 KB
9 KB

@fago #41
2. Been trying to avoid that too, but couldn't find a satisfying solution.

hm, I think the code needlessly differentiates between configurable fields and others, what makes it more complex. So what about the following:

- Do not hard-code configurable vs not configurable field distinctions in to the display, but let configurable fields specify their defaults by specifying a respective default by implementing getDisplayOptions().
- Add getDisplayOptions() to the interface and avoid this problematic check

Apart from that, I think setting display configurability should be a separate setter, as it is a separate getter and we usually have a 1:1 map between getters and setters. So, attached patch separates this for consistency (and results into a better readable field definition imho).

The last submitted patch, 56: d8_display_options.patch, failed testing.

fago’s picture

Note sure why this wasn't popping up earlier, but we've got a clash between the extra field "langcode" defined by node.module and the langcode base field.

Also, we've got non-configurable fields which have no default_widget or formatter defined, but the code assumes that this is there. I'd suggest adding the default value of 'hidden' to the field type definition, but we need to fix the langcode problem first as else the langcode extra field would be hidden.

Opened #2157545: Convert langcode extra fields to formatter and widget plugins for that.

Attached is an updated patch which adds an interim check + the @todo. Also, I moved the display options array documentation into the interface - it's probably better to consolidate such documentation at the interface.

Then, I noted that the defined display options are not per display mode. Shouldn't I be able to specify different display options per display mode?

Finally, it's a bit funny to have two layers of adding default display options, for one we've got the field type default added in via the widget/formatter manager, then we've got default handling of the entity display (which I refactored). Wouldn't it make sense to handle that all via the entity display / field definition default logic, or is there a reason we don't do that?

fago’s picture

Wouldn't it make sense to handle that all via the entity display / field definition default logic, or is there a reason we don't do that?

Answering the question myself: No it would not make sense, as it are different defaults. The one are defaults on field type level, while the others are on field definition level. So having both makes sense.

This confused me over at Formatter/WidgetPluginManager::prepareConfiguration() as there the field-type defaults gets applied on a field definition level if there are display options specified *without* a formatter/widget. However, supporting display options for a field without a specified widget/formatter seems quite weird to me, imho at least the widget/formatter should be required here?

I could see a display option without formatter/widget being useful for keeping the default + specifying the weight only. However, as the default
behaviour of no display-config is being hidden, this seems to be deprecated to me? Howsoever, we cannot fix/change that until #2157545: Convert langcode extra fields to formatter and widget plugins gets resolved.

yched’s picture

Sorry for the absence, busy week. Getting back at this.

The changes around setDisplayoptions() / setDisplayConfigurable() / getDisplayOptions() do stuff that I was precisely trying to avoid. I'll mull a bit more on that and try to formulate my objections.

For now:
- EntityDisplayBase::getFieldDefinitions() is used to separate "what is rendered with a formatter/widget" vs "what is not" - i.e fields that are still rendered with custom code and expose themselves as 'extra fields' (e.g. 'langcode').
So it really needs to only be about fields that expose displayOptions(). I re-added a filter in there.
- Thus, reverted the hotfixes in FormatterPluginManager / WidgetPluginManager.

The last submitted patch, 60: base_fields_in_field_ui-2144919-fago-60.patch, failed testing.

yched’s picture

Grumble... 5.3 / $this in closures...

The last submitted patch, 62: base_fields_in_field_ui-2144919-fago-62.patch, failed testing.

yched’s picture

yched’s picture

So, regarding my reservations with the latest patches:

+++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
@@ -188,6 +188,63 @@ public function setPropertyConstraints($name, array $constraints) {
+  public function setDisplayOptions($display_context, array $options)
+  public function setDisplayConfigurable($display_context, $configurable) {

I went for one single setDisplayoptions($options, $is_configurable) method because you shouldn't be allowed to do setDisplayConfigurable(TRUE) without specifying display options. It's a two step thing : "do you want to be part of the widgets/formatters rendering and how ? and *if yes*, do you want it to additionally be configurable in the UI ?". That's what #14 is about.

If your definition doesn't specify ->setDisplayoptions() (which is the case for most base fields), you stay out - not rendered, or rendered by your own ad-hoc code, not our business.
If you just call ->setDisplayConfigurable(), what would that mean ? "don't mind me, but make this configurable" ? That's not something we intend to support. You're in (possibly "hidden by default + configurable") or you're out. I went with an API that reflects that.

+++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
@@ -628,6 +628,21 @@ public function isConfigurable() {
+  public function getDisplayOptions($display_context) {
+    // Hide configurable fields by default.
+    return array('type' => 'hidden');
+  }

As is, this makes no sense unless you know EntityDisplay only treats those as defaults.

I wanted to avoid cluttering the method names too much for baseFieldDefinitions() DX, but if we're going there I'd rather have:
$definition->setDefaultDisplayOptions($context, $options, $configurable)
$definition->getDefaultDisplayOptions()
$definition->isDisplayConfigurable()

Waiting for an agreement before doing the changes though :-)

fago’s picture

I went for one single setDisplayoptions($options, $is_configurable) method because you shouldn't be allowed to do setDisplayConfigurable(TRUE) without specifying display options.

Not sure about that. Field types have a default widget/formatter, so the display options should be optional. I could just enable display configurability and it can work with the defaults?
So even if the default is "hidden" (which I think should be the default "default_widget|formatter" if left unspecified), turning configurability on alone makes sense to me - that means people are allowed to configure a widget/formatter via the UI then.

The other remarks boil down to that issue also, so I think that's the one we need to agree upon first: What should be the default if no display options are specified?
Having discovered the default_widget and default_formatter settings of a field type, I think one would expect them to be applied.

yched’s picture

What should be the default if no display options are specified? [...]
Field types have a default widget/formatter, so the display options should be optional. I could just enable display configurability and it can work with the defaults

1)
I don't think this works. Again, this is what #14 is about:

The first default is : a field that says noting at all about "display" (e.g node type, id, vid, uuid, promote - most of them probably) stays out of the widget/formatter pipeline. It's not rendered, or possibly it takes care of rendering itself with its own arbitrary render/FAPI code, and then possibly exposes this abritrary render array as an "extra field" for Field UI... - we don't know and don't care.

What you suggest is that just adding "->setDisplayConfigurable(TRUE)" on such a field would trigger "gets rendered by the default widget/formatter". That's a change that's totally not implied by the method name, which is just about a "configurable" flag on top of the display options specified otherwise (whether "displayed" or "hidden").

Without setDisplayConfigurable(TRUE) : field is not visible
Add setDisplayConfigurable(TRUE) -> field becomes visible ??

I don't think the argument of 1:1 mapping between getters & setters holds here. It's a progressive API - you add display options, *then* you can say whether those options are configurable.

If you really want two separate setters, then all I can propose is:
"->setDisplayConfigurable() alone" does an implicit ->setDisplayOptions('type' => 'hidden'). The field stays hidden by default, but it becomes eligible for widget/formatter configuration in the UI.

2)
Yes, on the EntityDisplay side you can do:
$display->setComponent($field_name) without an explicit $options parameter,
and this gets set as "displayed using the default widget/formatter for the field type, with its default settings, and #weighted below all the current components in the Display".

This is mostly useful:
- for less verbosity in test setups
- when you create a new "configurable field" in the UI, Field UI does this to have the new field automatically displayed in the 'default' view mode, because that's the most commonly expected definition of "it just works" (add a field -> without further configuration it's instantly shown on the node full page and not in teaser/RSS/whatever).

I'm not against allowing that here as well, but I'm not too convinced it's really valuable for base fields:
- An explicit declaration of the intended widget/formatter should IMO be encouraged over "just use the default for the field type", since that default can potentially change over time. But why not, I'm not strictly opposed to a syntax that allows that.
- The absence of an explicit weight is more problematic though. If a base field does not specify an explicit default weight, the resulting default display order is going to be... what ? FWIW, hook_field_extra_fields() requires defining a default weight for that reason. This IMO largely invalidates the usefulness of a "just display me, don't bother me with details" DX sugar.

-----------------

My proposal if we must have two methods for default options / configurability:

a) $definition says nothing about display:
Field is not part of the widgets/formatters pipeline. Maybe it's rendered by ad-hoc code, maybe it's additionally exposed as an "extra field" for the UI... not our problem.

b) $definition->setDdisplayOptions($options with either 'hidden' or at least a weight);
Field is displayed using the options (default formatter/widget if not specified)

c) $definition->setDdisplayOptions($options with either 'hidden' or at least a weight)
->setDisplayConfigurable(TRUE):
Defaults to same as above, but actual display is configurable in the UI.

d) $definition->setDisplayConfigurable(TRUE)
Hidden by default, but can be switched to a widget/formatter in the UI.

effulgentsia’s picture

I like the proposal at the end of #67. I agree that without an explicit call to setDisplayOptions(), a base field should not be displayed. I.e., the default is "hidden", not the field type default. Perhaps some utility method to get the field type default would be handy, so you could have code like $definition->setDisplayOptions($context, array('type' => DEFAULT_FORMATTER($field_type), 'weight' => 5)); but that's a minor thing that can be discussed in a separate issue.

yched’s picture

Attached patch does as per the end of #67

fago’s picture

Without setDisplayConfigurable(TRUE) : field is not visible
Add setDisplayConfigurable(TRUE) -> field becomes visible ??

Of course setDisplayConfigurable() should not change the default behaviour, it would just allow you to change the default via the UI - whatever the default is.

..., a base field should not be displayed. I.e., the default is "hidden", not the field type default

I see the reasoning behind that and that we need a weight anyway, so that works for me. I think we should document how the default_formatter|widget works though, i.e. where it is used, so that people won't get confused by that. I took my quite a while to realize how that plays together.

The proposal from #67 looks good to me - I just don't see a) as separate case to cover. Imho every field should always go through the rendering pipeline, and for a) it's just 'hidden' and not configurable.

Perhaps some utility method to get the field type default would be handy, so you could have code like $definition->setDisplayOptions($context, array('type' => DEFAULT_FORMATTER($field_type), 'weight' => 5)); but that's a minor thing that can be discussed in a separate issue.

As yched mentioned this should already be possible by just specifying the weight. So I guess this could need documentation also then.

So, apart from the mentioned docu-issues the patch looks good to me.

One thing came to my mind though - maybe related to handling a) separately: I'm not sure we need the somehow duplicated state of "hidden" which is modelled implicitly via "it's not set" and another time explicitly with "type => hidden" - as it should get handled the same, not? But that's a minor internal and shouldn't bother us much right now. It could be improved in another issue or not.

yched’s picture

I just don't see a) as separate case to cover. Imho every field should always go through the rendering pipeline, and for a) it's just 'hidden' and not configurable.

We can't do that, there are fields that are not part of the widget / formatter pipeline, but are rendered by ad-hoc code + exposed as "extra fields".
Those have actual entries in the EntityDisplay (as "extra fields", not as "fields (rendered by widgets)"), which we cant just crush with 'type => hidden'.

That's the whole intent of the patch: re-establish a clear separation between what EntityDisplay sees as a "field" with a 'type = some_widget|hidden', and what it sees as an "extra field", so that #1875974: Abstract 'component type' specific code out of EntityDisplay becomes possible again.

I think we should document how the default_formatter|widget works though, i.e. where it is used, so that people won't get confused by that. I took my quite a while to realize how that plays together.

The behavior of the 'default_widget/formatter' is documented in FieldDefinitionInterface::getDisplayOptions(). Where do you feel it should go ?

yched’s picture

re: docs - Does this do the job ?

fago’s picture

We can't do that, there are fields that are not part of the widget / formatter pipeline, but are rendered by ad-hoc code + exposed as "extra fields".

I see. Imo it's a bit unfortunate as instead of a custom extra-field rendering and configurable extra field, it should just expose a custom formatter/widget. However, we are not there yet - so this is how it has to be now.

The 'default_widget/formatter' docs improvements are good to me, the getDisplayOptions() docs are still a bit confusing regarding which defaults we are talking about. So what about the following?

yched’s picture

instead of a custom extra-field rendering and configurable extra field, it should just expose a custom formatter/widget

Yeah, I'm not sure how that notion of "something that is entirely one-off/custom but somehow would still be a widget / formatter" would work... As you say, not there yet :-)
Also, that wouldn't remove the need for "extra fields" to support stuff like https://drupal.org/project/eva - regardless of what happens with base fields, there are still use cases for non-field, arbitrary but reorderable additions in rendered entities.

I don't really get the benefit of interdiff #73, as it's removing info more than it's adding, but it works for me if it works for you :-p

yched’s picture

58: d8_display_options.patch queued for re-testing.

The last submitted patch, 58: d8_display_options.patch, failed testing.

The last submitted patch, 58: d8_display_options.patch, failed testing.

yched’s picture

Re-tested the wrong patch :-/, but patch needed a small reroll anyway.

Status: Needs review » Needs work

The last submitted patch, 78: base_fields_in_field_ui-2144919-78.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
985 bytes
55.64 KB

Status: Needs review » Needs work

The last submitted patch, 80: 2144919-80.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

80: 2144919-80.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 80: 2144919-80.patch, failed testing.

fago’s picture

I don't really get the benefit of interdiff #73, as it's removing info more than it's adding, but it works for me if it works for you :-p

:D, yeah - imho this nails it better while the part about when it get applied confused me.

Yeah, I'm not sure how that notion of "something that is entirely one-off/custom but somehow would still be a widget / formatter" would work... As you say, not there yet :-)

Yep, I guess widgets/formatters could need a no_ui or configurable setting.

Also, that wouldn't remove the need for "extra fields" to support stuff like https://drupal.org/project/eva - regardless of what happens with base fields, there are still use cases for non-field, arbitrary but reorderable additions in rendered entities.

Sure, but those shouldn't clash with fields, ideally even if the same name is used? But that's probably more a topic for the display component patch.

yched’s picture

Status: Needs work » Needs review
FileSize
57.07 KB
956 bytes

Test fail in DrupalUnitTestBaseTest after #2005716: Promote EntityType to a domain object.

testEnableModulesFixedList() creates an EntityDisplay for the 'entity_test' entity type, while user.module is not enabled - and entity_test entities have a 'user_id' entity_ref field pointing to users.

This fails with the patch because entity_create('entity_display') triggers EntityDisplay::init(), which currently still triggers creating a fake $entity to collect field definitions.
--> breaks in EntityReferenceItem::getPropertyDefinitions(), \Drupal::entityManager()->getDefinition($target_type) is NULL and NULL->isSubclassOf() fatals out...

Before #2005716: Promote EntityType to a domain object, that code was just doing is_subclass_of(NULL['class'], 'ContentEntityInterface'), which worked fine because PHP is magic :-p.

I have no clue why this test creates an EntityDisplay to begin with (and does absolutely nothing with it, not even saving it) - this got added in #1331486: Move module_invoke_*() and friends to an Extensions class iteration #207, with no explanation I can make sense of :-p.

This raises an interesting question about entity_ref & dependencies, but that's far outside the scope of this patch... For now, changed the test so that it installs user.module at some point.

yched’s picture

Green. All we need is an RTBC now ;-)

effulgentsia’s picture

I wanted to RTBC this patch, I really did, but...

+++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
+ /**
+   * Returns whether the display for the field can be configured in the UI.
+   *
+   * @param string $display_context
+   *   The display context. Either 'view' or 'form'.
+   *
+   * @return bool
+   *   TRUE if the display for this field is configurable in the given context.
+   */
+  public function isDisplayConfigurable($display_context);

The top line of the doc says "in the UI", but neither the method name nor the @return doc says that. The actual functionality of the patch does implement this as a UI hint only, as per the initial line, but I think that's undesirable, because, for example, in this patch Node.php decides to make the node title display in 'view' context not configurable, but then if you open up entity.display.node.article.teaser.yml from the active config directory, 'title' is right there ready to be configured. And, if you copy this file to staging, and make changes to 'title' (e.g., change the formatter), and sync them back to active, then those changes do take effect.

I guess the question is, when someone is writing an entity type, and deciding which fields should have their displays be configurable, are they wanting to affect the "Manage Display" UI only, or are they also wanting to prevent hand-editing of YAML files from overriding their code-level decisions? My current thinking is that in general, the latter, but node.title doesn't really present a very compelling use case for that thinking. I wonder if other base fields would, however.

If we wanted to change this patch to my way of thinking, I don't think it would be too hard. I think it would just mean changing EntityDisplayBase::init() to only iterate display-configurable fields, and to add the non-display-configurable ones inside entity_get_render_display() prior to the alter() step.

On the other hand, if we think the functionality of the patch is what is desirable, then can we change the method name to something like isDisplayUIConfigurable(), and similar for related methods?

effulgentsia’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Regardless of #87, the issue summary could use an update.

yched’s picture

Right, currently all display options are written in the EntityDisplay config files, even if "not explicitly configurable". So technically they are all in config land and can all be manually changed.

I was not too sure whether that was really a problem.
- The alter hook makes everything potentially under the reach of config-land changes anyway.
- There are other places in core that store things in config and rely on convention (e.g. a 'locked' property) to make them editable in the UI - but well, I'm the 1st one to claim that those are awkward / brittle :-p.
- I think I remember some discussion in Prague's pre-conf "hard discussion room" where someone (@msonnabaum ?) argued for "just put everything that will be displayed", so that's what I went for. But the memory might be wrong, and this can totally be reconsidered anyway.

In short - Yeah, I probably agree that it would be best if we could clean that...

That would mean:
- an EntityDisplay yml file never holds the full story, only stores stuff about components that are supposed to be configurable. What we currently do in EntityDisplayBase::init() (fill in defaults for components we have never heard about) should be restricted to "the components that are configurable"
- it gets completed at runtime, before being used for rendering, with the hardcoded display options for "non-configurable components"

Drawbacks:
- Introduces "state" in the lifecycle of the Display, where it doesn't contain the same things depending on which step it's in.
- Related: if you write some code that ->save()s the Display after using it for rendering, well, you still end up with storing stuff that's not supposed to be configurable. So unless we write some safeguard code that removes the corresponding entries on save, we're still not clean, strictly speaking.
- I'd rather avoid further hardcoding in the "generic code in EntityDisplayBase" the fact that our current core implementations are ConfigEntities and are stored in config. We'll see how the code can be written...

yched’s picture

Status: Needs work » Needs review
FileSize
57.42 KB
811 bytes

Actually, this is probably enough to do the trick:
- in init(), fill in everything as before (configurable or non configurable)
- in getExportProperties(), remove knowledge about things that are not "set to be configurable"
(happens in getExportProperties() rather than preSave() so that we don't mess with the runtime Display properties if it's reused after being saved)

effulgentsia’s picture

Thanks. That interdiff looks like a good approach.

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
+  public function init() {
...
+    foreach ($fields as $name => $definition) {
+      if (!isset($this->content[$name]) && !isset($this->hidden[$name])) {
+        $options = $definition->getDisplayOptions($this->displayContext);

Can we also change this to apply getDisplayOptions() for non-display-configurable fields even if they are in ->content and ->hidden? So that someone can't hack their YAML file as a way to configure something they shouldn't be allowed to.

if you write some code that ->save()s the Display after using it for rendering

Pre-existing condition in HEAD due to the alter(). I think a separate issue to put the display into locked-from-saving mode prior to calling the alter() would be a good one.

Can we also add tests to this patch to make sure we have coverage for all 3 cases: display-configurable, display-nonconfigurable, display-no-options?

yched’s picture

Done, & tests.

effulgentsia’s picture

Thanks. And now for the nits.

  1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -188,6 +188,69 @@ public function setPropertyConstraints($name, array $constraints) {
    +   * Sets whether the display for the field can be configured in the UI.
    +   *
    +   * @param string $display_context
    +   *   The display context. Either 'view' or 'form'.
    +   * @param bool $configurable
    +   *   Whether the display options can be configured in the "Manage display" /
    +   *   "Manage form display" UI screens. If TRUE, the options specified via
    

    Remove references to UI?

  2. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
    @@ -135,6 +135,48 @@ public function isTranslatable();
    +   * Returns whether the display for the field can be configured in the UI.
    

    Remove "in the UI"?

  3. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentPagerTest.php
    @@ -281,7 +281,7 @@ function testTwoPagers() {
    -        'weight' => 20,
    +        'weight' => 30,
    

    Why?

  4. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -164,16 +175,71 @@ public function getExportProperties() {
    +      'hidden',
    

    I was about to say this needs to be added to entity.schema.yml, except that's missing the display schema entirely. Separate issue, then, I guess.

  5. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -373,6 +373,8 @@ public static function baseFieldDefinitions($entity_type) {
    +      // @todo Account for $node_type->title_label when per-bundle overrides are
    +      //   possible - https://drupal.org/node/2114707.
    

    Thanks for this @todo.

  6. +++ b/core/modules/node/node.module
    @@ -217,16 +211,8 @@ function node_entity_display_alter(EntityViewDisplayInterface $display, $context
    +    if (!$node_type->has_title) {
           $form_display->removeComponent('title');
         }
    

    Should we put the same @todo here? Since I think this can go away once the per-bundle definition doesn't have it, right?

+ we still need the issue summary to explain everything the patch changes.

yched’s picture

Forgot to add the tests on the forms side
+ a bit of self review.

yched’s picture

Crosspost with #93.

1. 2. Really sure about that ? The fact that we added safeguards is one thing, but mentioning the UI and the "Manage display" screens in the PHPdocs IMO makes the intent & impact of the methods much clearer than just saying "configurable".

3. See #52.

4. Yup, There are existing issues for EntityDisplay config schemas, but they are blocked by #1875974: Abstract 'component type' specific code out of EntityDisplay, which itself is blocked by this issue.

6. Added.

Will update the issue summary tomorrow.

yched’s picture

Issue tags: +Entity Field API

Missing tag

effulgentsia’s picture

How about this as a way to connect the dots to the UI without implying that it's only about the UI?

I'm fine with either #95 or this though. It would be ideal to get a final +1 from fago, and also one from swentel. But if they're not available in the next day or two, I'm fine RTBC'ing this without that.

yched’s picture

Issue summary: View changes

#97 works for me

Updated the issue summary.

yched’s picture

Issue summary: View changes
yched’s picture

Issue summary: View changes
yched’s picture

Issue summary: View changes
yched’s picture

Issue summary: View changes
yched’s picture

Issue summary: View changes
yched’s picture

And removing tag - sorry for the spam, I think I'm done.

yched’s picture

Assigned: yched » Unassigned
effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Alrighty. Let's do this :)

Wim Leers’s picture

Nice work! :)

yched’s picture

Reroll, just in case.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 108: base_fields_in_field_ui-2144919-108.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Green, so back to RTBC.

effulgentsia’s picture

Issue tags: +beta target

Adding beta target tag, because it's a (depending on POV, either soft or hard) blocker for #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).

andyceo’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 108: base_fields_in_field_ui-2144919-108.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
68.27 KB

Just a reroll. Straight to RTBC if green.

webchick’s picture

Title: Allow widgets and formatters for base fields to be configured in Field UI » Change notice: Allow widgets and formatters for base fields to be configured in Field UI
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Awesome! Alex Bronstein walked me through this patch for about 90 minutes, and everything looks good to me.

Glad to have this in, not only from the standpoint of unificating all the things. ;) But also to unblock progress on #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).

The only outstanding thing that was odd to me is the old school Title UI on the content type edit page sticks around after this patch. But it sounds like removing this is blocked on #2114707: Allow per-bundle overrides of field definitions.

Therefore!

Committed and pushed to 8.x. Great work!

Tagging for change notice.

yched’s picture

w00t ! After months of work, widgets and formatters on base fields are official :-)
Thanks to everyone who helped, and major kudos to @effulgentsia for driving that effort.

Now we "just" need to actually leverage this for more base fields in core...
See you all in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) !

I think it would make sense to add this to the change record about FieldDefinitions for base fields, but I can't seem to find it ?

yched’s picture

The only outstanding thing that was odd to me is the old school Title UI on the content type edit page sticks around after this patch

Thing is, this UI also lets you pick a custom "label" for the title field, which is orthogonal to and not deprecated by this patch.

effulgentsia’s picture

I think it would make sense to add this to the change record about FieldDefinitions for base fields, but I can't seem to find it ?

See #2112239-42: Convert base field and property definitions and below.

effulgentsia’s picture

Thing is, this UI also lets you pick a custom "label" for the title field, which is orthogonal to and not deprecated by this patch.

Right. But see #2114707-5: Allow per-bundle overrides of field definitions. Looks like that issue has gone a different direction though, and I don't think we should hold that issue up to revisit that, but perhaps once that issue is in, we can have a followup to reevaluate the UI of configuring per-node-type title settings.

xjm’s picture

Issue tags: +Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

Wim Leers’s picture

Wow, I didn't realize this went in! HURRAY! :) Now we can finally truly make it possible to in-place edit ALL the things :)

yched’s picture

Assigned: Unassigned » yched

Got a change notice draft lying around, I just need to actually finish it...

yched’s picture

Title: Change notice: Allow widgets and formatters for base fields to be configured in Field UI » Allow widgets and formatters for base fields to be configured in Field UI
Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

Change record added at https://drupal.org/node/2186583

andypost’s picture

Follow-up #2191555: Allow DisplayOverviewBase use all field types to fix bugs in introduced tests

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.