Commits

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:

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

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.

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

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.

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

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?

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.

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

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.

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.

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.

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.

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

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.

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.

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

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

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.

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

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.

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

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

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?