Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
menu system
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
25 Nov 2013 at 20:58 UTC
Updated:
29 Jul 2014 at 23:09 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
dawehnerThis is a starter
Comment #3
tim.plunkettSo, this will override all drupal_set_title calls. I fixed a couple for now...
Comment #5
catchNote there's #1830588: [META] remove drupal_set_title() and drupal_get_title() for removing drupal_set_title() calls - many of the sub-issues have patches and some are RTBC (I'll try to clean out the RTBC ones next time I'm going through the queue).
Comment #6
tim.plunkett@catch it seems very few of those actually address the problems here...
So BreadcrumbTest was still asserting that a menu link title could override a page title from hook_menu. But now that its a route title, it shouldn't be possible.
Comment #7
pwolanin commentedSo, I looked through a lot of the yml files and didn't see any obvious omissions and it's close to RTBC.
One trivial problem I saw was some inconsistency about quoting strings in the YAML files.
e.g.
vs.
while quoting is probably not required in any case, I think it makes for a clearer meaning, so this change quotes titles that were not quoted, and fixes in views_ui.routing.yml one _title with a mismatched quote mark.
Also, in core/modules/node/node.pages.inc the call to drupal_set_title() uses PASS_THROUGH but other calls do not. This needs to be checked - either that's wrong since the title will now be escaped, or other cases are emitting the title unsanitized.
Comment #8
dawehnerI know that the various drupal_set_title() issues really take care about escaping and not escaping. This should not be part of this patch, if possible, as it adds a quite big amount of complexity.
Comment #9
tim.plunkettI added Xss::filter() where appropriate in the patch. It passes, and leaves the rest of the drupal_set_titles to their respective issues. I only changed as much as I had to.
RTBC?
Comment #10
larowlanAdded related
Comment #11
pwolanin commented@tim.plunkett - there are a couple shifts directs from drupal_set_title() to using #title in the patch - possibly those should be omitted fro this patch. I agree the ones switching to title callback should be fine.
Comment #12
tim.plunkettIf you switch them back, tests will fail.
Comment #13
dawehnerAwesome work tim.
This blocks hook_menu(), so let's better get this in rather sooner than later and see how many remaining failures we still have.
Comment #14
webchickPatch seems fine and yay for killing hook_menu() and a critical both, but...
Was quite surprised about this. I figured it would be more of a 1:1 ratio, but instead it seems we're adding titles *far* more often in routes than we needed to do in the D7 version. Hm. :\
Especially for entities, it seems suboptimal to have to enter those titles by hand. I could see contrib ending up a smörgåsbord of "Create foo" and "Add foo" and "Add new foo" and whatnot. Is this worth discussing in a follow-up?
At any rate, committed and pushed to 8.x to move things along. Thanks!
Comment #15
pwolanin commented@webchick - I suspect many of the old hook_menu entries were not removed here, since we will kill them all later.
e.g. aggregator_menu() was still left with a lot of title entries.