Follow-up to #2032535: Resolve 'title' using the route and render array to add _title_callback also which was omitted in that issue.

Problem/Motivation

As seen on #1974408: Convert aggregator_page_source() to a Controller, routes defined via the route system does not have yet the concept of titles. Titles
are not only needed for menu links, but also for the actual title on the page, so things which has been MENU_CALLBACK's before also have titles.

Proposed resolution

Title for a page will be determined by a hierarchy of sources.

Menu links and breadcrumb building will first look for _title_callback; if that's found, it will use the return STRING from that callable. If not found, it will use the string in _title. If that's not found, there is no title. If _title_callback returns a string, it is assumed to be already translated.

HtmlPageController will support the same logic (callback wins over static), but will continue to also support the #title property on the content array as a final override. That way there's not an additional API break with what we just put in. :-) However, the use of #title will be discouraged unless you want the title to be different between the page and the link. This parallels calling drupal_set_title() from within a page callback in D7, which was always inferior to using "title callback".

The solution consists of several points - the 1) _title and 3) #title portions are committed already:

  1. Route definitions can contain a _title property which will be used for static titles

    route_name:
      pattern: '/route-pattern'
      defaults:
        _content: ...
        _title: 'Static title'
  2. Route definitions can contain a _title_callback property which will be used for dynamic titles The value of _title_callback will be a callable definition (identical syntax to _content and _controller). _title_callback will be called using reflection in the same way as _content and _controller, so it has access to all the same parameters.

    route_name:
      pattern: '/route-pattern'
      defaults:
        _content: ...
        _title_callback: 'MyClass::titleMethodName'
  3. If you need an even more dynamic title or override you can return it as part of the main render array. This is like drupal_set_title() in 7 and is discouraged:
    <?php
    class controllerClass {
      class
    controllerMethod() {
        return array(
         
    '#title' => 'Dynamic title' . mt_rand(0, 10),
        );
      }
    }
    ?>

API changes

Add _title_callback to _title on the route and #title on the render array for setting the page title.

#1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)
#1830588: [META] remove drupal_set_title() and drupal_get_title()
#1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff

Files: 
CommentFileSizeAuthor
#47 title-2076085-47.patch13.49 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,338 pass(es).
[ View ]
#47 interdiff.txt2.31 KBdawehner
#45 title_callback-2076085-44.patch13.01 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,645 pass(es).
[ View ]
#45 2076085-43-44.increment.txt1.94 KBpwolanin
#43 title_callback-2076085-43.patch12.87 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,706 pass(es).
[ View ]
#43 2076085-41-43.increment.txt2.7 KBpwolanin
#41 title_callback-2076085-41.patch10.17 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#41 interdiff.txt2.24 KBdawehner
#36 title_callback-2076085-36.patch13.91 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,715 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#34 title_callback-2076085-34.patch13.91 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title_callback-2076085-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 interdiff.txt11.67 KBdawehner
#32 title_resolver-2076085-32.patch13.19 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,259 pass(es), 103 fail(s), and 0 exception(s).
[ View ]
#29 2076085-29.patch8.81 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,731 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#19 2076085-13_18_interdiff.txt1.72 KBLetharion
#19 2076085-18.patch8.74 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 58,546 pass(es), 9 fail(s), and 1 exception(s).
[ View ]
#17 2076085-13_17_interdiff.patch1.72 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2076085-13_17_interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 2076085-17.patch8.75 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#16 2076085-16.patch8.75 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] 58,030 pass(es), 10 fail(s), and 1 exception(s).
[ View ]
#13 2076085-13.patch8.25 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 57,990 pass(es), 10 fail(s), and 1 exception(s).
[ View ]
#13 interdiff-2076085-13.txt476 bytesdamiankloip
#9 2076085-9.patch8.26 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 27 fail(s), and 2 exception(s).
[ View ]
#9 interdiff-2076085-9.txt2.6 KBdamiankloip
#6 2076085-6.patch5.66 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,469 pass(es).
[ View ]
#6 interdiff-2076085-6.txt4.36 KBdamiankloip
#2 title-callback-2076085-1.patch1.3 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,514 pass(es).
[ View ]

Comments

Category:feature» task

Status:Active» Needs review
Issue tags:+API change, +MenuSystemRevamp, +WSCCI, +Stalking Crell, +Approved API change
StatusFileSize
new1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,514 pass(es).
[ View ]

re-posting patch title-callback-2032535-109.patch from the #2032535: Resolve 'title' using the route and render array with a new name.

Issue summary:View changes

Updated issue summary.

Just wondering whether we should name it '_title_controller' as this is exactly what it is.

@dawehner - I think "callback" is easier to understand, even if it's not quite as accurate in terms of the code flow and the use of the controller resolver.

Agreed on callback or callable, not controller. We overload the word controller too much as is. :-)

As noted in the previous issue, this needs at least one test and at least one implementation. Let's do just that, then get this in.

StatusFileSize
new4.36 KB
new5.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,469 pass(es).
[ View ]

Tests, and one aggregator conversion of _title option, _title_callback conversion todo.

user_page would be a good candidate as it will help with fails on #2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.

Status:Needs review» Needs work

As per #6.

Status:Needs work» Needs review
StatusFileSize
new2.6 KB
new8.26 KB
FAILED: [[SimpleTest]]: [MySQL] 58,413 pass(es), 27 fail(s), and 2 exception(s).
[ View ]

Here is the conversion of the user/% page (now user_view route), I guess that's what you meant? As user_page just redirects to here, or login.

Status:Needs review» Needs work

The last submitted patch, 2076085-9.patch, failed testing.

Crell suggested at some issue to wait until #2068471: Normalize Controller/View-listener behavior with a Page object is in, though I doubt that rerolling that other issue would be really hard.

The conflicting changes here are contained enough that I can handle rerolling the HtmlPage issue if needs be, since I don't think HtmlPage is landing in the next 48 hours. :-)

Status:Needs work» Needs review
StatusFileSize
new476 bytes
new8.25 KB
FAILED: [[SimpleTest]]: [MySQL] 57,990 pass(es), 10 fail(s), and 1 exception(s).
[ View ]

This should fix those I think.

Status:Needs review» Needs work

The last submitted patch, 2076085-13.patch, failed testing.

  1. +++ b/core/modules/user/lib/Drupal/user/Controller/UserController.php
    @@ -42,6 +44,19 @@ public function userPage(Request $request) {
    +   * @param \Drupal\Core\Entity\EntityInterface $user
    ...
    +  public function userTitle(EntityInterface $user = NULL) {
    +    return $user ? Xss::filter($user->getUsername()) : '';

    Let's typehint in UserInterface, so we do have the getUsername method available.

  2. +++ b/core/modules/user/user.module
    @@ -915,13 +915,9 @@ function user_menu() {
       $items['user/%user'] = array(
         'title' => 'My account',
    -    'title callback' => 'user_page_title',
    -    'title arguments' => array(1),
    -    'page callback' => 'user_view_page',
    -    'page arguments' => array(1),
    -    'access callback' => 'entity_page_access',
    -    'access arguments' => array(1),
    +    'route_name' => 'user_view',
       );

    Nice!!

Status:Needs work» Needs review
StatusFileSize
new8.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,030 pass(es), 10 fail(s), and 1 exception(s).
[ View ]

Changed to UserInterface as per #15.

Removing the actual callback user_view_page, not just it's reference in hook_menu. Unless someone objects to that, this patch covers #1987898: Convert user_view_page() to a new style controller, and that issue will be closed dup.

StatusFileSize
new8.75 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
new1.72 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2076085-13_17_interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ignore the patch in #16.

Here's the real patch with an interdiff.

Status:Needs review» Needs work

The last submitted patch, 2076085-17.patch, failed testing.

StatusFileSize
new8.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,546 pass(es), 9 fail(s), and 1 exception(s).
[ View ]
new1.72 KB

Wrong namespace on the userinterface. Trying again.

Status:Needs work» Needs review

Yep, looks better to me. Let's see if we still have some of those remaining failures :)

I guess at least for menu links we need the title callback to apply there as well?

@dawehner - good question. Do we want to support dynamic menu link titles in 8?

I think that's a bit of a Drupal WTF, since the title will be dynamic possibly until you edit the link and then it might stop being dynamic.

@pwolanin
It seems to be that we simply have to.
How else will we be able to translation node titles on menu links etc.

dawehner - so, currently when you create a menu link to a node you can set a distinct title from the node title - only in book module do we expect them to be synchronized.

So... I'm not sure what's the right approach here - I might expect each menu link entity to have a language translation if needed.

Status:Needs review» Needs work

The last submitted patch, 2076085-18.patch, failed testing.

I asked gabor via IRC what the plans are for these translations.

This is pretty important for the breadcrumb patch, so let's get it done :)

Status:Needs work» Needs review
StatusFileSize
new8.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,731 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Just rerolled for now, What else do we need to get done here? Some more stuff to enable menu link translations to work? We decided in the other issue that the callback should return a translated string?

Status:Needs review» Needs work

The last submitted patch, 2076085-29.patch, failed testing.

Assigned:Unassigned» dawehner

Working on a generic component.

Assigned:dawehner» Unassigned
Status:Needs work» Needs review
StatusFileSize
new13.19 KB
FAILED: [[SimpleTest]]: [MySQL] 59,259 pass(es), 103 fail(s), and 0 exception(s).
[ View ]

Let's see whether this causes the same amount of failures.

Status:Needs review» Needs work

The last submitted patch, title_resolver-2076085-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.67 KB
new13.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title_callback-2076085-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
  • Ensure if the title resolver returns something else than NULL, so drupal_set_title() still works.
  • Restore the title callback on user_menu() as the breadcrumb is still based upon hook_menu(), so 'title_callback' is used.
  • There is still a failure in breadcrumb test: it relies on a interesting functionality: Set the page title using a menu link.

Status:Needs review» Needs work

The last submitted patch, title_callback-2076085-34.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,715 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Just a rerole.

Status:Needs review» Needs work
Issue tags:-API change, -MenuSystemRevamp, -WSCCI, -Stalking Crell, -Approved API change

The last submitted patch, title_callback-2076085-36.patch, failed testing.

Status:Needs work» Needs review

#36: title_callback-2076085-36.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +MenuSystemRevamp, +WSCCI, +Stalking Crell, +Approved API change

The last submitted patch, title_callback-2076085-36.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.24 KB
new10.17 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Commented out the tests in order to fix it, these ones are removed in the path based breadcrumb builder anyway.

Status:Needs review» Needs work

The last submitted patch, title_callback-2076085-41.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.7 KB
new12.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,706 pass(es).
[ View ]

The last patch is missing the new TitleResolver class from patch #36.

for both _title_callback and _title I think we should either check that they are non-empty, or simply use getDefault(), since the value could be NULL which cuased problems on another issue when calling t().

StatusFileSize
new1.94 KB
new13.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,645 pass(es).
[ View ]

This makes the change per discussion in IRC.

  1. +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
    @@ -0,0 +1,83 @@
    +class TitleResolver {

    Shouldn't this have an interface?

  2. +++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
    @@ -0,0 +1,83 @@
    +    // named default is not set.  By testing thevalue directly, we also

    thevalue, needs a space.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -441,7 +441,7 @@ function testBreadCrumbs() {
    -    $this->assertBreadcrumb('user/' . $this->admin_user->id(), $trail, $link_admin_user['link_title'], $tree);
    +    // $this->assertBreadcrumb('user/' . $this->admin_user->id(), $trail, $link_admin_user['link_title'], $tree);

    If there's a good reason we're doing this, it should be noted in a comment.

Those are all fairly minor nitpicks; once those are fixed this is RTBCable.

StatusFileSize
new2.31 KB
new13.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,338 pass(es).
[ View ]

Kept the commented tests as they are out of scope to fix ... we won't support it anyway in the future in core.

Status:Needs review» Reviewed & tested by the community

Seems TitleResolverInterface.php is in the patch, but not in the interdiff - check the patch not that file.

Also, discussed with dawhener using $this->translationManager->translate() instead of $this->t(). Using the full form makes as much or more sense because this will likely never be subclassed and it's never being applied to an actual string where having t() would help you discover a string that needs translation.

Setting to RTBC per Crell's comment above since the patches fixes his concerns.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.phpundefined
@@ -441,7 +441,7 @@ function testBreadCrumbs() {
-    $this->assertBreadcrumb('user/' . $this->admin_user->id(), $trail, $link_admin_user['link_title'], $tree);
+    // $this->assertBreadcrumb('user/' . $this->admin_user->id(), $trail, $link_admin_user['link_title'], $tree);

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.phpundefined
@@ -467,7 +467,7 @@ function testBreadCrumbs() {
-    $this->assertBreadcrumb('user/' . $this->admin_user->id(), $trail, $link_admin_user['link_title'], $tree);
+    // $this->assertBreadcrumb('user/' . $this->admin_user->id(), $trail, $link_admin_user['link_title'], $tree);

Adding commented out code.

Status:Needs work» Reviewed & tested by the community

We do know that, but this is a test which will be removed by another issue, which though sadly is depending on this one. (#2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.)

Title:Resolve the need for a 'title callback' using the route Change notice: Resolve the need for a 'title callback' using the route
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed 47ec9cf and pushed to 8.x. Thanks!

Title:Change notice: Resolve the need for a 'title callback' using the route Resolve the need for a 'title callback' using the route
Status:Active» Fixed
Issue tags:-Needs change record

added change notice: https://drupal.org/node/2095317

updated change notice: https://drupal.org/node/2067859

Small follow-up to add 'title' back to aggregator_menu(): #2103285: Feed aggregator configuration missing menu title

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

Issue summary:View changes

update summary