Correctly determine when to display fields as inline

Confirm that this patch together with manage_display 8.x-1.0-alpha6 is working for me

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -66,10 +66,11 @@ public function buildTitle(array $page) {
   -    // correctly (e.g. RDFa, in-place editing).
   +    // correctly (e.g. in-place editing).
   

   Unintended change?

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -66,10 +66,11 @@ public function buildTitle(array $page) {
   +        $page[$label_field]['#is_page_title'] = TRUE;
   

   I'm not a huge fan of new undocumented magic array keys

3. +++ b/core/modules/node/node.module
   @@ -571,6 +573,23 @@ function node_preprocess_block(&$variables) {
   +    $variables['inline_field'] = !$variables['element']['#object']->getEntityType()->get('node_basefield_skip_preprocess');
   

   this relies on elements from the other issue? I don't think we can add this without that

4. +++ b/core/modules/node/node.module
   @@ -609,16 +640,25 @@ function template_preprocess_node(&$variables) {
   +  $preprocess = !$node->getEntityType()->get('node_basefield_skip_preprocess');
   +  if ($preprocess) {
   +    // Make created, uid and title fields available separately.
   

   this looks like the code from #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI?

5. +++ b/core/themes/stable/templates/field/field--node--created.html.twig
   @@ -15,12 +15,20 @@
   +{% if not inline_field %}
   
   +++ b/core/themes/stable/templates/field/field--node--title.html.twig
   @@ -15,12 +15,20 @@
   +{% if not inline_field %}
   
   +++ b/core/themes/stable/templates/field/field--node--uid.html.twig
   @@ -15,12 +15,20 @@
   +{% if not inline_field %}
   

   are we allowed to make changes to markup in stable?

Yes, this depends on #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI.

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -66,10 +66,11 @@ public function buildTitle(array $page) {
+        $page[$label_field]['#is_page_title'] = TRUE;

I'm not a huge fan of new undocumented magic array keys

Looks like @AdamPS introduced this in #12 to make it "easier for contib modules to control the formatting within the page title". Let's hope he can explain the need.

I believe postponed has to be postponed on something; this should be closed, and someone can reopen if they disagree.

Actually, my bad, if what you're proposing is to leave this to 9.x, then it should be an active issue on that branch I think. Same done for #2993642: Mechanism to disable preprocessing of base fields in taxonomy and aggregator entity types so they can be configured via the field UI.

I have tried the patch and I can confirm it works as expected. I wanted to show the created date of the node with a label and with the patch applied I only had to set the field as configurable in the view mode.

I'd change it to RTBC but I think it might need a change record for people who are setting those fields as configurable and don't expect the markup to change.

Please review the change record
https://www.drupal.org/node/3043840

The code looks good to me.

The patch is failing tests.

Random fail

Tagging for front end framework manager review because of the change from span to div for those fields

The current approach has some side effects that could be disruptive to themes as well as theming experience. Removing these items from the theme registry seems to completely prevent users from overriding templates using the suggestion matching the theme registry item. So for example, after field__node__created has been removed from theme registry, themes are unable to provide field--node--created.html.twig template. This is also not something we can or should fix because after it gets fixed, this solution also becomes ineffective because Classy and Stable both provide templates using more specific theme suggestions for this template.

I think the approach used earlier on this issues (#22) would be less disruptive. Even though markup changes to Stable and Classy are not allowed, that would be a less disruptive approach than what has been taken currently.

Looks good to me.

I like the universality of this approach. The calling context knows the surrounding markup, so it's got responsibility for informing the template about it. This pattern could well be useful beyond these few node fields.

+++ b/core/modules/node/templates/field--node--title.html.twig
@@ -15,14 +15,23 @@
+ * - inline_field: If false, display an ordinary field.
+ *   If true, display an inline format, suitable for inside span/h1/h2 etc
...
+

Wouldn't is_inline_field be clearer? Or even is_inline, so that it eventually could be generalized for use in non-Entity Field situations?

+1 for is_inline

Thanks for responding so quickly, @jonathanshaw :)

Since you previously RTBC'd this patch and now agree with my suggestion, I'm gonna mark this Needs work.

An easy fix.

Excellent, thanks! I wanted to re-RTBC, but in my final review I found some more problems:

1. +++ b/core/modules/node/node.module
   @@ -581,6 +583,25 @@ function node_preprocess_block(&$variables) {
   +  elseif (in_array($variables['field_name'], ['created', 'uid', 'title'])) {
   

   Nit: let's use that third in_array() parameter for strict comparison. We do that always.

2. +++ b/core/modules/node/node.module
   @@ -581,6 +583,25 @@ function node_preprocess_block(&$variables) {
   +    // @todo For back-compatibility, delete in D9. See
   +    // https://www.drupal.org/node/3015623
   +    $node = $variables['element']['#object'];
   +    $skip_custom_preprocessing = $node->getEntityType()->get('enable_base_field_custom_preprocess_skipping');
   +    $variables['is_inline'] = !($node->getFieldDefinition($variables['field_name'])->isDisplayConfigurable('view') && $skip_custom_preprocessing);
   
   +++ b/core/modules/node/templates/field--node--created.html.twig
   @@ -15,14 +15,23 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/modules/node/templates/field--node--title.html.twig
   @@ -15,14 +15,23 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/modules/node/templates/field--node--uid.html.twig
   @@ -15,14 +15,23 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/themes/classy/templates/field/field--node--created.html.twig
   @@ -15,10 +15,17 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/themes/classy/templates/field/field--node--title.html.twig
   @@ -15,10 +15,17 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/themes/classy/templates/field/field--node--uid.html.twig
   @@ -15,10 +15,17 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/themes/stable/templates/field/field--node--created.html.twig
   @@ -15,12 +15,20 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/themes/stable/templates/field/field--node--title.html.twig
   @@ -15,12 +15,20 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   
   +++ b/core/themes/stable/templates/field/field--node--uid.html.twig
   @@ -15,12 +15,20 @@
   + * @todo Delete in D9, see https://www.drupal.org/node/3015623
   

   There's some shorthand notations ("back-compatibility" and "d9") in this comment that we generally don't use :)

   More importantly: if we're going to remove this, shouldn't we be deprecating it in this issue?

3. +++ b/core/modules/node/templates/field--node--title.html.twig
   @@ -15,14 +15,23 @@
   + *   If true, display an inline format, suitable for inside span/h1/h2 etc
   

   for inside span/h1/h2 etc is not as precise as we tend to write comments. How about: for inline elements such as <span>, <h2>, and so on.?

More importantly: if we're going to remove this, shouldn't we be deprecating it in this issue?

That makes sense. Unfortunately https://www.drupal.org/core/deprecation does not explain how to deprecate a template. Any ideas?

+    // @todo For back-compatibility, delete in D9. See
+    // https://www.drupal.org/node/3015623
...
+    $skip_custom_preprocessing = $node->getEntityType()->get('enable_base_field_custom_preprocess_skipping');

This flag was introduced in a prior issue; I'd suggest the question of how it should be deprecated is out of scope for this issue?

#58 Thanks for that background information, I was not aware. Thanks for pushing this along! 🙏

I do like the current approach more! Thank you!

1. +++ b/core/modules/node/node.module
   @@ -581,6 +583,24 @@ function node_preprocess_block(&$variables) {
   +/**
   + * Implements hook_preprocess_HOOK() for node field templates.
   + */
   +function node_preprocess_field__node(&$variables) {
   

   Maybe this would be a good place to briefly document what is is_inline including intentions behind introducing it. And for better discoverability, we could maybe add @see documentation to templates.

2. +++ b/core/modules/node/node.module
   @@ -581,6 +583,24 @@ function node_preprocess_block(&$variables) {
   +    // Page title is always inline.
   

   Could we include an explanation on why the title must be always inline?

3. I have similar concern as #56.2. Making this change would likely not be enough to allow those templates to be removed since users aren't being notified that there's something expected from them. Could we open an issue to discuss how to deprecate templates? We could then add @todo comments to properly deprecate the templates once that issue has been solved.

Maybe good feedback, but at Dropsolid we are waiting anxiously on these commits to happen. Ideally before 9 gets released.

Maybe @xjm wanted the frontend framework manager review for #61.3, but that doesn't directly affect this patch.

I'm tagging it for frontend framework manager review to validate that the improvements described in #62 and implemented in #63 address his feedback. This request for his signoff is part of trying to get this issue committed. Thanks!

1. We need to update these changes to stable9 too

2. +++ b/core/modules/node/templates/field--node--title.html.twig
   @@ -15,14 +15,25 @@
   + * @todo Delete as part of https://www.drupal.org/node/3015623
   

   Maybe I'm missing something but how are we going to be able to remove the inline template for titles?

3. +++ b/core/themes/stable/templates/field/field--node--uid.html.twig
   @@ -15,12 +15,22 @@
   + * @todo Delete as part of https://www.drupal.org/node/3015623
   

   I still had the concern that we should deprecate these templates for us to be able to delete them eventually. Maybe we could use the Twig deprecated tag for this https://twig.symfony.com/doc/2.x/tags/deprecated.html. I think it would be fine to do this in a follow-up that would happen between this issue and #3015623: [PP-many] Remove outdated code relating to "Expose Title in Manage Display". Instead of marking these to be deleted as part of that issue, let's reference the new follow-up.

4. +++ b/core/modules/node/node.module
   @@ -581,6 +583,28 @@ function node_preprocess_block(&$variables) {
   +    $variables['is_inline'] = TRUE;
   ...
   +    $variables['is_inline'] = !($node->getFieldDefinition($variables['field_name'])->isDisplayConfigurable('view') && $skip_custom_preprocessing);
   

   I assume we will have to remove the is_inline variable eventually too. Let's open another follow-up to figure out how to deprecate that.

Fixed failed tests of patch #82.

Back to @laurii

I RTBC'd in #64, and that still stands, but no one's going to commit without frontend framework manager sign off.

Random

1. +++ b/core/modules/node/node.module
   @@ -431,6 +433,28 @@ function node_preprocess_block(&$variables) {
   +  if (isset($variables['element']['#is_page_title'])) {
   

   This should test for truthiness too. $variables['element']['#is_page_title'] = FALSE will pass this check too which would be wrong.

   I think:

   if ($variables['element']['#is_page_title'] ?? FALSE) {
   

   would work.

2. +++ b/core/modules/node/templates/field--node--created.html.twig
   @@ -15,14 +15,25 @@
   +{% if not is_inline %}
   +  {% include "field.html.twig" %}
   +{% else %}
    <span{{ attributes }}>
      {%- for item in items -%}
        {{ item.content }}
      {%- endfor -%}
    </span>
   +{% endif %}
   

   This is making me wonder if field.html.twig should support is_inline too.

3. +++ b/core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
   @@ -62,9 +62,10 @@ public function testDisplayConfigurable() {
   -    $assert->elementTextContains('css', 'span.field--name-created', \Drupal::service('date.formatter')->format($node->getCreatedTime()));
   -    $assert->elementTextContains('css', 'span.field--name-uid[data-quickedit-field-id="node/1/uid/en/full"]', $user->getAccountName());
   -    $assert->elementNotExists('css', 'span.field--name-uid a');
   +    $assert->elementTextContains('css', 'div.field--name-created', \Drupal::service('date.formatter')->format($node->getCreatedTime()));
   +    $assert->elementTextContains('css', 'div.field--name-uid[data-quickedit-field-id="node/1/uid/en/full"] .field__item[rel="schema:author"]', $user->getAccountName());
   +    $assert->elementNotExists('css', 'div.field--name-uid a');
   +    $assert->elementTextContains('css', 'div.field--name-uid div.field__label', 'Authored by');
   

   This is the sum total of the test coverage introduced here. The good thing is that the test contains the opposite assertions here proving that BC is maintained.

I'm leaving at rtbc because the change required to fix the if is very small.

1. +++ b/core/modules/node/templates/field--node--title.html.twig
   --- a/core/modules/node/templates/field--node--uid.html.twig
   +++ b/core/modules/node/templates/field--node--uid.html.twig
   
   +++ b/core/profiles/demo_umami/themes/umami/templates/classy/field/field--node--created.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/profiles/demo_umami/themes/umami/templates/classy/field/field--node--title.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/profiles/demo_umami/themes/umami/templates/classy/field/field--node--uid.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/themes/bartik/templates/classy/field/field--node--created.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/themes/bartik/templates/classy/field/field--node--title.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/themes/bartik/templates/classy/field/field--node--uid.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/themes/claro/templates/classy/field/field--node--created.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/themes/claro/templates/classy/field/field--node--title.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   
   +++ b/core/themes/seven/templates/classy/field/field--node--created.html.twig
   @@ -15,10 +15,19 @@
   +  {% include "field.html.twig" %}
   

   Discussed with @lauriii. In light of #3144443: Phase out Drupal\Core\Template\Loader\ThemeRegistryLoader I think we should use something like

    {% include "@seven/templates/classy/field/field.html.twig" %}</code
   </li>
   
   <li>
   <code>
   +++ b/core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
   @@ -62,9 +62,10 @@ public function testDisplayConfigurable() {
   -    $assert->elementTextContains('css', 'span.field--name-created', \Drupal::service('date.formatter')->format($node->getCreatedTime()));
   -    $assert->elementTextContains('css', 'span.field--name-uid[data-quickedit-field-id="node/1/uid/en/full"]', $user->getAccountName());
   -    $assert->elementNotExists('css', 'span.field--name-uid a');
   +    $assert->elementTextContains('css', 'div.field--name-created', \Drupal::service('date.formatter')->format($node->getCreatedTime()));
   +    $assert->elementTextContains('css', 'div.field--name-uid[data-quickedit-field-id="node/1/uid/en/full"] .field__item[rel="schema:author"]', $user->getAccountName());
   +    $assert->elementNotExists('css', 'div.field--name-uid a');
   +    $assert->elementTextContains('css', 'div.field--name-uid div.field__label', 'Authored by');
   

   I think we need to change this test to test all the themes change here so we have test coverage of all the changes.

Discussed #99.1 with @alexpott in more detail and we thought that even though #3144443: Phase out Drupal\Core\Template\Loader\ThemeRegistryLoader is happening, we still prefer using the Drupal\Core\Template\Loader\ThemeRegistryLoader here because it benefits contrib theme authors without too much of a downside. Most of the downsides listed on that issue are specific to using Drupal\Core\Template\Loader\ThemeRegistryLoader with the Twig extend or embed. That issue will likely end up providing an alternative, more explicit way of loading templates from the theme registry.

We also briefly discussed the benefits of the current approach over the approach taken in #34. We agreed that the current approach is better because it makes this behavior explicit for front end developers. It also makes sure that any pre-existing overrides don't break as a result of setting enable_base_field_custom_preprocess_skipping to true.

Patch attached addresses #98 and #99

But leaves one more thing to do which is to fix the test to test Olivero and probably to fix the Olivero theme.

Fixing the deprecation.

Hello,
It is nice we are trying to fix this, but to me this seems like the wrong approach:

• Lots of repetition (already the case now, we are adding to it).
• Logic in the template, that should be elsewhere.
• Choosing the template based on the field name seems like generally the wrong solution to the problem.

Alternative: field--inline.html.twig

Instead we should simply have a field--inline.html.twig and a regular field.html.twig, and then make the distinction on render element level or on theme hook suggestion level.

Possible problems and challenges with this idea:

• Backwards compatibility: How can we make sure themes, modules or custom hacks that rely on e.g. field--node--title.html.twig still work?
• Name clash: What if an entity type, bundle or field is named 'inline', would this accidentally trigger the 'field--inline.html.twig' template? We could avoid this by naming this template differently, perhaps even prefix it with something and then artificially set 'field' as the base theme hook.

@AdamPS Thanks for the explanation!

The strategy chosen here has now been reviewed by many key members of the Drupal.org community and they have collectively agreed it is the correct approach.

If this is indeed the best or only BC-friendly solution, then I am ok with it for D9.

So in D10 when we can remove the deprecated code we will have the perfect solution.

Where would I learn about the "perfect" solution in D10? In #2353867: [META] Expose Title and other base fields in Manage Display?
So in D10, the title field would be rendered like any other field using the regular field template? So the markup would be something like <div class="field ..."><h2>...</h2></div>, where the <h2> would be coming from the field formatter? Or is this not fully planned yet?

I think there are other use cases where it would be useful to switch the field template independently from the field name and the formatter. Currently this can be done in contrib, e.g. using Display suite field templates. But any such contrib solution will need to incorporate or work around the theme hook suggestion mechanism. E.g. any contrib module that wants to introduce a field--inline.html.twig will need to consider the case where "inline" actually matches an existing theme hook suggestion. Currently this could only happen if there is either a field type or an entity type named "inline" (field names are typically prefixed), which seems unlikely. But the current suggestion mechanism does limit the namespace that such contrib modules could use. Perhaps we need to rethink the suggestion mechanism for D10.

But I guess we should keep this issue strictly about D9, and discuss anything related to D10 elsewhere.

Some code review for #103.

+function node_preprocess_field__node(&$variables) {

Is the '__node' reliable?
I think it will match or fail depending on the field template the theme uses.
It will fail on "field__{$field_type}" or "field__{$field_name}", but it will match "field", "field__{$entity_type}", "field__{$entity_type}__{$bundle}", "field__{$entity_type}__{$field_name}" and "field__{$entity_type}__{$field_name}__{$bundle}". At least this is what I remember from last time I checked how this mechanism works.

+    $node = $variables['element']['#object'];

We should add /** @var \Drupal\node\NodeInterface $node */ in the line above, to allow static code analysis by the IDE.

+    $variables['is_inline'] = !($node->getFieldDefinition($variables['field_name'])->isDisplayConfigurable('view') && $skip_custom_preprocessing);

For my taste, there is too much happening in one line here.
But then I looked at existing places where we do this as an if (..) condition, they also have this check in one line.

However, we can eliminate the parentheses and replace && with ||, this also makes us more consistent with other places in code. And swap the two sides of the ||, like so:

$variables['is_inline'] = !$skip_custom_preprocessing || !$node->getFieldDefinition($variables['field_name'])->isDisplayConfigurable('view');

---------

Besides this, I wonder, is preprocess the right place to do this?
Perhaps it is. But for contrib, should we provide ways to control this behavior from the render element? E.g. to allow formatters to set an '#is_inline' flag?

The two distinct settings 'display configurable' and 'enable_base_field_custom_preprocess_skipping' seem awkward to me. Since both are boolean, there are 4 different combinations/scenarios per field. I wonder if each of those 4 are meaningful choices, and whether we have a well-designed "expected behavior" for all of those 4 combos.
This was already introduced in #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI, so there is no point to change it now. But we should at least document what we expect to happen in each of those cases.

Besides this, I wonder, is preprocess the right place to do this?

Some observations

With current code and this patch, I found the following places where we are going to check for 'enable_base_field_custom_preprocess_skipping' and ->isDisplayConfigurable():

• hook_entity_extra_field_info(), introduced in #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI:
  • aggregator_entity_extra_field_info(), for 'aggregator_feed:image', 'aggregator_feed:description', 'aggregator_item:description'.
• Entity preprocess hook, introduced in #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI:
  • template_preprocess_aggregator_item(), setting 'title' from $entity->label().
  • template_preprocess_aggregator_feed(), setting 'title' from $entity->label().
  • template_preprocess_node(), for 'date', 'author_name', 'title', each from $variables['elements'][$field_name].
  • template_preprocess_taxonomy_term(), setting 'name' from $variables['elements']['name'].
• Field preprocess hook, proposed in this issue, for now only for node fields, but possible follow-up for other entity types?
  • node_preprocess_field__node(), setting 'is_inline' for 'created', 'uid', 'title'.

  This made me wonder: Why don't we have a field template override for taxonomy term name?
  I installed D9 with umami, set up a view mode for terms, and found this html in a term view mode:

  <h2><a .. href=".."><div class="field ...">Accompaniments</div></a></h2>. So we are not changing term names to "inline" currently.
  
  Also interesting to see how some fields use <code>$entity->label()

  , instead of using the field.

  Proposal

  It seems to me that:

  • Whatever is in $variables['label'] should render as inline, either using <span class="field ...">, or no wrappers at all.
  • Whatever remains in $variables['elements']['title'] should have the regular field wrapper markup with <div class="field ...">.

  Therefore, could we set the 'is_inline' in template_preprocess_node()?
  Like so:

    if (isset($variables['elements']['title']) && (!$skip_custom_preprocessing || !$node->getFieldDefinition('title')->isDisplayConfigurable('view'))) {
      $variables['label'] = $variables['elements']['title'];
      // Trigger the inline display in the field template.
      // Respect contrib modules that set '#is_inline' explicitly.
      $variables['label'] += ['#is_inline' => TRUE];
      unset($variables['elements']['title']);
    }
  

  We could then add 'is_inline' to the variables in the 'field' theme hook, so it will be added to the $variables array.
  Also, by doing this, we perhaps no longer need the '#is_page_title'.

  As a next step, we could move this entire logic from the entity preprocess phase to hook_entity_view_alter(), so it would happen on render element level. We could also allow a setting in the view mode render element, that could be set in contrib, and that would control this behavior per view mode.

  I don't know yet if any of these ideas would be problematic for BC.

So you have applied "De Morgan's laws". I don't believe that there is a universally agreed view that this makes things simpler. In this case, the logic I wrote exactly corresponds to the comment above so I think it's clearer to stay as it is.

Yes, I forgot how it is called, too long ago :)
I cannot point to a specific Drupal convention, but I do find that comparable places in code use a negation normal form.
(I just looked this up, I am not trying to prove any authority on the matter)

Here is the result of a search:

~/projects/d8/www $ ag if.*skip_custom_preprocessing core/
core/modules/node/node.module
494:  if (!$skip_custom_preprocessing || !$submitted_configurable) {
501:  if (isset($variables['elements']['title']) && (!$skip_custom_preprocessing || !$node->getFieldDefinition('title')->isDisplayConfigurable('view'))) {

core/modules/aggregator/aggregator.theme.inc
44:  if (!$skip_custom_preprocessing || !$item->getFieldDefinition('title')->isDisplayConfigurable('view')) {
81:  if (!$skip_custom_preprocessing || !$feed->getFieldDefinition('title')->isDisplayConfigurable('view')) {

core/modules/aggregator/aggregator.module
120:  if (!$skip_custom_preprocessing || !$base_field_definitions['image']->isDisplayConfigurable('view')) {
128:  if (!$skip_custom_preprocessing || !$base_field_definitions['description']->isDisplayConfigurable('view')) {
144:  if (!$skip_custom_preprocessing || !$base_field_definitions['description']->isDisplayConfigurable('view')) {

core/modules/taxonomy/taxonomy.module
154:  if (!$skip_custom_preprocessing || !$term->getFieldDefinition('name')->isDisplayConfigurable('view')) {

There is also a performance argument to be made: evaluating the variable should be faster than doing the method call, so it would make sense to put the variable first.

+1 to the approach here. Looks really good.

+++ b/core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
@@ -35,17 +61,14 @@ public function testDisplayConfigurable() {
-    $assert->elementTextContains('css', 'span.field--name-created', \Drupal::service('date.formatter')->format($node->getCreatedTime()));
-    $assert->elementTextContains('css', 'span.field--name-uid[data-quickedit-field-id="node/1/uid/en/full"]', $user->getAccountName());
-    $assert->elementTextContains('css', 'div.node__submitted', 'Submitted by');
-    $assert->elementTextContains('css', 'span.field--name-title', $node->getTitle());

It looks like the assertions that replaced these removed the CSS classes (e.g., .field--name-created) from the selectors. Is that because some of the themes don't include those CSS classes? Do we want to add additional assertions for the themes that do? Otherwise, seems like we might introduce a regression that removes the classes without a test that catches that.

One could certainly argue that asserting for these classes isn't really part of the scope of this test, but I don't think we have any other test that does it. Bonus points if we figure out a better test (or create a new one) to put that into.

Nice

Thank you for the test changes. Adding issue credits for reviewers.

Pushed to 9.3.x, published the CR, and added a release note snippet to the issue summary (feel free to edit that snippet as needed) for when it's time to make release notes for 9.3.0.

Thank you everyone for all your work and patience on this.

Congratulations! Thank you to all.

Looks CR is wrong about inline_field usage in template, it should be is_inline

polished CR