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:
    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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Category: feature » task
pwolanin’s picture

Status: Active » Needs review
Issue tags: +API change, +MenuSystemRevamp, +WSCCI, +Stalking Crell, +Approved API change
FileSize
1.3 KB

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

xjm’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

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

pwolanin’s picture

@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.

Crell’s picture

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.

damiankloip’s picture

FileSize
4.36 KB
5.66 KB

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

larowlan’s picture

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}.

jibran’s picture

Status: Needs review » Needs work

As per #6.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.6 KB
8.26 KB

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.

dawehner’s picture

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.

Crell’s picture

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. :-)

damiankloip’s picture

Status: Needs work » Needs review
FileSize
476 bytes
8.25 KB

This should fix those I think.

Status: Needs review » Needs work

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

dawehner’s picture

  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!!

Letharion’s picture

Status: Needs work » Needs review
FileSize
8.75 KB

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.

Letharion’s picture

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.

Letharion’s picture

Wrong namespace on the userinterface. Trying again.

Letharion’s picture

Status: Needs work » Needs review
damiankloip’s picture

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

dawehner’s picture

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

pwolanin’s picture

@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.

dawehner’s picture

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

pwolanin’s picture

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.

dawehner’s picture

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

pwolanin’s picture

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
8.81 KB

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.

dawehner’s picture

Assigned: Unassigned » dawehner

Working on a generic component.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
13.19 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.67 KB
13.91 KB
  • 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.91 KB

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.

dawehner’s picture

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.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
10.17 KB

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.7 KB
12.87 KB

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

pwolanin’s picture

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().

pwolanin’s picture

This makes the change per discussion in IRC.

Crell’s picture

  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.

dawehner’s picture

FileSize
2.31 KB
13.49 KB

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

pwolanin’s picture

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.

pfrenssen’s picture

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.

dawehner’s picture

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}.)

alexpott’s picture

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!

pwolanin’s picture

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

star-szr’s picture

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.

Anonymous’s picture

Issue summary: View changes

update summary

joachim’s picture