Follow-up to #2032535: Resolve 'title' using the route and render array to add _title_callback also which was omitted in that issue.
Problem/Motivation
As seen on #1974408: Convert aggregator_page_source() to a Controller, routes defined via the route system does not have yet the concept of titles. Titles
are not only needed for menu links, but also for the actual title on the page, so things which has been MENU_CALLBACK's before also have titles.
Proposed resolution
Title for a page will be determined by a hierarchy of sources.
Menu links and breadcrumb building will first look for _title_callback; if that's found, it will use the return STRING from that callable. If not found, it will use the string in _title. If that's not found, there is no title. If _title_callback returns a string, it is assumed to be already translated.
HtmlPageController will support the same logic (callback wins over static), but will continue to also support the #title property on the content array as a final override. That way there's not an additional API break with what we just put in. :-) However, the use of #title will be discouraged unless you want the title to be different between the page and the link. This parallels calling drupal_set_title() from within a page callback in D7, which was always inferior to using "title callback".
The solution consists of several points - the 1) _title and 3) #title portions are committed already:
- Route definitions can contain a _title property which will be used for static titles
route_name: pattern: '/route-pattern' defaults: _content: ... _title: 'Static title'
- 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'
- If you need an even more dynamic title or override you can return it as part of the main render array. This is like drupal_set_title() in 7 and is discouraged:
class controllerClass { class controllerMethod() { return array( '#title' => 'Dynamic title' . mt_rand(0, 10), ); } }
API changes
Add _title_callback to _title on the route and #title on the render array for setting the page title.
Related Issues
#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
Comment | File | Size | Author |
---|---|---|---|
#47 | title-2076085-47.patch | 13.49 KB | dawehner |
#47 | interdiff.txt | 2.31 KB | dawehner |
#45 | title_callback-2076085-44.patch | 13.01 KB | pwolanin |
#45 | 2076085-43-44.increment.txt | 1.94 KB | pwolanin |
#43 | title_callback-2076085-43.patch | 12.87 KB | pwolanin |
Comments
Comment #1
xjmComment #2
pwolanin CreditAttribution: pwolanin commentedre-posting patch title-callback-2032535-109.patch from the #2032535: Resolve 'title' using the route and render array with a new name.
Comment #2.0
xjmUpdated issue summary.
Comment #3
dawehnerJust wondering whether we should name it '_title_controller' as this is exactly what it is.
Comment #4
pwolanin CreditAttribution: pwolanin commented@dawehner - I think "callback" is easier to understand, even if it's not quite as accurate in terms of the code flow and the use of the controller resolver.
Comment #5
Crell CreditAttribution: Crell commentedAgreed on callback or callable, not controller. We overload the word controller too much as is. :-)
As noted in the previous issue, this needs at least one test and at least one implementation. Let's do just that, then get this in.
Comment #6
damiankloip CreditAttribution: damiankloip commentedTests, and one aggregator conversion of _title option, _title_callback conversion todo.
Comment #7
larowlanuser_page would be a good candidate as it will help with fails on #2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.
Comment #8
jibranAs per #6.
Comment #9
damiankloip CreditAttribution: damiankloip commentedHere is the conversion of the user/% page (now user_view route), I guess that's what you meant? As user_page just redirects to here, or login.
Comment #11
dawehnerCrell suggested at some issue to wait until #2068471: Normalize Controller/View-listener behavior with a Page object is in, though I doubt that rerolling that other issue would be really hard.
Comment #12
Crell CreditAttribution: Crell commentedThe conflicting changes here are contained enough that I can handle rerolling the HtmlPage issue if needs be, since I don't think HtmlPage is landing in the next 48 hours. :-)
Comment #13
damiankloip CreditAttribution: damiankloip commentedThis should fix those I think.
Comment #15
dawehnerLet's typehint in UserInterface, so we do have the getUsername method available.
Nice!!
Comment #16
Letharion CreditAttribution: Letharion commentedChanged to UserInterface as per #15.
Removing the actual callback user_view_page, not just it's reference in hook_menu. Unless someone objects to that, this patch covers #1987898: Convert user_view_page() to a new style controller, and that issue will be closed dup.
Comment #17
Letharion CreditAttribution: Letharion commentedIgnore the patch in #16.
Here's the real patch with an interdiff.
Comment #19
Letharion CreditAttribution: Letharion commentedWrong namespace on the userinterface. Trying again.
Comment #20
Letharion CreditAttribution: Letharion commentedComment #21
damiankloip CreditAttribution: damiankloip commentedYep, looks better to me. Let's see if we still have some of those remaining failures :)
Comment #22
dawehnerI guess at least for menu links we need the title callback to apply there as well?
Comment #23
pwolanin CreditAttribution: pwolanin commented@dawehner - good question. Do we want to support dynamic menu link titles in 8?
I think that's a bit of a Drupal WTF, since the title will be dynamic possibly until you edit the link and then it might stop being dynamic.
Comment #24
dawehner@pwolanin
It seems to be that we simply have to.
How else will we be able to translation node titles on menu links etc.
Comment #25
pwolanin CreditAttribution: pwolanin commenteddawehner - so, currently when you create a menu link to a node you can set a distinct title from the node title - only in book module do we expect them to be synchronized.
So... I'm not sure what's the right approach here - I might expect each menu link entity to have a language translation if needed.
Comment #27
dawehnerI asked gabor via IRC what the plans are for these translations.
Comment #28
pwolanin CreditAttribution: pwolanin commentedThis is pretty important for the breadcrumb patch, so let's get it done :)
Comment #29
damiankloip CreditAttribution: damiankloip commentedJust rerolled for now, What else do we need to get done here? Some more stuff to enable menu link translations to work? We decided in the other issue that the callback should return a translated string?
Comment #31
dawehnerWorking on a generic component.
Comment #32
dawehnerLet's see whether this causes the same amount of failures.
Comment #34
dawehnerComment #36
dawehnerJust a rerole.
Comment #38
dawehner#36: title_callback-2076085-36.patch queued for re-testing.
Comment #40
dawehnerThe remaining failures will disappear once #2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}. is in.
Comment #41
dawehnerCommented out the tests in order to fix it, these ones are removed in the path based breadcrumb builder anyway.
Comment #43
pwolanin CreditAttribution: pwolanin commentedThe last patch is missing the new TitleResolver class from patch #36.
Comment #44
pwolanin CreditAttribution: pwolanin commentedfor both _title_callback and _title I think we should either check that they are non-empty, or simply use getDefault(), since the value could be NULL which cuased problems on another issue when calling t().
Comment #45
pwolanin CreditAttribution: pwolanin commentedThis makes the change per discussion in IRC.
Comment #46
Crell CreditAttribution: Crell commentedShouldn't this have an interface?
thevalue, needs a space.
If there's a good reason we're doing this, it should be noted in a comment.
Those are all fairly minor nitpicks; once those are fixed this is RTBCable.
Comment #47
dawehnerKept the commented tests as they are out of scope to fix ... we won't support it anyway in the future in core.
Comment #48
pwolanin CreditAttribution: pwolanin commentedSeems TitleResolverInterface.php is in the patch, but not in the interdiff - check the patch not that file.
Also, discussed with dawhener using $this->translationManager->translate() instead of $this->t(). Using the full form makes as much or more sense because this will likely never be subclassed and it's never being applied to an actual string where having t() would help you discover a string that needs translation.
Setting to RTBC per Crell's comment above since the patches fixes his concerns.
Comment #49
pfrenssenAdding commented out code.
Comment #50
dawehnerWe do know that, but this is a test which will be removed by another issue, which though sadly is depending on this one. (#2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}.)
Comment #51
alexpottCommitted 47ec9cf and pushed to 8.x. Thanks!
Comment #52
pwolanin CreditAttribution: pwolanin commentedadded change notice: https://drupal.org/node/2095317
updated change notice: https://drupal.org/node/2067859
Comment #53
star-szrSmall follow-up to add 'title' back to aggregator_menu(): #2103285: Feed aggregator configuration missing menu title
Comment #54.0
(not verified) CreditAttribution: commentedupdate summary
Comment #55
joachim CreditAttribution: joachim commentedNeeds documentation: #2218311: Document _title_callback.