Title formatting broken due to flawed EntityViewController->buildTitle

Worth noting that the current approach was introduced for performance reasons in #2498849: Entity view controller title rendering is expensive.

Discard the existing behaviour that field templates affect the page title. The page-title.html.twig template is the logical place to govern that giving consistent formatting across the site.

This seems like the fundamentally correct solution. The way page titles are currently built is just too different from how field displays work for them to automatically share templates. I imagine that the output of buildTitle() is also used in the browser's window title too (the HTML head section)? A very different use to the page title block.

It also feels like there should be a page-title--node etc. template suggestions in the system somewhere, to allow overriding of page titles on a entity-by-entity basis.

How about doing stopping the field template affecting the title only if the field display is configurable? This might help BC, by mostly retaining existing behavior, while everyone gradually transitions to a bright configurable future.

I think there's a bigger picture that I'm not sure we're yet addressing. Or I'm still getting my head around. It seems to me that the fundamental issues here are:

1. there is a #title variable in the page render array, which controls much more than the context of the page title block. It also controls the window title, and is a fundamental piece of metadata
2. like any good controller, the EntityViewController populates the page's #title property. It happens to do this using a prerender callback with its buildTitle method. It uses the entity's label field.
3. there is a page title block, which for convenience and universality gets the title to show from the #title property
4. entity's have a default view display; this is used OOTB as the fallback when any view mode, including 'full' is not specifically configured. Therefore if the label field is configurable and enabled on it then there is a potential for duplication in the full mode which is not a good OOTB default, and if it is configurable and not enabled then labels will be missing, which in non-full contexts is not a good OOTB default.
5. this problem (4) is currently worked-around by having #printed set on the label field in the EntityViewController's buildTitle method,
   which suppresses further output as part of {{ content }}, and this only takes effect in the full view mode as only then is the EntityViewController in use.

The problem (4) is hard and important.

The current solution (5) is a nasty hack. Most obviously it suppresses the label on full entity pages even when the page title block is not in use. More fundamentally it links the building of the page's title property with the display of the label in the body of the page, and these are things that should be completely independent because the page title property is a more akin to metadata than a part of the entity display.

The conclusion I draw from this is that it is just wrong for the buildTitle method to be calling render and setting #printed. It should use a less invasive approach to preparing a title from the entity's label, an approach that respects the fact that it may just be gathering a piece of metadata and not necessarily preparing the principal rendering of the label.

Instead I suggest that it is the page title block that should be responsible for suppressing default later display of the label if it has taken over label display from the main entity display.

The best solution might be to have buildTitle not do any rendering, and leave the rendering up to the page title block.

An alternative would be to have buildTitle not set #printed, but add a #title_fields property to the #title or #page, holding the machine names of entity fields used in preparing the title. Then the page title block could unset (or set to #printed) these fields from the page if it is choosing to take over printing the #title. This also puts the responsibility in the correct place.

I'm not sure how adding a hook really helps here. If modules want to override they already can, by swapping in an alternative EntityViewController. Isn't the problem we're facing is not how to override this but how to change the default in a BC way.

Bottom line: it's really hard to reconcile configurable titles with default view modes.

And that's even before all the other related wrinkles you've found.

Sorry Adam, I had somehow missed the meat of your IS proposal. I think your way of dividing the problem up makes basic sense.

We already have a simple hook that solves problem 2 though: you can undo #printed in an entity preprocess. Simpler than swapping in a new controller. So we have workarounds already, I see the question as being whether there is any way to avoid the very need for them. I'd be surprised if there was much appetite for adding yet another workaround.

With regard to 1 and 3, I agree that a different output mechanism is needed in buildTitle, something that is independent of the entity's display configuration. I think it's fairly safe to assume the label is a string, although someone out there probably has the label key pointing to an entity reference field.

I agree with you on the BC concerns. BuildTitle ideally needs to stop setting #printed, but some people will be unknowingly relying on this. People not using the supplied page title block are my biggest concern because they get left out if we transfer responsibilities to it.

Re-rolled the patch from #22 against the latest codebase.

Adam asked me if I could review this patch; I can't because I'm struggling to envisage a satisfactory D9 future for sitebuilder-managed title fields and that's getting in the way of me engaging with this particular issue; Adam asked me to try to express my concerns here.

The background we all know is:

- OOTB, entities have only the 'default' pseudo-view-mode
- OOTB, entities need to provide a reasonable view of themselves (including their title) when rendered in non-full-page contexts, AND a view of themselves on their canonical full-page route that does not have a duplicate title (one in the page title block and one in the content block).
- Therefore OOTB we need to do some fancy work to show/hide the title depending on which context we're in.

D8 handles this by excluding the title field from the view mode UI. It's handled automatically behind the scenes. This keeps things relatively simple, but prevents sitebuilders from using the view mode UI to configure the title in view modes, which is a very normal thing for them to want to do.

Therefore in D9 we want the title to be manageable in the view mode UI like any other field. Which raises the question of how to reconcile the title configured in the view mode with the title in the page title block.

We want all these things:
1) No duplicate title OOTB in full-page route
2) (Maybe) Title formatter on full-page view mode is controlled by view mode UI
3) Presence & position of title amidst other fields on non-full-page view mode is controlled by view mode UI
4) Title formatter on non-full-page view mode is controlled by view mode UI

And they're very hard to reconcile.

What I dislike about the direction Adam is going in, if I understand it, is that OOTB in D9 we would have the title floating in the default view mode, but the positioning of the title amidst other fields is ignored. Probably what formatter you choose will effect the page title block, but otherwise you can drag the title around in the default view mode and it effects the entity rendering in all contexts except the full page.

This feels like weird magic that increases code complexity and obstructs developers and sitebuilders developing a mental model of how view modes work.

But I don't have a better solution. I seriously wonder if in D9 we should force entities to have explicit 'full' and 'title' view modes, so we're not relying on magic to handle all this.

The reason I didn't say all this until Adam asked me to is that I'm not sure it's constructive and it's also likely out of scope. But i find myself unable to evaluate the proposals in this issue without a satisfactory overall direction of travel for D9.

@AdamPS has explored this problem space very thoroughly, and it tested in contrib in the 'Manage display' module.

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -70,14 +70,32 @@ public static function create(ContainerInterface $container) {
+      if ($template_enabled && $entity->getFieldDefinition($label_field)->isDisplayConfigurable('view')) {

I think we need to handle the case where there is no label field (yes, it can happen in contrib/custom)

E.g. I have client code where the label is derived from other fields (implemented in ::label()) method so there is no label field.

Just a quick code review, I still need to have a closer look.

+++ b/core/modules/quickedit/quickedit.module
@@ -125,6 +125,20 @@ function quickedit_preprocess_page_title(&$variables) {
+  if (!\Drupal::currentUser()->hasPermission('access in-place editing') || !$entity->isLatestRevision()) {

This is missing a $entity instanceof RevisionableInterface check.

Thanks for the thorough issue summary and careful planning, I think we are very close here!

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -70,14 +70,32 @@ public static function create(ContainerInterface $container) {
   +        // Prevent output of the label field in the main content.
   +        unset($page[$label_field]);
   

   I think we should do $page[$label_field]['#access'] = FALSE here for two reasons:

   • if the enable_page_title_template flag is set and the enable_base_field_custom_preprocess_skipping is not, the following notice is raised for nodes:
     

     Notice: Undefined index: title in template_preprocess_node() (line 648 of core/modules/node/node.module).
     

   • If the field needs to be displayed later in the process by some custom/contrib code it's easier to restore it, otherwise the field render array needs to be built again.

2. +++ b/core/modules/node/tests/modules/node_display_configurable_test/node_display_configurable_test.module
   @@ -25,4 +25,5 @@ function node_display_configurable_test_entity_base_field_info_alter(&$base_fiel
    function node_display_configurable_test_entity_type_build(array &$entity_types) {
      // Allow skipping of extra preprocessing for configurable display.
      $entity_types['node']->set('enable_base_field_custom_preprocess_skipping', TRUE);
   +  $entity_types['node']->set('enable_page_title_template', TRUE);
   

   Test coverage for nodes looks good, but since we are introducing a feature that any entity can leverage, we should have it tested for all supported entity types. For instance, content translation has a base test class that all supported entity types can extend with little effort. It would be valuable to do something like this here, especially because we are special casing node and user entity types.

3. +++ b/core/modules/quickedit/quickedit.module
   @@ -125,6 +125,20 @@ function quickedit_preprocess_page_title(&$variables) {
   +  if (!\Drupal::currentUser()->hasPermission('access in-place editing') || !$entity->isLatestRevision()) {
   

   As mentioned in #53, this is missing an $entity instanceof RevisionableInterface check.

This will also require a change record.

I also struggle to understand how and why to implement #54.2

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -71,14 +71,32 @@ public static function create(ContainerInterface $container) {
   +        // Prevent output of the label field in the main content.
   +        $page[$label_field]['#access'] = FALSE;
   

   I'm not sure I agree we should be hiding the label field in the main content. This creates confusing behavior because site builders could see the label in field UI but it doesn't render on the page. Shouldn't it be up to the site builder to configure this?

2. What is the plan to address the problems related missing label caused by field--node--title.html.twig template override? Should we at least open a follow-up to try to solve that?

3. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -71,14 +71,32 @@ public static function create(ContainerInterface $container) {
   +        // Set page title to the output from the entity_page_title template.
   

   I would love if we could add an explanation of why we are doing this as well.

@lauriii "I'm not sure I agree we should be hiding the label field in the main content. This creates confusing behavior because site builders could see the label in field UI but it doesn't render on the page."

I share these reservations, but Adam has repeatedly pointed out to me that if we don't do this then you get a duplicate title on the page (title block & content block) which is arguably equally confusing.

If you're sure this concern blocks commit, could you suggest a process where as a community we could figure out how to solve this UI problem? Our current paradigms don't seem to offer a neat answer.

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

This is failing on 9.1.x:

https://www.drupal.org/pift-ci-job/1672842

We need a different version of the patch for D9. Thanks!

Added patch for 9.1 branch

Trying to fix failed test

Fixing this:

1) Drupal\KernelTests\Core\Theme\StableDecoupledTest::testDecoupledStable with data set "claro" ('core/themes/claro', array('block.admin.css', 'filter.caption.css', 'progress.module.css', 'status-report.html.twig', 'system-modules-details.html.twig', 'system-modules-uninstall.html.twig'))
Failed asserting that an array is empty.

@adamps,

Because method testDecoupledStable confirms that theme assets are decoupled from stable and we have added new template entity-page-title.html.twig in the stable which differs from core themes, this test case is failing.

In #74 the solution I proposed was of adding the new included theme in the to-skip array (the asset filenames to be ignored). Because another option that I can see of passing this test is of adding a template in core themes which in my view is not correct.

See https://www.drupal.org/project/drupal/issues/3115223. This issue states that

Stable will be moved to contrib during Drupal 9. After we have ensured that core themes (Bartik, Claro, Seven, Umami) are not using Stable templates or libraries, we can remove Stable as a base theme of these themes.

If so then we can remove/modify this test?

@sja112 do you think #72 is right? It didn't fix the test in the way it intended to.

@jonathanshaw the only test case failing in #72 is Drupal\KernelTests\Core\Theme\StableDecoupledTest::testDecoupledStable because of the newly added template in stable.

After further investigating this issue I found the solution of fixing this test either by adding the new template in the core themes (Bartik, Claro, Seven, Umami) or adding it in the to-skip (the asset filenames to be ignored) array.

I have added the patch including the later solution in #74.

Also like I have mentioned in #76 as per the logged issue #3115223 stable will be moved to contrib during Drupal9. After we have ensured that core themes (Bartik, Claro, Seven, Umami) are not using Stable templates. If so then can we remove/modify this test?

To move forward I want to know the right approach we should take to fix the issue.
Does anyone in the comments have any thoughts on this?

What I'm saying is:

- #70 had a test failure
- #72 added a new template to stable as to fix the failure from #70
- this caused a new different failure
- #74 fixed the failure from #72

What Adam and I are saying is that we suspect #72 is a mistake, we should be trying to fix the failure from #70 without adding a new template to stable.

@jonathanshaw thanks for highlighting the issue.

After investing patch submitted in #70 yes I can see that entity-page-title.html.twig template was added in stable to pass the test which leads to another problem.

Since we are removing dependencies from the stable of the existing core themes. I have added the new template in $templatesToSkip array.

Seems right. Thanks @sja112!

According to [#2580687]:

The Stable theme will function as a backwards compatibility layer for Drupal 8's core markup, CSS and JS.
...
This change allows core markup, CSS and JS to further evolve throughout Drupal 8, while still allowing themes to have a stable base for the clean, minimal markup provided by Drupal 8 core.

I'm not familiar with these matters, but I suspect that in order to function as a BC layer, stable MUST have a duplicate of the original version of every template present in core. This would allow core templates to change without breaking themes that use stable.

And so as we're adding a new template to core, we should add it to stable. Stable is being moved to contrib, but it hasn't been yet.

This makes we wonder if @sja112 had it right in #74 after all.

I think you're referring to StableDecoupledTest (which comes from #3113211: Create test for theme asset decoupling from Stable and address any regressions that decoupling may cause).

According to that issue, and the docs of StableDecoupledTest, it should only be concerned with assets that are in core but not in stable. Which would not include our template once it was added to stable by #72. This is what you pointed out already in #75.

I think the answer to why #72 fails in StableDecoupledTest might be this:

/core/tests/Drupal/KernelTests/Core/Theme/StableDecoupledTest.php
@@ -0,0 +1,305 @@
      $before = [
        '* Default theme implementation',
...
      $after = [
        '* Theme override',

It looks like the test has an opinion of how Stable's templates should be documented, different to how core's should be.

So #72 doesn't document Stable's template correctly.

+++ b/core/themes/stable/templates/field/entity-page-title.html.twig
@@ -0,0 +1,15 @@
+ * @file
+ * Default theme implementation for entity page title.

This should be changed to:

+++ b/core/themes/stable/templates/field/entity-page-title.html.twig
@@ -0,0 +1,15 @@
+ * @file
+ * Theme override for entity page title.

@jonathanshaw and @AdamPS
the change from #86 not enough to fix test.
If we pay attetion to failure test. to we can see, that test try to find template files in themes dir (\Drupal\KernelTests\Core\Theme\StableDecoupledTest::providerTestDecoupledStable), but doens't them.
So I added template files to themes dir and now test should pass (it was done on my local env at least)
Let's see that test bot say!

Yay!

Makes sense.

This has been a long time coming. Are we nearly there yet?

Thank you all for the great work here!
Let's try to trigger some subsystem maintainer and commiter with a few tags they might follow.

I think this really needs front end framework manager review so tagging with that.

The logic looks OK to me and the comments are clear, but I don't have an opinion on the new template.

Re-uploading entity.title-2941208-93.patch so it gets retested every 2 days against 9.2.x-dev.

Currently it's tested every 2 days against 9.1.x-dev.

+++ b/core/modules/node/tests/modules/node_display_configurable_test/node_display_configurable_test.module
@@ -25,4 +25,5 @@ function node_display_configurable_test_entity_base_field_info_alter(&$base_fiel
   $entity_types['node']->set('enable_base_field_custom_preprocess_skipping', TRUE);
+  $entity_types['node']->set('enable_page_title_template', TRUE);

We should have documented enable_base_field_custom_preprocess_skipping better when we added it. This issue is adding enable_page_title_template. The past mistake doesn't mean we should do better here.

This makes me wonder if we should be adding it to annotation on \Drupal\node\Entity\Node and set to FALSE - the default - if we do that we'll need to cope with entity type info updating. But at least it is more discoverable.

Or should we be documenting it on \Drupal\Core\Entity\EntityType or \Drupal\Core\Entity\ContentEntityType?

There's no general documentation on the EntityType classes, so I don't see how a documentation would find a home there.

Unless @alexpott was thinking of changing the fix as well to do something more explicit by declaring the property on the entity type e.g.

  /**
   * Indicates whether page title should use a template.
   *
   * @var bool
   */
  protected $enable_page_title_template = NULL;

There's also

  /**
   * Any additional properties and values.
   *
   * @var array
   */
  protected $additional = [];

But if we want to document additional properties, maybe that should be a seperate issue.

Nice

1. Yes to #114

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -64,14 +77,34 @@ public static function create(ContainerInterface $container) {
   +        $page_title = [
   +          '#theme' => 'entity_page_title',
   +          '#title' => $entity->label(),
   +          '#entity' => $entity,
   +          '#view_mode' => $page['#view_mode'],
   

   Maybe we should add $page_title['#formatted'] = $page[$label_field]; so that a themer could use the output of a configured custom formatter in a custom template if they wanted to. The default template should still use {{ title }} because only the plain label can be trusted to have the right markup.

3. +++ b/core/modules/system/templates/entity-page-title.html.twig
   @@ -0,0 +1,15 @@
   + * Available variables:
   + * - attributes: HTML attributes for the containing span element.
   + * - title: Entity label
   

   We're missing the entity and view_mode variables from the comments on the template. NW for that.

Added missing the entity and view_mode variables from the comments on the template.

There's 3 templates, #116 only adds to one.

Nit: title and entity don't have a period at the end of their line of the comment, whereas the others do. I'm not sure which is right, but we should be consistent.

Added to remaining templates as well.

Test fail looks like a random test failure. Ordered a retest to make sure.

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

Restoring status after HEAD was broken.

patch #133 successfully applied for drupal 9.3.x-dev. thank's @AdamPS

re-roll

+++ b/core/themes/stable/templates/field/entity-page-title.html.twig
@@ -0,0 +1,15 @@
+ * Theme override for entity page title.

+++ b/core/themes/stable9/templates/field/entity-page-title.html.twig
@@ -0,0 +1,15 @@
+ * Theme override for entity page title.

probably this files should be added to test core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php but it needs frontend review

Rerolled/rebased on 9.4.x, resolved conflicts after #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods.

Patch from MR also applies to 9.3.0-rc1

+++ b/core/themes/stable/templates/field/entity-page-title.html.twig
@@ -0,0 +1,15 @@
+ * Theme override for entity page title.
...
+<span{{ attributes }}>
+  {{ title }}
+</span>

+++ b/core/themes/stable9/templates/field/entity-page-title.html.twig
@@ -0,0 +1,15 @@
+ * Theme override for entity page title.
...
+<span{{ attributes }}>
+  {{ title }}
+</span>

it needs review, probably stable[9] should not be changed

I think #142 may be a bad reroll, it seems to revert changes introduced by #2993647: Correctly determine when to display fields as inline in EntityViewController.

Yikes, thanks @jonathanshaw. Should we be concerned tests still passed?

I applied the patch from #147 to the MR.

I noticed and reverted an unintentional change in 147 to the :void return type for \Drupal\Tests\node\Functional\NodeDisplayConfigurableTest::assertNodeHtml, originally added by #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods

is it possible to bring this also back to 9.3 or is it already to late because it is in beta already ?

As a bug it needs backport but probably to 9.3.1

hi is this also working for D8.9.20.. i am trying to apply patch but not working

@delacosta456, judging by #153 it could be available for 9.3.1, but Drupal 8 is no longer supported.

Final observations, none of this is actionable.

I think the only thing missing here is to get the 'Needs frontend framework manager review' tag removed so that requires another look from @lauriii - I'll ping him.

I discussed this issue with @catch and his preference was that if this were to go into 9.4, sooner would be better.

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -55,6 +55,19 @@ public static function create(ContainerInterface $container) {
+   *   back-compatibility to support sites that expect attributes set on the

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -64,18 +77,37 @@ public static function create(ContainerInterface $container) {
+        $page_title = [
+          '#theme' => 'entity_page_title',
+          '#title' => $entity->label(),
+          '#entity' => $entity,
+          '#view_mode' => $page['#view_mode'],
+        ];
+        $page['#title'] = $this->renderer->render($page_title);

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -64,18 +77,37 @@ public static function create(ContainerInterface $container) {
+      else {

+++ b/core/modules/quickedit/quickedit.module
@@ -128,6 +128,23 @@ function quickedit_preprocess_page_title(&$variables) {
+  $variables['#cache']['contexts'][] = 'user.permissions';
+  $entity = $variables['entity'];

I added one comment on the MR - overall I think this looks great. The draft CR also looks great. Looking forward to working on #3015623: Remove outdated code relating to "Expose Title in Manage Display" 😁

Tagging with needs follow-up because of #156.

Whoops, confirmed.

This looks good to go to me.

Queued a test run against 10.0.x - good news is it applied cleanly (at least with a -3)

Once we get a green run on 10.0.x I'll commit this.

Thanks @AdamPS and @jonathanshaw for your perseverance here, having an issue on and off RTBC for 3 years is a mammoth effort.

Adding commit credits for folks I've spoken to about this or who have left reviews above.