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:
    <?php
    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)

Files: 
CommentFileSizeAuthor
#109 title-callback-2032535-109.patch1.3 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,500 pass(es).
[ View ]
#108 title-callback-2032535-108.patch1.22 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]
#101 title-2032535-101.patch657 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]
#78 2032535-title.patch18.85 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 58,090 pass(es).
[ View ]
#75 title-2032535-74.patch18.85 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title-2032535-74.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#71 title-2032535-71.patch19.33 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title-2032535-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#71 interdiff.txt1.18 KBdawehner
#62 title-2032535-62.patch19.48 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,977 pass(es).
[ View ]
#62 interdiff.txt1.09 KBdawehner
#54 title-2032535-54.patch19.52 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,876 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#54 interdiff.txt709 bytesdawehner
#52 drupal-2032535-52.patch19.52 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,152 pass(es).
[ View ]
#52 interdiff.txt4.34 KBdawehner
#48 title-2032535-48.patch20.08 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,952 pass(es).
[ View ]
#48 interdiff.txt608 bytesdawehner
#43 interdiff.txt1.41 KBdawehner
#43 title-2032535-43.patch20.13 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,853 pass(es).
[ View ]
#41 title-2032535-41.patch20.19 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,504 pass(es).
[ View ]
#38 title-2032535-38.patch20.56 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title-2032535-38.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#36 title-2032535-34.patch18.94 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#36 interdiff.txt3.9 KBdawehner
#33 title-2032535-33.patch16.91 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,325 pass(es).
[ View ]
#33 interdiff.txt745 bytesdawehner
#31 drupal-2032535-31.patch16.8 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,768 pass(es).
[ View ]
#31 interdiff.txt1.28 KBdawehner
#29 title-2032535-29.patch16.77 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,898 pass(es).
[ View ]
#29 interdiff.txt663 bytesdawehner
#27 title-2032535-27.patch16.77 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,284 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#27 interdiff.txt2.09 KBdawehner
#20 title-2032535-20.patch18.06 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,052 pass(es).
[ View ]
#20 interdiff.txt2.74 KBdawehner
#17 drupal-2032535-17.patch16.89 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,818 pass(es).
[ View ]
#15 drupal-2032535-15.patch16.56 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,775 pass(es).
[ View ]
#13 drupal-2032535-13.patch15.78 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#11 vdc-2032535-11.patch15.27 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#11 interdiff.txt9.24 KBdawehner
#7 2032535.routing.page-title.7.patch13.95 KBkatbailey
PASSED: [[SimpleTest]]: [MySQL] 57,299 pass(es).
[ View ]
#7 interdiff.txt1.17 KBkatbailey
#5 2032535.routing.page-title.5.patch13.85 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] 56,554 pass(es), 52 fail(s), and 5 exception(s).
[ View ]
#3 routing-2032535-3.patch4.75 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,092 pass(es), 31 fail(s), and 0 exception(s).
[ View ]
#1 drupal-2032535-1.patch897 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,711 pass(es).
[ View ]

Comments

StatusFileSize
new897 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,711 pass(es).
[ View ]

Maybe just this is already enough?

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.

StatusFileSize
new4.75 KB
FAILED: [[SimpleTest]]: [MySQL] 57,092 pass(es), 31 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new13.85 KB
FAILED: [[SimpleTest]]: [MySQL] 56,554 pass(es), 52 fail(s), and 5 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new13.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,299 pass(es).
[ View ]

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.

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.

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

StatusFileSize
new9.24 KB
new15.27 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new15.78 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Forgot the Title class.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new16.56 KB
PASSED: [[SimpleTest]]: [MySQL] 56,775 pass(es).
[ View ]

This time with even less missing files (TitleControllerInterface)

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

Status:Needs work» Needs review
StatusFileSize
new16.89 KB
PASSED: [[SimpleTest]]: [MySQL] 56,818 pass(es).
[ View ]

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

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

@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

StatusFileSize
new2.74 KB
new18.06 KB
PASSED: [[SimpleTest]]: [MySQL] 57,052 pass(es).
[ View ]

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

Tagging

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

TitleInterface--

<?php
+    // 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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new2.09 KB
new16.77 KB
FAILED: [[SimpleTest]]: [MySQL] 57,284 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This time without the title interface.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new663 bytes
new16.77 KB
PASSED: [[SimpleTest]]: [MySQL] 57,898 pass(es).
[ View ]

No wonder that this does not work.

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

StatusFileSize
new1.28 KB
new16.8 KB
PASSED: [[SimpleTest]]: [MySQL] 57,768 pass(es).
[ View ]

Yeah I agree.

<?php
+    $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?

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

Should be !isset().

StatusFileSize
new745 bytes
new16.91 KB
PASSED: [[SimpleTest]]: [MySQL] 57,325 pass(es).
[ View ]

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.

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

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

StatusFileSize
new3.9 KB
new18.94 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new20.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title-2032535-38.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Forgot the new interface

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.

Status:Needs work» Needs review
StatusFileSize
new20.19 KB
PASSED: [[SimpleTest]]: [MySQL] 57,504 pass(es).
[ View ]

Rerolled.

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

StatusFileSize
new20.13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,853 pass(es).
[ View ]
new1.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

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

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

+++ 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?

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.

StatusFileSize
new608 bytes
new20.08 KB
PASSED: [[SimpleTest]]: [MySQL] 57,952 pass(es).
[ View ]

So, this time with a patch.

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.

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

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

StatusFileSize
new4.34 KB
new19.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,152 pass(es).
[ View ]

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.

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

StatusFileSize
new709 bytes
new19.52 KB
FAILED: [[SimpleTest]]: [MySQL] 57,876 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

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

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

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

(And bah, bot!)

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
new19.48 KB
PASSED: [[SimpleTest]]: [MySQL] 57,977 pass(es).
[ View ]

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

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.

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.

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.

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.

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.

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

StatusFileSize
new1.18 KB
new19.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title-2032535-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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?

Status:Needs review» Reviewed & tested by the community

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

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new18.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch title-2032535-74.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new18.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,090 pass(es).
[ View ]

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

Issue summary:View changes

Updated issue summary.

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

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.

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.

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.

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

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

Looks good to me. Thanks!

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?)

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.

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

<?php
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 :)

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.

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

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.

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.

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.

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.

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

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

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

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

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.

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

Status:Active» Needs review
StatusFileSize
new657 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]

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

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.

Status:Needs review» Needs work

CNW based on #102.

?

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

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

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

Issue summary:View changes

update issue summary

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Title:[Docs needed] Resolve 'title/title callback' using the route and render arrayResolve 'title/title callback' using the route and render array
Status:Needs work» Needs review
StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]

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

StatusFileSize
new1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,500 pass(es).
[ View ]

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

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

Needs an example and a test.

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?

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

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

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.

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

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.

Issue summary:View changes

Updated issue summary.

Title:Resolve 'title/title callback' using the route and render arrayResolve '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. :)

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

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.

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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

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

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

Status:Needs work» Fixed

Priority:Normal» Critical

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

Issue summary:View changes

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