From #1981644: Figure out how to deal with 'title/title callback'. Updated: Comment #121

Commits

Patch in #78 was committed in #81.

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:

  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

#1974408: Convert aggregator_page_source() to a Controller
#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
#2023795: REGRESSION: hook_local_actions doesn't use title callback
#2027183: hook_menu() title callback is ignored on routes
#2031473: Convert menu local actions to plugins so that we can generate dynamic titles and paths
#2076085: Resolve the need for a 'title callback' using the route (follow-up)
#2076059: Missing documentation for 'title/title callback' using the route and render array (follow-up)
#2068437: _title does not work on _form (follow-up to make forms work)

CommentFileSizeAuthor
#109 title-callback-2032535-109.patch1.3 KBpwolanin
#108 title-callback-2032535-108.patch1.22 KBpwolanin
#101 title-2032535-101.patch657 bytesdawehner
#78 2032535-title.patch18.85 KBCrell
#75 title-2032535-74.patch18.85 KBdawehner
#71 title-2032535-71.patch19.33 KBdawehner
#71 interdiff.txt1.18 KBdawehner
#62 title-2032535-62.patch19.48 KBdawehner
#62 interdiff.txt1.09 KBdawehner
#54 title-2032535-54.patch19.52 KBdawehner
#54 interdiff.txt709 bytesdawehner
#52 drupal-2032535-52.patch19.52 KBdawehner
#52 interdiff.txt4.34 KBdawehner
#48 title-2032535-48.patch20.08 KBdawehner
#48 interdiff.txt608 bytesdawehner
#43 interdiff.txt1.41 KBdawehner
#43 title-2032535-43.patch20.13 KBdawehner
#41 title-2032535-41.patch20.19 KBdawehner
#38 title-2032535-38.patch20.56 KBdawehner
#36 title-2032535-34.patch18.94 KBdawehner
#36 interdiff.txt3.9 KBdawehner
#33 title-2032535-33.patch16.91 KBdawehner
#33 interdiff.txt745 bytesdawehner
#31 drupal-2032535-31.patch16.8 KBdawehner
#31 interdiff.txt1.28 KBdawehner
#29 title-2032535-29.patch16.77 KBdawehner
#29 interdiff.txt663 bytesdawehner
#27 title-2032535-27.patch16.77 KBdawehner
#27 interdiff.txt2.09 KBdawehner
#20 title-2032535-20.patch18.06 KBdawehner
#20 interdiff.txt2.74 KBdawehner
#17 drupal-2032535-17.patch16.89 KBpwolanin
#15 drupal-2032535-15.patch16.56 KBdawehner
#13 drupal-2032535-13.patch15.78 KBdawehner
#11 vdc-2032535-11.patch15.27 KBdawehner
#11 interdiff.txt9.24 KBdawehner
#7 2032535.routing.page-title.7.patch13.95 KBkatbailey
#7 interdiff.txt1.17 KBkatbailey
#5 2032535.routing.page-title.5.patch13.85 KBkatbailey
#3 routing-2032535-3.patch4.75 KBdawehner
#1 drupal-2032535-1.patch897 bytesdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
897 bytes

Maybe just this is already enough?

Crell’s picture

Title: Figure out how to deal with 'title/title callback' » Resolve 'title/title callback'
Status: Active » Needs review

Renaming for uniqueness. Probably could do with a better name than this.

dawehner’s picture

FileSize
4.75 KB

The only way how to get information from the subrequest to the mainrequest seem to be the header information, so this patch introduces a new "drupal_page_title" header which stores the title.

This title can be either specified in the route information or directly in the render array.

Status: Needs review » Needs work

The last submitted patch, routing-2032535-3.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
13.85 KB

Discussed this in person with effulgentsia and msonnabaum and briefly on irc with dawehner, larowlan and beejeebus. My feeling is that the subrequest handling that happens with these non-response-returning controllers is causing more problems than it solves (in fact, it's not clear to me that it solves any problem at all..?)

So I'm trying an approach that changes the HtmlPageController to not result in a subrequest being handled at all and instead just call the controller directly and wrap the result in drupal_render_page().

I've merged this with the other changes in #3 that did not affect HtmlPageController. Sorry for the lack of an interdiff, it was a bit messy - I originally started out with the patch in #1830588-30: [META] remove drupal_set_title() and drupal_get_title().

Status: Needs review » Needs work

The last submitted patch, 2032535.routing.page-title.5.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
13.95 KB
dawehner’s picture

I really really like the approach.

+++ b/core/includes/theme.incundefined
@@ -2624,8 +2624,14 @@ function template_preprocess_html(&$variables) {
+  // Construct page title

I guess this should be a full sentence.

+++ b/core/includes/theme.incundefined
@@ -2624,8 +2624,14 @@ function template_preprocess_html(&$variables) {
+  if (isset($variables['page']['#title'])) {
+    $head_title = array(
+      'title' => strip_tags($variables['page']['#title']),
+      'name' => check_plain($site_config->get('name')),
+    );
+  }

@@ -2950,7 +2968,13 @@ function template_preprocess_maintenance_page(&$variables) {
+  if (isset($variables['page']['#title'])) {
+    $head_title = array(
+      'title' => strip_tags($variables['page']['#title']),
+      'name' => check_plain($site_name),
+    );
+  }

Let's use the same code path for both the if and elseif.

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.phpundefined
@@ -44,28 +47,22 @@ public function __construct(HttpKernelInterface $kernel) {
   public function content(Request $request, $_content) {

+1 for making the html page controller way easier to understand!

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.phpundefined
@@ -44,28 +47,22 @@ public function __construct(HttpKernelInterface $kernel) {
+    if (!is_array($page_content)) {
+      $page_content = array(
+        '#markup' => $page_content,
+      );
+    }
+    if (!isset($page_content['#title']) && $request->attributes->has('_title')) {
+      $page_content['#title'] = $request->attributes->get('_title');
+    }
+    $response = new Response(drupal_render_page($page_content));

Just a general question: Now that we don't need subrequests anymore, would it make sense to add a title method on the controller? This would bring controllers neared to blocks.

fubhy’s picture

Just a general question: Now that we don't need subrequests anymore, would it make sense to add a title method on the controller? This would bring controllers neared to blocks.

Yes, I would very much like that. I had a discussion with @sdboyer and @EclipseGc (and also daniel for some time of the call) on exactly that topic. A title method would be very cool.

Crell’s picture

+++ b/core/lib/Drupal/Component/Utility/String.php
@@ -13,6 +13,11 @@
   /**
+   * Flag for drupal_set_title(); text is not sanitized, so run String::checkPlain().
+   */
+  const CHECK_PLAIN = 0;
+

Seems like this should be a more self-documenting constant. Even with context it's hard to grok.

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -44,28 +47,22 @@ public function __construct(HttpKernelInterface $kernel) {
+    $resolver = $this->container->get('controller_resolver');

I'd much rather keep HtmlPageController not-container aware and just inject the controller resolver. This isn't a factory, so it shouldn't be able to get away with being container aware. We just removed container aware from it recently. :-)

dawehner’s picture

FileSize
9.24 KB
15.27 KB

As better name for the ControllerInterface was suggested, though this would have blown up the patch size, so I went with a TitleControllerInterface.

Status: Needs review » Needs work

The last submitted patch, vdc-2032535-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.78 KB

Forgot the Title class.

Status: Needs review » Needs work

The last submitted patch, drupal-2032535-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.56 KB

This time with even less missing files (TitleControllerInterface)

pwolanin’s picture

Status: Needs review » Needs work

I don't understand - I don't think this:

$callable->getTitle($request);

can ever work since $callable is an array

also: core/includes/theme.inc: patch does not apply

pwolanin’s picture

Status: Needs work » Needs review
FileSize
16.89 KB

Fixes conflict in theme.inc, and I think fixes the logic of the $callable.

damiankloip’s picture

Sounds like that needs to be tested if the changes you just made are a fix.

pwolanin’s picture

@damiankloip - yes - unclear why the test code didn't fail before. Also, I think dawehner or someone else needs to double-check the changes to theme.inc

dawehner’s picture

FileSize
2.74 KB
18.06 KB

assertTitle seems to check just the < title > element, but not the h1 element on the page.

Crell’s picture

Tagging

dawehner’s picture

Does anyone has a problem with removing the TitleInterface and just use the render array?

Crell’s picture

TitleInterface--

Damien Tournoud’s picture

+    // createController() will usually return a callable in array format,
+    // but it's not guaranteed.
+    $obj = NULL;
+    if (is_array($callable)) {
+      list($obj, $method) = $callable;
+    }

This smells like a gross hack. At the minimum, let's set in stone a [obj, method_name] return value for our ControllerResolver::createController.

Crell’s picture

Damien: Impossible. createController() returns a PHP callable. A PHP callable could be 5 different things: function name, closure object, object implementing __invoke(), class name and static method name, or object and method name. Only two of those will be returned as an array.

That said, if TitleInterface goes away then I don't think we need that code anyway.

Damien Tournoud’s picture

Status: Needs review » Needs work

@Crell: we can decide in our own implementation to restrict the callable to be a array of object and method name. That's perfectly compatible with behavioral subtyping. It's very very far from being impossible by any definition of "impossible".

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
16.77 KB

This time without the title interface.

Status: Needs review » Needs work

The last submitted patch, title-2032535-27.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
663 bytes
16.77 KB

No wonder that this does not work.

Crell’s picture

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -44,28 +46,24 @@ public function __construct(HttpKernelInterface $kernel) {
+    $page_content = call_user_func_array($callable, $arguments);
+    if ($page_content instanceof Response) {
+      return $page_content;
+    }
+    if (!is_array($page_content)) {
+      $page_content = array(
+        '#markup' => $page_content,
+        '#title' => '',
+      );
+    }
+    if ($request->attributes->has('_title')) {
+      $page_content['#title'] = $request->attributes->get('_title');

My read here is that _title overrides a returned #title. I'd think it should be the other way around: Runtime wins over hard coded.

+++ b/core/modules/system/tests/modules/test_page_test/lib/Drupal/test_page_test/Controller/Test.php
@@ -0,0 +1,31 @@
+    $build['#markup'] = t('Hello Drupal');

This is just a test class. We don't need it translated. No t() please. (It's not worth injecting, either.)

In fact the test for it would fail if the text was translated to something else. -:-)

dawehner’s picture

FileSize
1.28 KB
16.8 KB

Yeah I agree.

Damien Tournoud’s picture

+    $callable = $this->controllerResolver->createController($_content);

By calling createController() directly, we support only the service:method and class:method notations and we are missing the other cases handled directly by getController() (aka. function names, object-method pairs, lambdas, classes). Are we happy with that?

+    if (empty($page_content['#title']) && $request->attributes->has('_title')) {
+      $page_content['#title'] = $request->attributes->get('_title');
     }

Should be !isset().

dawehner’s picture

FileSize
745 bytes
16.91 KB

By calling createController() directly, we support only the service:method and class:method notations and we are missing the other cases handled directly by getController() (aka. function names, object-method pairs, lambdas, classes). Are we happy with that?

Well, at least our createController implementation always returns array($object, $method), but it is right that we at somehow internal dependencies.

Let's check for the empty title.

Crell’s picture

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -58,7 +58,8 @@ public function content(Request $request, $_content) {
+    if ((!isset($page_content['#title']) || $page_content['#title'] === '') && $request->attributes->has('_title')) {

Why checking for a #title of ''? If the controller explicitly specified no title, we should have no title. That's a valid thing to do.

katbailey’s picture

Per discussion in irc, we'll do some refactoring so that we'll have a method for getting the callable from a controller, which will take care of all possibilities (addressing DamZ's first point in #32).

dawehner’s picture

FileSize
3.9 KB
18.94 KB

As talked about in IRC, this adds a new method, which allows you to cover all cases of getController.

Status: Needs review » Needs work

The last submitted patch, title-2032535-34.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.56 KB

Forgot the new interface

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +Stalking Crell

The last submitted patch, title-2032535-38.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.19 KB

Rerolled.

Crell’s picture

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
@@ -52,6 +59,47 @@ public function __construct(ContainerInterface $container, LoggerInterface $logg
+  public function getControllerFromAttribute($controller, $path = '') {

There's no "attribute" here. The whole point of this method is that there's no concept of an attribute; just a string in one of the various formats we support.

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -44,28 +54,25 @@ public function __construct(HttpKernelInterface $kernel) {
+        '#title' => '',

I don't think we want this in the default. If a controller returns a string, my assumption is that it should have whatever default title was specified, not force no title.

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -44,28 +54,25 @@ public function __construct(HttpKernelInterface $kernel) {
+    if ((!isset($page_content['#title']) || $page_content['#title'] === '') && $request->attributes->has('_title')) {

Pre previous comment, if an empty title was explicitly set, we should honor that. "Use whatever the default is" is indicated by returning no #title at all.

dawehner’s picture

FileSize
20.13 KB
1.41 KB

There's no "attribute" here. The whole point of this method is that there's no concept of an attribute; just a string in one of the various formats we support.

We (tstoeckler and yesct) talked about that, and we came to the conclusion that we don't have a better idea. If we could rename getController we would name it getControllerFromRequest so getController would be make sense for the new method we introduce here. For sure we can't do that, so a "YesCT: possible solution for the name is getControllerFromNotRequest or getControllerFromDefaultAttribute?". Other suggestions have been getControllerFromRoutingSchema or getControllerFromRoutingDefinition or getControllerFrom ... Do you maybe have an idea?

I don't think we want this in the default. If a controller returns a string, my assumption is that it should have whatever default title was specified, not force no title.

Good review!
Let's fix it, see interdiff.txt

msonnabaum’s picture

Perhaps out of scope, but I think the reason getControllerFromAttribute is an awkward name, is because it's on the wrong object.

That method should be on a new Method Object, something like CallableParser::parse($callable_string).

Crell’s picture

Strictly speaking we're not getting a controller. We're getting a callable. getCallable() sounds good to me.

(In IRC, Mark pointed out that getCallable() shouldn't be part of ControllerResolver but its own object. Maybe, but that's out of scope here so let's not worry about that.)

aspilicious’s picture

+++ b/core/modules/system/tests/modules/test_page_test/lib/Drupal/test_page_test/Controller/Test.phpundefined
@@ -0,0 +1,31 @@
+use Symfony\Component\HttpFoundation\Request;

Request isn't used, does it needs a use statement?

dawehner’s picture

Good catcfh aspilicous!

(In IRC, Mark pointed out that getCallable() shouldn't be part of ControllerResolver but its own object. Maybe, but that's out of scope here so let's not worry about that.)

If we don't get a RTBC in the meantime I will maybe just do it at somepoint here.

dawehner’s picture

FileSize
608 bytes
20.08 KB

So, this time with a patch.

Crell’s picture

We need at least a better method name before we can RTBC. Given how much this blocks I'm OK with not factoring it out to a separate service just yet; that's a non-breaking follow-up IMO.

dawehner’s picture

What about getControllerFromDefinition, as we are working with route definitions (not sure though whether this is the proper word).

fubhy’s picture

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.phpundefined
@@ -38,11 +38,18 @@ class ControllerResolver extends BaseControllerResolver {
+   * The psr3 logger. (optional)

I think we should call it PSR-3 in the documentation. Also, I've never seen "(optional)" in this context.

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.phpundefined
@@ -38,11 +38,18 @@ class ControllerResolver extends BaseControllerResolver {
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
...
+   * @param \Symfony\Component\HttpKernel\Log\LoggerInterface $logger

Unrelated. But, whatever...

+++ b/core/lib/Drupal/Core/Controller/ControllerResolver.phpundefined
@@ -52,6 +59,47 @@ public function __construct(ContainerInterface $container, LoggerInterface $logg
+  public function getControllerFromAttribute($controller, $path = '') {

I'd agree that we need a separate service thing for this. Potentially even just a new Drupal\Components component for resolving arguments for callables. Our ControllerResolver could then simply reference and re-use that one.

Can be a follow-up though.

+++ b/core/lib/Drupal/Core/Controller/ControllerResolverInterface.phpundefined
@@ -0,0 +1,44 @@
+   * The resolver must only throw an exception when it should be able to load
+   * controller but cannot because of some errors made by the developer.

"to load 'a' controller".

Also, "some errors made by the developer" sounds weird. :P (we don't make mistakes anyways, eh?). Not sure about this entire sentence tbh.

+++ b/core/lib/Drupal/Core/Controller/ControllerResolverInterface.phpundefined
@@ -0,0 +1,44 @@
+   *
+   * @api
+   */

Hm? What's that for? :)

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.phpundefined
@@ -44,28 +54,24 @@ public function __construct(HttpKernelInterface $kernel) {
+    // If no title was specified, allow to override it via the request.
+    if (!isset($page_content['#title']) && $request->attributes->has('_title')) {
+      $page_content['#title'] = $request->attributes->get('_title');

+++ b/core/modules/block/block.routing.ymlundefined
@@ -24,5 +24,6 @@ block_admin_add:
     _content: '\Drupal\block\Controller\BlockAddController::blockAddConfigureForm'
+    _title: 'Configure block'

I don't find it particularly appealing to stuff yet another attribute into the request but I can't think of an equally simple solution for D8 so I guess this is fine. Putting it into the route definition is not very nice easier but it's the best we can do for now I guess. At least this is much better than what we have now (nothing). So let's just do it for now and further improve it for D9.

+++ b/core/lib/Drupal/Core/Utility/Title.phpundefined
@@ -0,0 +1,20 @@
+ * Defines some constants related with drupal page titles.

"related 'to' drupal page title'. (I believe)

+++ b/core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.phpundefined
@@ -2,22 +2,24 @@
+ * Contains \Drupal\system\Tests\System\PageTitleTest.

Also completely unrelated.

dawehner’s picture

FileSize
4.34 KB
19.52 KB

Thanks for your review!

I think we should call it PSR-3 in the documentation. Also, I've never seen "(optional)" in this context.

Oh right this is looking way better. Well, the logger might not be there, see constructor.

Unrelated. But, whatever...

let's remove it then.

I'd agree that we need a separate service thing for this. Potentially even just a new Drupal\Components component for resolving arguments for callables. Our ControllerResolver could then simply reference and re-use that one.

Can be a follow-up though.

Do you want to create a follow up for it?

Also, "some errors made by the developer" sounds weird. :P (we don't make mistakes anyways, eh?). Not sure about this entire sentence tbh.

This is all coming from symfony, so let's remove it.

Hm? What's that for? :)

Okay, let's also remove it.

Also completely unrelated.

Not really, because the file got renamed.

Crell’s picture

+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -44,28 +54,24 @@ public function __construct(HttpKernelInterface $kernel) {
+    // If no title was specified, allow to override it via the request.

This line is German grammar, not English. :-)

"If no title was returned fall back to one defined in the route."

Otherwise I think we're good here. It would be nice to factor the callable creation logic out to a stand-alone class, but if we really want that can be done later in a BC-compatible way.

dawehner’s picture

FileSize
709 bytes
19.52 KB

This line is German grammar, not English. :-)

Are you sure this is even german?

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -MenuSystemRevamp, -WSCCI, -Stalking Crell

The last submitted patch, title-2032535-54.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +Stalking Crell

#54: title-2032535-54.patch queued for re-testing.

Crell’s picture

No, but grammatical errors involving the word "allows" seem to mostly come from German developers. I have no idea why. :-)

(And bah, bot!)

jibran’s picture

@@ -0,0 +1,39 @@
+
+/**
+ * Extends the ControllerResolverInterface from symfony.
+ *
+ *
+ */

Extra white space and it still needs issue summary update.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -MenuSystemRevamp, -WSCCI, -Stalking Crell

The last submitted patch, title-2032535-54.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

EDIT sorry the testbot was broken

#54: title-2032535-54.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +MenuSystemRevamp, +WSCCI, +Stalking Crell

The last submitted patch, title-2032535-54.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
19.48 KB

If someone would have said that there is a problem with the twig iniative in a routing issue I would have laughed.

xjm’s picture

Priority: Major » Critical
Issue tags: +API change

I believe this is critical for its impact on #1830588: [META] remove drupal_set_title() and drupal_get_title(), which I've postponed on this, even if the actual menu system change is not in itself release-blocking (which it might or might not be).

Let's get the API changes for this issue documented in the issue summary so that a core maintainer can sign off on them.

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

The last submitted patch, title-2032535-62.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#62: title-2032535-62.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, title-2032535-62.patch, failed testing.

dawehner’s picture

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

#62: title-2032535-62.patch queued for re-testing.

catch’s picture

Issue tags: +Approved API change

Tagging - we need to change the API so we can rip out drupal_set_title(), I'm a week or so out of date on this issue so didn't review most recent approach.

dawehner’s picture

Title: Resolve 'title/title callback' » Resolve 'title/title callback' by adding a _title propery on the route and allow a title be part of the render array.

Adapted the title.

tim.plunkett’s picture

@@ -0,0 +1,37 @@
+interface ControllerResolverInterface extends BaseControllerResolverInterface {

@@ -24,12 +24,22 @@ class HtmlPageController {
+  public function __construct(HttpKernelInterface $kernel, ControllerResolver $controller_resolver) {

Why add the interface and then not typehint by it?

@@ -24,6 +24,7 @@ block_admin_add:
+    _title: 'Configure block'

I guess this needs to be added to the places l.d.o checks for strings

dawehner’s picture

FileSize
1.18 KB
19.33 KB

Rerolled the patch.

Why add the interface and then not typehint by it?

Good catch!

I guess this needs to be added to the places l.d.o checks for strings

Right, is there some kind of meta or another process to add them?

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I'm satisfied, and this blocks other issues. Let's call it done.

Crell’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +API change, +MenuSystemRevamp, +WSCCI, +Stalking Crell, +Approved API change

The last submitted patch, title-2032535-71.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.85 KB

There we go, a straight one-file rerole.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, title-2032535-74.patch, failed testing.

donquixote’s picture

To clarify:
- This does sufficiently and elegantly replace drupal_set_title().
- It does replace the "title" value from D7 hook_menu().
- It does not replace the "title callback" from D7 hook_menu(). Unless we want to build the entire render array to get the dynamic title value. Which we usually want to avoid when we are not on that page already. E.g. to build a breadcrumb from entity urls with aliases.

This being said, I still agree with this patch as a step forward.
Whether we do something about "title callback" or let it die, can be decided in a follow-up, if at all (I seem the only one interested in it).

And apologies if I missed something on the way. I said most I had to say in the beginning (of the other issue). Then I deliberately took some distance from this subject, to avoid the temptation of repeating myself all over..
All I am asking now is to be clear about this in the issue summary and such.

Crell’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.85 KB

I have no idea why the block.routing.yml hunk was not applying, but let's try this.

Crell’s picture

Issue summary: View changes

Updated issue summary.

Dries’s picture

+++ b/core/includes/theme.incundefined
@@ -2675,7 +2677,13 @@ function template_preprocess_page(&$variables) {
+  if (isset($variables['page']['#title'])) {
+    $variables['title'] = $variables['page']['#title'];
+  }
+  else {
+    $variables['title'] = new RenderWrapper('drupal_get_title');

I'm still new to this patch but is there a reason why adding a _title property doesn't result in $variables['page']['#title'] being set? It seems a bit odd to to add all these if-tests in the preprocess functions instead of putting some logic in a centralized place like the HtmlPageController.

Crell’s picture

Currently that function serves both the old pipeline and the new pipeline, so we can't guarantee that page isn't coming from the old routing system. That means we have to account for both cases.

I'm working on a cleanup of that now that helps to smooth out the whole new-style pipeline that will likely clean that up further (including centralizing that logic in the HtmlPageController). Either way, that can get tidied up when we remove the old router.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Alright, that makes sense. Looking forward to the clean-up patch. Committed to 8.x. Thanks. Marking 'needs work' because we'll want to create a change notification.

Crell’s picture

Title: Resolve 'title/title callback' by adding a _title propery on the route and allow a title be part of the render array. » Change Notice: Resolve 'title/title callback' using the route and render array
Status: Needs work » Active

Retitling and filing accordingly.

dawehner’s picture

Status: Active » Needs review

Updated the existing one for the routing system: https://drupal.org/node/1800686/revisions/view/2806137/2807105
Added a new change notice on https://drupal.org/node/2067859

Crell’s picture

Title: Change Notice: Resolve 'title/title callback' using the route and render array » Resolve 'title/title callback' using the route and render array
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks good to me. Thanks!

Gábor Hojtsy’s picture

Well, so this now requires the translation extractor to also look at routing.yml files. Is there any plan to support the menu description as well in this, or is that going to be / is already at a totally different place? I mean items in the admin backend where we used to use hook_menu() only for the title and description at this point because the route handled the association of path to controller and access checking. I've opened #2069923: Support for Drupal 8 routing.yml title to support this in the extraction system. Awaiting clarity on description so we can ensure the translation system supports all cases.

(Aside: if we put the title and description into the route, then how is this at all different from hook_menu() with a different syntax?)

aspilicious’s picture

Gabor we don't need hook_menu anymore. If we leave title in hook_menu we also need to implement hook_menu.
I do miss the custom title callback option.

Gábor Hojtsy’s picture

@aspilicious: right, my question was what happens to this item:

function action_menu() {
  $items['admin/config/system/actions'] = array(
    'title' => 'Actions',
    'description' => 'Manage the actions defined for your site.',
    'route_name' => 'action_admin',
  );
  // ...

Ok, title goes to the routing file or the render array. What happens to description? There is a lack of mention of that on this issue and the change notice. If the title goes to the route, then everything (title, "callback", access) is now on the route for this item, except the description. Is there an issue for that too that I missed and was not mentioned above? I feel like I missed something and need to ensure this is covered for translations, so kind of need the info :)

Crell’s picture

The description is relevant only to the LINK. The title in the YAML file is for the PAGE. hook_menu() conflated those two. The split routing/menu-links systems do not (at least not automatically). That said, we can probably tweak menu links from hook_menu to inherit their title from the route if one is not specified.

Gábor Hojtsy’s picture

@Crell: ok, in short, what's the *code* to make that description appear without hook_menu? (It may only be me seeing a connection between title and description, but both need to have stable support in translations so this needs info).

dawehner’s picture

As the description belong to the menu link it will be either on hook_menu or hook_menu_link or however it will be called.

webchick’s picture

I'm with Gábor on this. I don't care how they're represented under the hood, but "title" and "description" are a unit as far as far as UI text is concerned. It makes absolutely no sense to define one in one place and the other somewhere else.

Gábor Hojtsy’s picture

I don't have strong opinions on this. The routing, tabs and title are already defined at different places. All I was looking for is information on what happens to description. Looks like we don't know yet and since I did not find an issue yet, that seems like still to be opened. We need hook_menu() for now then for that at least.

Crell’s picture

Gabor: I think you're conflating ROUTER ITEMS with MENU LINKS, which hook_menu in D7 and earlier did. There is no code to make description appear without hook_menu(), because hook_menu() is where it makes sense. We needed title to be available sans-menu link entity. We don't need description available sans-menu link entity.

So nothing has changed with regards to description translation.

Gábor Hojtsy’s picture

@Crell: I don't think I'm confusing anything with anything. People said above that hook_menu() is not needed anymore, eg. #86, and where description goes was never explained, I needed explanation on that so I can open / work on the right issues to ensure support for them is not broken. Whatever the answer to that question I just asked for answers :)

That said, tabs and local actions also work as menu links, and they are annotated classes now. So I think it was logical to assume description has a plan some way. Seems to be the only thing left for hook_menu that does not have a solution elsewhere.

tim.plunkett’s picture

I think #86 was an over-simplification. Until this went in, we either needed hook_menu for *routing* for the page title, or we had to add a drupal_set_title to the controller. Now a route is truly standalone from hook_menu().

Menu links, and their titles (not the page title) are still dependent on hook_menu().

Gábor Hojtsy’s picture

In other words, although the issue title says "Resolve 'title/title callback' using the route and render array" it does not have anything to do with hook_menu() at all (or title callback). hook_menu() titles and descriptions are still there and supported and should be used. The code committed here does not "resolve" that from the router item then I guess(?).

Crell’s picture

The thing "resolved" is that there was no way to set a title for a page that wasn't in the menu via hook_menu. Now there is. hook_menu() is at this point really just hook_menu_link_entities(). :-) See also: #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit

xjm’s picture

Title: Resolve 'title/title callback' using the route and render array » [Docs needed] Resolve 'title/title callback' using the route and render array
Status: Fixed » Active

This patch did not add any documentation for the new _title route thing nor #title. The change notice is great, but we also need these things actually documented in the codebase.

I couldn't sort out where to document either. It looks like both are handled by HtmlPageController, so I couldn't decide whether it makes sense to add #title to the drupal_render() docs or not. Also, does #title do anything when it's deeper than the full page array?

Similarly, where are the _routingwhatsit thingies documented?

Reopening rather than creating a followup since this is a serious documentation gate issue.

xjm’s picture

Also, we need to mark drupal_set_title() @deprecated and add an @see to wherever the new docs end up.

xjm’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
657 bytes

I don't think that there is any place in Drupal yet, where the new routing system is documented.
Here is something which deprecates just drupal_set_title().

xjm’s picture

Thanks @dawehner!

For now let's add:

  • A paragraph or two to HtmlPageController's docblock explaining how titles work, with code snippets.
  • An @see from @dawehner's @deprecated patch above to HtmlPageController.
  • A mention of #title in the drupal_render() docs, with an explanation that it is specific to the page array, not a part of Render "API", and an @see to HtmlPageController.
  • Probably an inline comment to template_preprocess_page() as well about where the title is coming from.

We can then do more with it once #2046367: Document the internals of the router is in.

Couple other thoughts:

+++ b/core/includes/theme.incundefined
@@ -2925,6 +2938,14 @@ function template_preprocess_maintenance_page(&$variables) {
+    $variables['title'] = drupal_get_title();

Also hmm, drupal_get_title() calls drupal_set_title(). We need to deprecate that too and add it to the change notice. Also seems like we need a followup to remove it there.

+++ b/core/lib/Drupal/Core/Utility/Title.phpundefined
@@ -0,0 +1,20 @@
+/**
+ * Defines some constants related with Drupal page title.
+ */
+class Title {

I'm wondering if it would make sense to use this class for more than just a bucket of constants? We could add methods and docs to it as appropriate if that were the case.

star-szr’s picture

Status: Needs review » Needs work

CNW based on #102.

pwolanin’s picture

?

donquixote’s picture

If we don't use _title for menu or breadcrumb or any other links, then what is the benefit over #title in the page render array?
And should we not name it _page_title then instead?

One benefit I could see is that one could alter the _title from yml in some way..

pwolanin’s picture

I still think we need to optionally have a title callback - I don't understand why that was removed from the patch.

Crell’s picture

We hashed through this in today's IRC meeting. Conclusion:

Routes will have an additional option parameter added, _title_callback

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.

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

Someone please update the issue summary with that. :-)

Crell’s picture

Issue summary: View changes

update issue summary

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Title: [Docs needed] Resolve 'title/title callback' using the route and render array » Resolve 'title/title callback' using the route and render array
Status: Needs work » Needs review
FileSize
1.22 KB

Here's an initial patch - need to add an implementation and tests.

pwolanin’s picture

oops used has() instead of get() and the wrong method on the controller resolver

Crell’s picture

This is going to conflict with the HtmlPage issue, but it's small enough that I should be able to reroll it. I'm half tempted to just RTBC and be done with it but I'll leave it open for a bit to get other weigh-in.

We may want to try and convert at least one example while we're here just to prove that it works. :-)

tim.plunkett’s picture

Needs an example and a test.

webchick’s picture

This may be a stupid question, but could we not just have some magic so that if _title is a string, it becomes a title, but if _title is a callable, it uses it as a callback to generate the title? Fewer properties to memorize++, and the word "callback" in the name is confusing since that traditionally means "a global function" and what you mean here is "any callable, but usually a method on a class," no?

tim.plunkett’s picture

@webchick, what if the name of your route was 'debug'? That's a callable :)

_title_callable and _title_callback were both tossed around today, callable is the PHP typehint but callback matches the hook_menu closer.

I'm not sure what to call it, but I don't think we should try to tack it onto _title.

Gábor Hojtsy’s picture

@webchick: please, no! If the _title may be a callable or may be user facing text, how would the translation extractor know to extract it for translation or not. Hint: it would have no idea.

dawehner’s picture

If it is a callable in the sense of either a method on an object or a service there would be at least a colon, but yeah this means we can't go with it.

webchick’s picture

Ok, stupid idea then. :P Carry on. :P

xjm’s picture

Status: Needs review » Fixed

#108 is a new thing and should go in its own issue; @pwolanin will file a separate issue for that. Meanwhile I filed #2076059: Missing documentation for 'title/title callback' using the route and render array so that can move forward separately. Docs are a gate and should not be postponed on things that don't exist yet.

pwolanin’s picture

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: Resolve 'title/title callback' using the route and render array » Resolve 'title' using the route and render array

Retitling to clarify how this is different from the other one so I stop confusing them in my tracker. :)

Gábor Hojtsy’s picture

For me, even $page['#title'] does not seem to work in its simplest form: #2070055: drupal_set_title() is deprecated

YesCT’s picture

I was looking at the patch in #78 that was committed in #81 to see if it had a test that was using #title to override a title, as the example in the issue summary example #3 in #2076085: Resolve the need for a 'title callback' using the route . But the test seems to set a title, not override one. @dawehner pointed out forms are different. And that #2068437: _title does not work on _form is needed and has a test with a form.

#2070055: drupal_set_title() is deprecated (in config_translation) is marked postponed on 2068437.

I'll add it to the issue summary here as a follow-up to this issue.

pwolanin’s picture

@Gábor Hojtsy - can you post a snippet of what you are trying?

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

added followup to make forms work, and issue to add docs. added commit patch and comment number.

thedavidmeister’s picture

Priority: Critical » Normal
Status: Fixed » Needs work
-  // Construct page title
-  if (drupal_get_title()) {
+   // Construct the page title.
+  if (isset($variables['page']['#title'])) {
+    $head_title = array(
+      'title' => strip_tags($variables['page']['#title']),
+      'name' => String::checkPlain($site_config->get('name')),
+    );
+  }
+  elseif (drupal_get_title()) {
     $head_title = array(
       'title' => strip_tags(drupal_get_title()),
-      'name' => check_plain($site_name),
+      'name' => String::checkPlain($site_config->get('name')),
     );
   }
   else {
@@ -2889,7 +2903,6 @@ function template_preprocess_maintenance_page(&$variables) {
   $variables['site_name']         = (theme_get_setting('features.name') ? check_plain($site_name) : '');
   $variables['site_slogan']       = (theme_get_setting('features.slogan') ? filter_xss_admin($site_slogan) : '');
   $variables['tabs']              = '';
-  $variables['title']             = drupal_get_title();
 
   // Compile a list of classes that are going to be applied to the body element.
   $variables['attributes']['class'][] = 'maintenance-page';
@@ -2925,6 +2938,14 @@ function template_preprocess_maintenance_page(&$variables) {
   // be called when printed.
   $variables['styles'] = new RenderWrapper('drupal_get_css', array($css));
   $variables['scripts'] = new RenderWrapper('drupal_get_js');
+
+  // Allow the page to define a title.
+  if (isset($variables['page']['#title'])) {
+    $variables['title'] = $variables['page']['#title'];
+  }
+  if (!isset($variables['title'])) {
+    $variables['title'] = drupal_get_title();
+  }

Why was this added to template_preprocess_maintenance_page()?

$variables['page'] can't exist there at all AFAICS. hook_theme() only allows 'content' for maintenance pages. system_region_list($GLOBALS['theme']) for core provides the following regions:

Stark:
- sidebar_first
- sidebar_second
- content
- header
- footer
- highlighted
- help
- page_top
- page_bottom

Seven:
- content
- help
- page_top
- page_bottom
- sidebar_first

Bartik:
- header
- help
- page_top
- page_bottom
- highlighted
- featured
- content
- sidebar_first
- sidebar_second
- triptych_first
- triptych_middle
- triptych_last
- footer_firstcolumn
- footer_secondcolumn
- footer_thirdcolumn
- footer_fourthcolumn
- footer

dawehner’s picture

Status: Needs work » Fixed
star-szr’s picture

Priority: Normal » Critical

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

Anonymous’s picture

Issue summary: View changes

trying to note which are follow-ups and not just "related"