Problem/Motivation

This is a direct blocker for #2352155: Remove HtmlFragment/HtmlPage. Over there, we're working to make page rendering understandable again.

In HEAD, we MUST render the main content FIRST, because we need that for the title. i.e. we can't just render $page, we must first render $page['content'], because $page['content']['#title'] may define the page title, which we need to print as part of $page's template.

This breaks the stack-based bubbling, because rendering $page['content'] (the main content) first separately means its bubbleable metadata (attachments, cache tags and #post_render_cache callbacks) won't be part of the stack of metadata being bubbled when rendering $page. The reason HEAD gets around this are: hacks that use globals to juggle things around, which we've been working to get rid of, plus drupal_render() not yet being as strict as it should be (a non-recursive (root) call to drupal_render() that does not result in an empty stack at the end of the root drupal_render() call does not complain currently, but should throw an exception, because it means bubbleable metadata is being left behind).
As of #9, this is no longer true. It'd still be better if we could enforce this, but we can't. But we can convert many (most?) uses in Drupal 8 HEAD to no longer use $main_content['#title']. Also see #7.

It also creates an utterly awful DX, the first testament of which is Drupal core itself: its *.routing.yml files are littered with _title attributes which are never ever used, because they're overridden by $main_content['#title'] ($page['content']['#title'])!
So: we think we define a title, but then dynamically override it. Or also: we think we override the title in a hook_form_alter() and then it's never actually overridden due to a change elsewhere, and is then falling back to the static _title. To top it off, the path-based breadcrumb builder will just look at every single path component along the way… including those that don't represent a valid path! E.g. /nl/node/1/translations/add/en/nl, for which _title = 'Add' is defined, but it's dynamically overridden using #title. The breadcrumb builder then gets the titles for /nl/node/1/translations/add/en and /nl/node/1/translations/add, which is 'Add' according to the static title, but in fact neither of those paths are *valid*! They both trigger exceptions. So we get a breadcrumb like Home > NODE TITLE > Translations > Add > Add, but neither of the two last links work, and this fact is subtly hidden, in part thanks to the combination of _title and #title.
An incredibly large percentage of titles is currently broken in this way in Drupal 8 HEAD.

I'd argue something as fundamental as page titles for a CMS should work in a very clear, consistent way, and bubbling is clearly not the best way to achieve that.

Proposed resolution

Remove the ability to set a dynamic page title using #title, always use _title or _title_callback.

Remaining tasks

Get green.

User interface changes

None. Except more consistent page titles for entity routes.

API changes

No longer able to set/override the page title using #title on main content returned in a _content or _form controller

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
119.4 KB

This patch won't be green, but it does the majority of the work.

A nice consequence is that all _entity_view and _entity_formroutes for example automatically get standardized titles, rather than the many inconsistent, missing and quite often plain broken page titles.

Status: Needs review » Needs work

The last submitted patch, 1: rm_title_bubbling-2359901-1.patch, failed testing.

dawehner’s picture

First, its a total valid statement to convert as many of those examples to use _title and _title_callback, but let's play the realist here.

The actual reason (not mentioned sadly in the IS)
why we not only added support for _title/_title_callback but especially used #title, is really simple:

We want to be able to change the title of a page/block/... depending on the content of the page/block. A really simple example is the frontpage "/node".

If you have no content yet, the title of it says "Welcome to Drupal". In other cases there is no title, but you easily can come up with many other examples.
A search page for example also might want to display the amount of results inside the title ...

In HEAD, we MUST render the main content FIRST, because we need that for the title. i.e. we can't just render $page, we must first render $page['content'], because $page['content']['#title'] may define the page title, which we need to print as part of $page's template.

There are other reasons why we render the main content first, for example we do have this function called views_get_page_view() which themers use all over the place to add classes to the page template
based upon it. It can't have no results when we come to page rendering, or we should drop support for it entirely.

Is there a reason why we can't render all stuff in all "regions" first and then render the page/html itself?
So you start with rendering each block, and then extract the bubbleable data out of it and put that into the main page template.

I opened a dedicated test issue to ensure that things don't break under the hood, again: #2359931: Ensure that empty title support does not break.

dawehner’s picture

  1. index 810309f..3eab0a0 100644
    --- a/core/lib/Drupal/Core/Controller/TitleResolverInterface.php
    
    --- a/core/lib/Drupal/Core/Controller/TitleResolverInterface.php
    +++ b/core/lib/Drupal/Core/Controller/TitleResolverInterface.php
    
    +++ b/core/lib/Drupal/Core/Controller/TitleResolverInterface.php
    @@ -28,8 +28,9 @@
    
    @@ -28,8 +28,9 @@
        * @param \Symfony\Component\Routing\Route $route
    ...
        *
    -   * @return string|null
    -   *   The title for the route.
    +   * @return string|array|null
    +   *   The title for the route. May be a string or a render array, or NULL if
    +   *   there is no title.
        */
       public function getTitle(Request $request, Route $route);
     
    diff --git a/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
    

    In case we want to adapt the TitleResolverInterface we might want to expand the test coverage as well?

  2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
    @@ -79,11 +117,11 @@ public function view(EntityInterface $_entity, $view_mode = 'full', $langcode =
             $build = $this->entityManager->getTranslationFromContext($_entity)
               ->get($label_field)
               ->view($view_mode);
    -        $page['#title'] = drupal_render($build, TRUE);
    +        return $build;
    ...
    +    return String::checkPlain($this->entityManager->getTranslationFromContext($_entity)->label());
    
    +++ b/core/modules/contact/src/Controller/ContactController.php
    @@ -60,6 +60,38 @@ public static function create(ContainerInterface $container) {
    +  public function contactSitePageTitle(ContactFormInterface $contact_form = NULL) {
    +    // Use the default form if no form has been passed.
    +    if (empty($contact_form)) {
    +      $contact_form = $this->entityManager()
    +        ->getStorage('contact_form')
    +        ->load($this->config('contact.settings')->get('default_form'));
    +    }
    +    return String::checkPlain($contact_form->label());
    +  }
    

    Just curious, but DO people really have to call check plain for their own, because many places in the patch doesn't do it ...

  3. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -46,6 +46,38 @@ class EntityForm extends FormBase implements EntityFormInterface {
    +      $type = entity_load($this->entity->getEntityType()->getBundleEntityType(), $this->entity->bundle())->label();
    

    Let's not introduce shit here, entity_load() should not be used since quite a time.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -46,6 +46,38 @@ class EntityForm extends FormBase implements EntityFormInterface {
    +    switch ($this->getOperation()) {
    +      case 'add':
    +        return $this->t('Add <em>@type</em>', ['@type' => $type]);
    +
    +      case 'edit':
    +        return $this->t('<em>Edit @type</em> @title', ['@type' => $type, '@title' => $this->entity->label()]);
    +
    +      case 'delete':
    +        return $this->t('<em>Delete @type</em> @title', ['@type' => $type, '@title' => $this->entity->label()]);
    +
    +      case 'configure':
    +        return $this->t('<em>Configure @type</em> @title', ['@type' => $type, '@title' => $this->entity->label()]);
    +
    +      case 'duplicate':
    +        return $this->t('<em>Duplicate @type</em> @title', ['@type' => $type, '@title' => $this->entity->label()]);
    +    }
    +  }
    

    What would other operations do, provide their own getTitle() method?

  5. +++ b/core/modules/comment/src/CommentForm.php
    @@ -116,11 +133,6 @@ public function form(array $form, FormStateInterface $form_state) {
           $status = $comment->getStatus();
    -      if (empty($comment_preview)) {
    -        $form['#title'] = $this->t('Edit comment %title', array(
    -          '%title' => $comment->getSubject(),
    -        ));
    
    +++ b/core/modules/comment/src/Controller/CommentController.php
    @@ -260,6 +257,23 @@ public function getReplyForm(Request $request, EntityInterface $entity, $field_n
    +    if ($request->request->get('op') == $this->t('Preview')) {
    

    Is it just me who thinks that bypassing FAPI here is a hack?

  6. +++ b/core/modules/config_translation/src/Controller/ConfigTranslationListController.php
    @@ -67,12 +67,32 @@ public function listing($mapper_id) {
    +    return $mapper->getTypeLabel();;
    

    I love php :)

  7. +++ b/core/modules/field_ui/src/Form/FieldStorageEditForm.php
    @@ -211,4 +210,17 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +    return $field_config->label();
    
    +++ b/core/modules/forum/src/Controller/ForumController.php
    @@ -100,18 +100,27 @@ public function forumPage(TermInterface $taxonomy_term) {
    +      return $this->vocabularyStorage->load($this->config('forum.settings')->get('vocabulary'))->label();
    

    Here is one of these instances I would not trust, if possible.

  8. +++ b/core/modules/help/src/Controller/HelpController.php
    @@ -145,4 +144,25 @@ public function helpPage($name) {
    +  public function title($name) {
    +    if ($this->moduleHandler()->implementsHook($name, 'help')) {
    +      $info = system_get_info('module');
    +      return String::checkPlain($info[$name]['name']);
    +    }
    +    else {
    +      throw new NotFoundHttpException();
    +    }
    +  }
    

    It is kinda crazy that we have to execute the hook_help code twice now. Are you sure this won't be problematic in terms actual performance?

  9. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
    @@ -138,6 +138,13 @@ public function build(RouteMatchInterface $route_match) {
    +      // @todo this does not yet take into account paths with required slugs,
    +      //   such as '/nl/node/1/translations/add/en/nl', where you cannot just
    +      //   chop off the individual parts and expect it to work. The only reason
    +      //   that "worked" so far, is because there was a static _title attribute
    +      //   on the route, which then resulted in "node title > Translations > Add
    +      //   > Add", which is utterly nonsensical. A way to deal with this must be
    +      //   figured out.
    

    ... I think the claim is not that the PathBasedBreadcrumbBuilder always works, its just the default implementation which works for most examples. If you need something more advanced you can always use a custom one, if you like. Maybe having a custom one for the translation page is something reasonable to do.

  10. +++ b/core/modules/system/tests/modules/entity_test/src/Controller/EntityTestController.php
    @@ -78,17 +92,33 @@ public function testAdd($entity_type_id) {
    -    return '';
    +    return [];
    

    oh? We cannot return a string anymore?

  11. +++ b/core/modules/views/src/Entity/View.php
    @@ -454,4 +454,40 @@ protected function drupalGetSchema($table = NULL, $rebuild = FALSE) {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getTitle($display_id) {
    +    $title = NULL;
    +
    +    // Start with the default (master) display's title.
    +    if (isset($this->display['default']['display_options']['title'])) {
    +      $title = $this->display['default']['display_options']['title'];
    +    }
    +
    +    // Then look at the specified display, it might override the title.
    +    $display_options = $this->display[$display_id]['display_options'];
    +
    +    // Static title override.
    +    if (isset($display_options['title'])) {
    +      $title = $display_options['title'];
    +    }
    +
    +    // Potentially dynamic title override.
    +    if (isset($display_options['arguments'])) {
    +      foreach ($display_options['arguments'] as $argument) {
    +        if ($argument['title_enable']) {
    +          $title = $argument['title'];
    +          if (strstr($title, '%') !== FALSE) {
    +            $title = FALSE;
    +          }
    +          break;
    +        }
    +      }
    +    }
    +
    +    return $title;
    +  }
    +
     }
    

    As described earlier, this doesn't work. On top of that, this functionality does not belong into the View entity, because the View entity is just about storage. All logic is part of the ViewExecutable.

  12. +++ b/core/modules/views/src/Routing/ViewPageController.php
    @@ -103,14 +131,38 @@ public function handle($view_id, $display_id, Request $request, RouteMatchInterf
    +    if (empty($entity)) {
    +      throw new NotFoundHttpException(String::format('Page controller for view %id requested, but view was not found.', array('%id' => $view_id)));
         }
    

    Is it just me, or are you really convinced to throw an exception as part of the title handling? With the current implementation of the TitleResolver this will bubble up to a 403 exception. Is this really what you wanted? This is afaik a functionality change ...

andypost’s picture

Issue tags: +API change

This is a serious API change at this stage! Suppose needs approval.
Overall the change is great step towards sanity of page building and unification of page titles!

The only caveat I see that some title callbacks will require to duplicate a logic of controller to set proper title.
The same we have for blocks that "build" their title within content, and probably this behaviour is used to set page title from the main content block(controller). So we could clearly document in change notice how to deal with that kind of changes with all recommended approaches.

Currently the "virtuality" of main content block mixed with controller's content (and #title) so we need better to separate this concerns

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityConfirmFormBase.php
    @@ -19,6 +19,13 @@
    +  public function getTitle() {
    
    +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
    @@ -62,10 +63,47 @@ public static function create(ContainerInterface $container) {
    +  public function title(EntityInterface $_entity, $view_mode = 'full') {
    
    +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
    @@ -61,22 +61,28 @@ public function __construct(ControllerResolverInterface $resolver, EntityManager
    +        $enhanced_route_defaults['_title_callback'] = '\Drupal\Core\Entity\Controller\EntityViewController::title';
    

    Let's unify that to getTile() - nice pattern

  2. +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
    @@ -61,22 +61,28 @@ public function __construct(ControllerResolverInterface $resolver, EntityManager
    +        if (empty($defaults['_title']) && empty($defaults['_title_callback'])) {
    +          $enhanced_route_defaults['_title_callback'] = array($wrapper, 'getTitle');
    

    Do this makes the method required?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -46,6 +46,38 @@ class EntityForm extends FormBase implements EntityFormInterface {
    +    // If this entity is of a certain bundle, use the bundle label, otherwise
    +    // use the entity type label.
    +    $bundle_entity_type = $this->entity->getEntityType()->getBundleEntityType();
    +    if ($bundle_entity_type === 'bundle') {
    +      $type = $this->entity->getEntityType()->getLowercaseLabel();
    +    }
    +    else {
    +      $type = entity_load($this->entity->getEntityType()->getBundleEntityType(), $this->entity->bundle())->label();
    

    The comment is wrong.
    ... if no entity type defined as bundle for this type we use type name label, otherwise the label of bundle entity type.

    Also "entity_load" needs injection of EM

  4. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
    @@ -46,6 +46,38 @@ class EntityForm extends FormBase implements EntityFormInterface {
    +    switch ($this->getOperation()) {
    +      case 'add':
    +        return $this->t('Add <em>@type</em>', ['@type' => $type]);
    

    Great! please change @type to @entity_type_label - this should help translation to get more context

    Not clear how to extend this for other opertions

  5. +++ b/core/modules/comment/src/CommentForm.php
    @@ -71,6 +71,23 @@ protected function init(FormStateInterface $form_state) {
    +        return $this->t('Add <em>@type</em>', ['@type' => $type]);
    ...
    +        return $this->t('<em>Edit @type</em> @title', ['@type' => $type, '@title' => $this->entity->label()]);
    

    the same, and better use "Edit comment %title" as it was

  6. +++ b/core/modules/comment/src/CommentForm.php
    @@ -71,6 +71,23 @@ protected function init(FormStateInterface $form_state) {
    +      case 'edit':
    
    @@ -116,11 +133,6 @@ public function form(array $form, FormStateInterface $form_state) {
    -        $form['#title'] = $this->t('Edit comment %title', array(
    

    this is a different pattern

  7. +++ b/core/modules/comment/src/Controller/CommentController.php
    @@ -260,6 +257,23 @@ public function getReplyForm(Request $request, EntityInterface $entity, $field_n
    +    $title = $this->t('Add new comment');
    +    if ($request->request->get('op') == $this->t('Preview')) {
    +      $title = $this->t('Preview comment');
    +    }
    +    return $title;
    

    Please reformat that to:

    if () {
      return ...
    }
    return ...
  8. +++ b/core/modules/field_ui/src/Form/FieldEditForm.php
    @@ -237,11 +230,17 @@ public function delete(array &$form, FormStateInterface $form_state) {
       public function getTitle(FieldConfigInterface $field_config) {
    ...
    +    $bundles = entity_get_bundles();
    

    is this fine?

  9. +++ b/core/modules/help/src/Controller/HelpController.php
    @@ -145,4 +144,25 @@ public function helpPage($name) {
    +      $info = system_get_info('module');
    

    maybe use module handler somehow?

  10. +++ b/core/modules/node/node.routing.yml
    @@ -51,8 +52,7 @@ entity.node.preview:
    -    _content: '\Drupal\node\Controller\NodeViewController::view'
    -    _title_callback: '\Drupal\node\Controller\NodeViewController::title'
    +    _entity_view: 'node.full'
    
    +++ /dev/null
    @@ -1,66 +0,0 @@
    -class NodeViewController extends EntityViewController {
    

    nice clean-up!

  11. +++ b/core/modules/node/src/Form/NodeRevisionRevertForm.php
    @@ -62,7 +62,8 @@ public function getFormId() {
    -  public function getQuestion() {
    +  public function getQuestion($node_revision = NULL) {
    +    $this->revision = $this->nodeStorage->loadRevision($node_revision);
    

    is there other way? loading NULL is not good

  12. +++ b/core/modules/search/search.routing.yml
    @@ -26,7 +25,6 @@ entity.search_page.edit_form:
    -    _title_callback: '\Drupal\search\Controller\SearchController::editTitle'
    
    +++ b/core/modules/search/src/Controller/SearchController.php
    @@ -158,19 +172,6 @@ public function redirectSearchPage(SearchPageInterface $entity) {
    -    return $this->t('Edit %label search page', array('%label' => $search_page->label()));
    

    is different to common pattern.

  13. +++ b/core/modules/search/src/Controller/SearchController.php
    @@ -58,6 +58,21 @@ public static function create(ContainerInterface $container) {
    +  public function title(SearchPageInterface $entity) {
    
    +++ b/core/modules/search/src/Routing/SearchPageRoutes.php
    @@ -80,7 +80,7 @@ public function routes() {
    -          '_title' => 'Search',
    +          '_title_callback' => 'Drupal\search\Controller\SearchController::title',
    

    maybe getTitle?

Berdir’s picture

(Wanted to post this before all the other comments, but failed because crosspost)

I can see the problems, but this is also a DX regression in some cases, as it will be a lot more complicated to set the title in some cases?

At the same time it can also help with the confusion between _title and #title. Which has been a discussion point for #2287733: Configure page title per display. Which is a good example, I'm not sure how to move forward there..

A page can have multiple displays, and one is chosen and runtime based on visibility conditions (like user being logged in or not, but also context for a specific site like current node). So we want to avoid having to do that calculation twice *and* if we are currently on a different page, we might not be able to detect the same display?

A common requirement in the past was to have different labels for local tasks/links than the page title, but I guess that is no longer a problem as those are now always stored separately (like having Edit in the local task and Edit @title as page title then)

Wim Leers’s picture

As long as nobody can adequately answer dawehner's big question, I don't see how we can do what this issue aims to do:

The actual reason (not mentioned sadly in the IS) why we not only added support for _title/_title_callback but especially used #title, is really simple:

We want to be able to change the title of a page/block/... depending on the content of the page/block. A really simple example is the frontpage "/node".

At least, I personally don't know how to adequately answer that excellent question.


… assuming we cannot answer dawehner's excellent question, I think we should relabel this issue to Limit usage of $main_content['#title'] to only the absolutely essential cases, because I think we all want simplicity whenever possible. dawehner is also in favor of that:

First, its a total valid statement to convert as many of those examples to use _title and _title_callback […]

catch’s picture

We want to be able to change the title of a page/block/... depending on the content of the page/block. A really simple example is the frontpage "/node".

Yeah that looks like a valid use-case and I don't have another idea.

At the very least, it would need an alternative mechanism for setting the title based on the content, even if that mechanism has a different implementation than #title.

Another thing here is that the page title in that case is only going to be changed when viewing the page - for menu links the code setting #title won't shouldn't run - and that's a good thing because we don't want to be checking view results just when rendering a link etc.

Wim Leers’s picture

Issue summary: View changes
Priority: Critical » Major
Parent issue: #2350943: [Meta] Untangle Drupal 8 page rendering »

Thanks for confirming, catch. I'll leave this open for another day or so in its current state. If nobody is able to articulate an adequate answer to dawehner's question, I'm retitling this as said in #7.

Already degrading to major, because I found a way around the problem described in the IS, which was the sole reason for me even beginning this work. Since I found a work-around, this is no longer a blocker, hence it's also no longer critical.

aspilicious’s picture

I also need to be able to dynamicly set the title in contrib. As long as we make that possible, I'm happy.

larowlan’s picture

I have an idea about 7, digging

larowlan’s picture

Status: Needs work » Needs review
FileSize
60.75 KB
11.88 KB

So here's my idea - this patch will fail because I've only made a couple of conversions - but it works.

The main premise is to change the reponsibility for creating the HtmlFragment object, creating it earlier and letting the controller resolver pass it to controllers if they have an argument called fragment

Requires some minor changes to CacheableInterface but other than that - it seems to work.

Sample conversions for help and forum module included.

eg help page has this routing

help.page:
  path: '/admin/help/{name}'
  defaults:
    _content: '\Drupal\help\Controller\HelpController::helpPage'
    _title: 'Help'
  requirements:
    _permission: 'access administration pages'

And I made this change

diff --git a/core/modules/help/src/Controller/HelpController.php b/core/modules/help/src/Controller/HelpController.php
index 2de0a53..29bef88 100644
--- a/core/modules/help/src/Controller/HelpController.php
+++ b/core/modules/help/src/Controller/HelpController.php
@@ -8,6 +8,7 @@
 namespace Drupal\help\Controller;

 use Drupal\Core\Controller\ControllerBase;
+use Drupal\Core\Page\HtmlFragment;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Url;
 use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -100,17 +101,18 @@ protected function helpLinksAsList() {
    *
    * @param string $name
    *   A module name to display a help page for.
+   * @param \Drupal\Core\Page\HtmlFragment $fragment
+   *   Html fragment for the page.
    *
    * @return array
    *   A render array as expected by drupal_render().
    *
-   * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException
    */
-  public function helpPage($name) {
+  public function helpPage($name, HtmlFragment $fragment) {
     $build = array();
     if ($this->moduleHandler()->implementsHook($name, 'help')) {
       $info = system_get_info('module');
-      $build['#title'] = String::checkPlain($info[$name]['name']);
+      $fragment->setTitle($info[$name]['name']);

       $temp = $this->moduleHandler()->invoke($name, 'help', array("help.page.$name", $this->routeMatch));
       if (empty($temp)) {

And it works

Thoughts?

larowlan’s picture

I have a niggling feeling that what I've done in the patch solves the need for #title but may not solve the bigger problem, but not sure

larowlan’s picture

In HEAD, we MUST render the main content FIRST, because we need that for the title. i.e. we can't just render $page, we must first render $page['content'], because $page['content']['#title'] may define the page title, which we need to print as part of $page's template.

So my patch does solve this, $page['content'] is no different to $page['sidebar_first'] - sweet

larowlan’s picture

so keeping going down that path

larowlan’s picture

FileSize
57.01 KB
68.89 KB

Interdiff against #12

Status: Needs review » Needs work

The last submitted patch, 16: title-woes-2359901.16.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
70.95 KB

doh

Status: Needs review » Needs work

The last submitted patch, 18: title-woes-2359901.18.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
73.14 KB

Changes to formcontroller and entityformcontroller to suit

Status: Needs review » Needs work

The last submitted patch, 20: title-woes-2359901.20.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
16.95 KB
87.99 KB

More fixes, but just noticed #2352155: Remove HtmlFragment/HtmlPage so going to pause this approach

Status: Needs review » Needs work

The last submitted patch, 22: title-woes-2359901.22.patch, failed testing.

larowlan’s picture

So before I work on those last fails - are we definitely removing HtmlFragment and the interface?

dawehner’s picture

@larowlan
Yeah probably, so how would your solution works in case we don't have it anymore? Passing something you can alter instead of looking around doesn't seem to be much of a difference though, to be honest.

larowlan’s picture

Yep, its a shame HtmlFragment is perfect for fixing this - will read the other issue to try and get the back story on why it is up for the chop

larowlan’s picture

The reasoning behind removing HtmlFragment make sense.

catch’s picture

So I think the non-HtmlFragment of this could end up similar to larowlan's plan.

- if you don't care about #title or it can be set in the YAML or a title callback, just return the render array
- if you care about title, and it has to happen in your page's _pre_render callback, then you'll have to early-render the page array yourself with drupal_render(), and return one with #markup, all the #attached, and most importantly #title set.

Then it'd just be a case of taking $content['#title'] if it's set.

So this pushes down the responsibility of ensuring #title is set to the controller.

This adds more drupal_render() calls which is not good though, so if it's the only viable way to fix this it might end up a won't fix.

Fabianx’s picture

So if #title is bubbeable we store its state in the stack.

Larowlan showed its possible to store it in some object.

Why don't we just add a title service, which holds the current page title?

The last call wins - similar to how it was before ...

Yes, this is kinda global state, but so is the stack ...

--

Alternatively like #28, we could define that the #title if set, MUST BE in the top level render element or not.

That would prevent us from having to drupal_render() it, but would still allow for controllers to set a custom page title.

And if you need something else, you could implement your own service for setting a title, and then putting that in the top-most-level of the render array.

Fabianx’s picture

Another possibility is a conditional placeholder.

That means:

$title = drupal_get_title_render_array();

which returns whatever the current title is when not in a stack and a placeholder using #post_render_cache for all other cases.

A more complex possibility (suggested in another context by EclipseGC) would be to have a title 'context', similar to how there can be e.g. a node context.

However similar to views_get_current_view() it remains difficult to push the default page controller to a #pre_render callback.

e.g. for the case where you want to display the number of search results. This is only known once you have executed the code to do so ...

However even that code could still execute directly and return attached #title as top level of the render array.

What is important to note is that any code that uses that "global" information though is inherently bound to the page, whereas a dynamic placeholder would even work with caching.

larowlan’s picture

Do we still need to do this now HtmlPage et al are gone?

Wim Leers’s picture

We would be able to get rid of this work-around if this were to happen:

      // We must render the main content now already, because it might provide a
      // title. We set its $is_root_call parameter to FALSE, to ensure
      // #post_render_cache callbacks are not yet applied. This is essentially
      // "pre-rendering" the main content, the "full rendering" will happen in
      // ::renderContentIntoResponse().
      // @todo Remove this once https://www.drupal.org/node/2359901 lands.
      if (!empty($main_content)) {
        $this->renderer->render($main_content, FALSE);
        $main_content = [
          '#markup' => $main_content['#markup'],
          '#attached' => $main_content['#attached'],
          '#cache' => ['tags' => $main_content['#cache']['tags']],
          '#post_render_cache' => $main_content['#post_render_cache'],
          '#title' => isset($main_content['#title']) ? $main_content['#title'] : NULL,
        ];
      }

It'd also be a better/more consistent DX if this patch got in.

But even if we choose to not make the necessary API change, the patch in this issue fixed many, many broken titles. If we choose not to fix the API, then we should rescope the issue to fix the many broken titles.

catch’s picture

Could we split the broken title fixing out to another issue? That's an easier commit and then the patch with the API change will be easier to review if we want to go ahead with it.

dawehner’s picture

Good idea!

Created an issue for that: #2403359: Use _title and _title_callback where possible

Wim Leers’s picture

Thanks for opening that, dawehner! I made the patch in #1 (which didn't contain all the (heroic!) work by larowlan to get rid of title bubbling) apply again to HEAD and uploaded the patch to the issue you opened.

Fabianx’s picture

Wim Leers’s picture

I don't think this is actually a child of #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance — this does not block cacheability of things nor SmartCache nor BigPipe.

So, turning that parent issue into a related issue, because it definitely is related.

andypost’s picture

I think better to keep approach with returning #title as top-level of buildarray
If we going to separate title in #2476947: Convert "title" page element into a block then we need this bubbling from one block to another and also separate preprocess to build HEAD title or read it from global state (#29)

But this "state" could be a block context or request attribute, last patch passes fragment to each controller but we used to pass request already and have a stack of them.
Maybe a kind of title resolver just needs to be updated to get page title from master request atrribute?

  1. +++ b/core/lib/Drupal/Core/Controller/AjaxController.php
    @@ -69,19 +74,23 @@ public function content(Request $request, $_content) {
    +    $request->attributes->set('fragment', $fragment);
         $arguments = $this->controllerResolver->getArguments($request, $callable);
    +    $request->attributes->remove('fragment');
         $page_content = call_user_func_array($callable, $arguments);
    
    +++ b/core/lib/Drupal/Core/Controller/DialogController.php
    @@ -136,19 +139,23 @@ public function dialog(Request $request, RouteMatchInterface $route_match, $_con
    +    $request->attributes->set('fragment', $fragment);
         $arguments = $this->controllerResolver->getArguments($request, $callable);
    +    $request->attributes->remove('fragment');
         $page_content = call_user_func_array($callable, $arguments);
    
    +++ b/core/lib/Drupal/Core/Controller/FormController.php
    @@ -70,9 +73,11 @@ public function getContentResult(Request $request) {
    +    $request->attributes->set('fragment', $fragment);
         $args = $this->controllerResolver->getArguments($request, array($form_object, 'buildForm'));
         $request->attributes->remove('form');
         $request->attributes->remove('form_state');
    +    $request->attributes->remove('fragment');
    
    +++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
    @@ -61,17 +63,21 @@ public function content(Request $request, $_content) {
    +    // Pass the fragment on to the controller resolver.
    +    $request->attributes->set('fragment', $fragment);
         $arguments = $this->controllerResolver->getArguments($request, $callable);
         $page_content = call_user_func_array($callable, $arguments);
    

    why the last does not remove fragment from attributes?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

dawehner’s picture

Title: Only allow _title and _title_calback on routes for setting page titles, no more $main_content['#title'] bubbling » Use _title and _title_callback for as many cases as possible

Let's replace the title to be more realistic

Wim Leers’s picture

#42++

Thanks Daniel!

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Title: Use _title and _title_callback for as many cases as possible » Deprecated #title in favour of route titles and title callbacks
catch’s picture

Title: Deprecated #title in favour of route titles and title callbacks » Deprecate #title in favour of route titles and title callbacks
catch’s picture

Status: Needs work » Postponed
effulgentsia’s picture

Title: Deprecate #title in favour of route titles and title callbacks » Deprecate $main_content['#title'] in favour of route titles and title callbacks

Clarifying issue title so people don't think we're trying to remove #title from form elements, etc.

tim.plunkett’s picture

Title: Deprecate $main_content['#title'] in favour of route titles and title callbacks » Deprecate $main_content['#title'] in favor of route titles and title callbacks

:)

dawehner’s picture

Well, I still believe that deprecate doesn't describe it really well, because deprecating has the tone that its gonna be removed in the future, which at the moment, doesn't work in that architecture, as described multiple times in this issue already :)

catch’s picture

Title: Deprecate $main_content['#title'] in favor of route titles and title callbacks » Discourage $main_content['#title'] in favor of route titles and title callbacks

How about that?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.