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
TODO
Remaining tasks
API changes
Related Issues
#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
#2067931: Use the new title method on views pages
Comment | File | Size | Author |
---|---|---|---|
#52 | menu-plugins-1981644-52.patch | 18.36 KB | pwolanin |
#52 | 1981644-incremental-41-52.patch.txt | 2.92 KB | pwolanin |
#41 | menu-plugins-1981644-41.patch | 17.88 KB | pwolanin |
#41 | 1981644-incremental-39-41.patch.txt | 9.54 KB | pwolanin |
#39 | drupal-1981644-39.patch | 14.59 KB | dawehner |
Comments
Comment #1
donquixote CreditAttribution: donquixote commentedTitles (and title callback) were also very useful for breadcrumbs and tabs in D7.
Esp, for breadcrumb items that are not in any menu.
Yes, they were also useful for the actual page title, but this could be solved with drupal_set_title() instead. And in D8 it might be possible to build it into the page controller, instead of the menu / routing system.
Comment #2
dawehnerYeah I though of a title() method which get's the parameter injected like the central routing method does. This would allow you to get the arguments from the url.
Comment #3
donquixote CreditAttribution: donquixote commentedA method on which object?
Ideally I want this independent of the current page, so you can evaluate the method for arbitrary paths.
(typically this would be for tabs or breadcrumb items)
Comment #4
dawehnerWell, so you propose a separate title controller? In general having proper injected controllers, doesn't seem to much of an overhead, even you just use it for the title.
Comment #5
donquixote CreditAttribution: donquixote commentedIt could trigger a bunch of dependency loading?
Also in D7 it used to be possible to swap out a page callback independent of the title and access callback..
Comment #6
alexpottTagging :)
Comment #7
alexpottI tried doing the following so that the path is not changed from
aggregator/sources/%aggregator_feed
toaggregator/sources/%
And whilst this worked to make the upcasting occur it broke other things :(
Comment #8
Crell CreditAttribution: Crell commentedWe discussed this on a recent WSCCI meeting Hangout. I believe what we ended up with as a recommendation was:
1) Controllers SHOULD return not a string or a render array but a SCOTCH HtmlFragment (or whatever it's called). That has a body and title and needed CSS/JS and link elements and whatever else. Title gets pulled from there for display.
2) IF the HtmlFragment has no title (i.e., $fragment->title() returns empty string), then fall back to the title for the current menu link (defined, um, somehow).
3) IF there is no current menu link, then there is no title. Well you should have returned one from your content!
4) Menu items continue to get their title info from hook_menu() as now, mostly because we don't have time/energy/ideas for doing anything else at the moment.
Since really, the content title and the menu title are NOT the same thing; them being the same thing is, nominally, an edge case. That separates that concept with fallback logic.
The main downside of this approach is that it is SCOTCH-dependent, and HtmlFragments are not yet in core. :-(
Comment #9
dawehnerIt seems to be a real overhead to require a second object for most cases. This object might also need dependency injection etc. as you don't know what a title needs.
Comment #10
dawehnerAdd back the tag.
Comment #11
catchThis looks like a near-duplicate of #1830588: [META] remove drupal_set_title() and drupal_get_title().
Comment #12
donquixote CreditAttribution: donquixote commented@Crell (#8):
As mentioned in #1, I see the main purpose of title / title callback not for the page title, but to easily determine a default title for a link that is not the current page.
Breadcrumbs, tabs, menu links, contextual links, etc: These are (or should be) independent subsystems, but it does make a lot of sense to let them reuse the title information defined in one place.
So, while I am perfectly happy with SCOTCH for the page title, but this leaves the question of page-independent link titles.
Ok, if hook_menu() stays in the game, then the other points become obsolete. We can continue to use menu_get_item() to determine a title for any link on the planet.
The only thing is that originally everybody wanted to kill hook_menu(), and at least some links have already moved away from it.
Comment #13
donquixote CreditAttribution: donquixote commentedThat's the new equivalent of a "callback": A class with one public method to implement the callback, plus DIC.
Although it would be imaginable to use one object (service) for multiple title callbacks.
Comment #14
vijaycs85Can we say it is major as it is blocking at least 41 items (which have 'title callback') and almost all items that have title in menu item?
Comment #15
donquixote CreditAttribution: donquixote commentedDo I understand correctly that anything that uses
the new Symfony routing systemsf-based route declaration can not at the same time use hook_menu() ? And thus, won't have a title or title callback?This does not sound like an acceptable solution.
I can see these options (not mutually exclusive):
I don't think anyone wants this.
I had the impression from the other thread that this would be the most preferred direction to take.
Personally I don't really like it, because it is too easy in contrib development to forget the title, or that routes and titles get out of sync. And DX suffers because you need to maintain two places instead of one.
(*) TItle callback could be a method on the page object, or on a separate service, or a callback (function), either of these can be allowed. (see #4 and #13)
(We could go beyond this and allow more than one (conditional) title callback per route, or allow the title callback to be altered after the route is loaded (so different entity bundles can have different title callback). But the basic questions and options remain the same.)
Comment #16
catchFor the actual page title #1830588: [META] remove drupal_set_title() and drupal_get_title() might be enough - just don't use the routing system at all and have the controller return it.
Comment #17
dawehnerI'm wondering what have been discussed about that on the meeting on saturday. Would be great to see some logs about that.
Comment #18
tim.plunkettThere was a google doc with awesome notes that I can't find. xjm you're our only hope!
Comment #19
andypostProbably we have meta for that #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff
PS: updated subbary with related links
Comment #20
tim.plunketthttps://docs.google.com/document/d/1sgxD0l2OM91EqTekXryhdaXQn_n4zPUnLgq_... is the doc in question it could use a summary.
Comment #21
donquixote CreditAttribution: donquixote commentedHad a look at the docs.
And again we are only talking about page title, thus ignoring the essential half of the problem :(
drupal_set_title() needs to go away, and be replaced by something returned by the page controller. I am all for that.
But this should be discussed in its own issue.
This issue *should* be about the "multiple-purpose navigation link title" which can be used in breadcrumbs, menus, tabs, any other navigational elements that might be added in contrib, for paths other than the current page.
Breadcrumbs in D8 core:
In #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service (breadcrumbs in D8 core) we still follow the quite boring D7 approach: One component (usually one that represents the current request, or some aspect of it) builds the entire breadcrumb.
The problem: The active breadcrumb builder needs to know not only the nature of the current request, but also of all parents it wants to have in the breadcrumb.
I do support this, because we lack the time to do something else *in core*, but I also believe that D8 contrib can do much better.
Breadcrumbs in D8 contrib:
Here is how it should work:
- For every path/route/etc (*) you can determine a parent path/route. Starting from the current page, you find the parent, the parent of the parent, etc, until you end at the front page. This gives you a reverse trail of breadcrumb paths.
- Different plugins (with weight) can negotiate what is the parent for a given path.
- Fallback for parent-finding: Chop off the last fragment of the path, consider the result as the parent path.
- For every path, you can find a (navigational) title.
- Different plugins can negotiate on the title.
- Fallback for the title is the one produced by title/title_callback, or whatever this becomes in the future. (**)
- Note: Navigational title typically wants to be shorter than the page title.
- paths + titles make up the breadcrumb.
(*) In D7 it was easy to just use a path (string) for this. Maybe in D8 we need to switch to an array or route + parameters.
(**) I hope this step explains why I desperately want to keep the title / title_callback, or have it replaced by something equivalent.
It also shows how a generic title / title_callback can be used by a completely new system in contrib.
Comment #22
pwolanin CreditAttribution: pwolanin commenteddiscussing this broadly with tim.plunkett in terms of #2023795: REGRESSION: hook_local_actions doesn't use title callback.
We shoudl have a consistent way to handle this for pages, local actions, and local tasks (tabs)
Comment #23
dawehnerI think pages and all kind of links are kind of different so I would suggest to keep them separate at least for now.
For local actions/local tasks and maybe even menu links in general I think we want to have an additional parameter on the route definition
which allows you to specify how to get the title.
As all new systems (like local actions/tasks) have the route name available we can also easily instanciate the route controllers,
once we are rendering the titles. We could enhance that by allowing simple strings on here like 'Custom title' (without the colons),
which then would just be put into t() later.
Comment #24
pwolanin CreditAttribution: pwolanin commented@dawehner - I think that's good for pages, but indeed - we still need a framework for handling titles for local actions and local tasks (tabs) within the new routing system. the current D8 decouples them from the page title, which I think is useful, but makes this a bit harder.
Comment #25
tim.plunkett@pwolanin what @dawehner is suggesting is only for local actions/tasks, the page title is being handled in #1830588: [META] remove drupal_set_title() and drupal_get_title()
Comment #26
dawehnerHere is a basic patch which adds menu title generation to local actions.
To be honest this is maybe not the proper place for the patch.
Comment #27
tim.plunkettI think that belongs in #2023795: REGRESSION: hook_local_actions doesn't use title callback, but all of these issues are tied together. Adding to the issue summary.
Comment #27.0
tim.plunkettrelated
Comment #29
pwolanin CreditAttribution: pwolanin commented@dawehner - I think the title for local actions, etc should be decoupled from the route title? Let's just use the route title for the page itself?
Comment #30
pwolanin CreditAttribution: pwolanin commentedPer discussion with dawehner, this patch allows the route to define a default title, and allows the local action to override it.
Comment #31
pwolanin CreditAttribution: pwolanin commentedNew plan with dawehner and fubhy - we'll define a plugin type each for local actions and local tasks (tabs). Annotation based discover. The methods on these plugins will handle the generation of the path and title.
Comment #32
pwolanin CreditAttribution: pwolanin commentedNote, the initial implementation of this may define the api (plugin type) but not be optimized, while we can later compile the result to make it faster.
Comment #33
pwolanin CreditAttribution: pwolanin commentedok, here's a start at a plugin patch for feedback on direction.
Works locally for the views UI action. Need to convert the test
Comment #34
pwolanin CreditAttribution: pwolanin commentedthis fixes up the test code too.
Note so far the patch is just switching from hook to plugin discovery.
The more interesting pieces will come as we create an instance and use it for e.g. generating the path and title.
Comment #36
pwolanin CreditAttribution: pwolanin commentedThe plugins aren't being constructed right so this patch is broken - posting for dawehner to see the direction
Comment #37
pwolanin CreditAttribution: pwolanin commentedok, fixed the factory and some other bugs.
Comment #38
dawehnerHere is some feedback.
it would be also possible to just use "ContainerFactoryPluginInterface" instead of the plugin base class, but yeah it is not important at all.
It seems to make sense to do this in logic in the constructor, as is more then just getting some services.
In order to get arbitrary injection of the request parameters getTitle/getPath should not be called directly but going through the Controller Resolver.
Comment #39
dawehnerThis should allow to use the controller resolver.
Comment #41
pwolanin CreditAttribution: pwolanin commentedHere's a patch that uses the url generator. Had to apply a trim or the leading '/' causes two action links.
Also fixes up other hook implementations that were not included above.
Comment #42
tim.plunkettWhat's the reasoning for a plugin that will only ever be an annotation?
If we're just using it for metadata and never instantiate it, it should be YAML.
I'm guessing I'm missing the justification for a full plugin.
Comment #43
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - so the base implementation is not just annotation. Methods on the class are invoked by the manager. The interesting use cases come in overriding the base plugin implementation to e.g. provide a dynamic title or path.
So, while most of these plugins will look like just annotation, this is restoring the dynamic title & path features where needed. Also, we can next copy this same kind of implementation for local tasks (tabs).
Comment #45
tim.plunkettAh, I missed this part... Hmm.
Comment #46
dawehnerOne problem which exists at the moment is the following: As the parameter converters converts stuff, the plain values, like the UID
does not exist anymore on the request object. fubhy is working on changing the way how the converter works and he has a plan to adress that, so the upcasted values are stored different.
On the meantime we could basically say that we don't support attributes in order to move forward. Once the parameter converter patch got it, this will automatically work fine.
Comment #47
pwolanin CreditAttribution: pwolanin commented@dawehner - yes, I think we should see this as laying the groundwork for dynamic titles/paths working once that other patch is in.
Comment #48
pwolanin CreditAttribution: pwolanin commented#41: menu-plugins-1981644-41.patch queued for re-testing.
Comment #49
pwolanin CreditAttribution: pwolanin commentedI cannot reproduce the failures locally.
Comment #51
pwolanin CreditAttribution: pwolanin commentedThese failures are being caused by a core bug with the URLgenerator when Drupal is installed in a subdir
Comment #52
pwolanin CreditAttribution: pwolanin commentednew patch includes a work-around for bug noted at #2031353: URLgenerator broken for Drupal installed in a subdirectory - doesn't have a way to get a Drupal path
Comment #53
dawehnerOn the longrun you want to pass in the request object into getPath().
Let's go with
to allow people to understand it.
Should be "Return" instead of Retun
Then we get the request we passed in and use $request->attributes->all() into the url generate method.
I am wondering why getPath is not part of the interface.
To be honest, I think we should throw an exception, as there is clearly something going wrong.
Comment #54
pwolanin CreditAttribution: pwolanin commentedmoving the local actions patch to #2031473: Convert menu local actions to plugins so that we can generate dynamic titles and paths
Comment #55
pwolanin CreditAttribution: pwolanin commentedmoved patch for local actions to #2031473: Convert menu local actions to plugins so that we can generate dynamic titles and paths
Comment #55.0
pwolanin CreditAttribution: pwolanin commentedadding more issues
Comment #56
Crell CreditAttribution: Crell commentedI only sort of understand what's going on here, but...
appearsOn
And all of these should be protected. Why do we have public properties?
It sounds like that would only happen if someone screwed up their $local_action object. That's an exceptional case, so throw an exception or fatal.
Actually, is getPath() part of the MenuLocalActionInterface definition? If so, then it should never not be callable as we'd get a fatal just calling this method. If not... it should be for us to use it here. :-)
Comment #57
tim.plunkettThose are the annotation properties.
This class is never used directly by developers, but rather turned into an array by plugin discovery (
$manager->getDefinition()
)Comment #58
dawehnerAdded a new issue as this one is totally full of unrelated patches :) #2032535: Resolve 'title' using the route and render array
Comment #59
amateescu CreditAttribution: amateescu commentedI think another issue with the exact same title and summary is a little confusing..
Comment #60
xjmI think the idea was to close this as a dupe in favor of the un-confusing issue?
Comment #60.0
xjmadd node/2031473
Comment #60.1
dawehner.