Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment 0
Problem/Motivation
Currently a lot (quite some) of routes are missing an _title property. This wasn't a problem when hook_menu() was still used to set the title but this is no longer the case for hook_menu() entries with placeholders.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#21 | title-2194823-21.patch | 13.87 KB | YesCT |
#15 | interdiff.txt | 3.25 KB | dawehner |
#15 | title-2194823-15.patch | 13.87 KB | dawehner |
#10 | title-2194823-10.patch | 13.06 KB | dawehner |
#8 | title-2194823-8.patch | 13.1 KB | dawehner |
Comments
Comment #1
andypostComment #2
dawehnerWent through all the _content and _form ones. Note: many of the ones which are mixing use #title instead.
Comment #4
dawehnerFixed the remaining failure.
Comment #5
YesCT CreditAttribution: YesCT commentedneeded a reroll. https://drupal.org/contributor-tasks/reroll
conflict:
I took out the _title that was added in #2193477: No page title on admin/structure/block/list/{theme}
And used the callback instead.
Comment #7
MixologicAppears as though the tests need to be updated to check that the title is correct.
#2193477: No page title on admin/structure/block/list/{theme} added an assertion that expected 'Block Layout'
http://drupalcode.org/project/drupal.git/blobdiff/e3dfcf9371822d8fd7d73e...
Seems like that check should be removed from that test, and handled more appropriately in #2194843: [meta] Write tests to make sure that pages have titles
Comment #8
dawehnerWell, Block layout seemds to be the prefered way.
Comment #9
star-szrTagging for reroll.
Comment #10
dawehnerSure.
Comment #11
YesCT CreditAttribution: YesCT commentedstill applies
Comment #12
dawehner10: title-2194823-10.patch queued for re-testing.
Comment #13
tim.plunkett@var what?
Is it safe to assume $theme will exist? Shame there is no dedicated method for getting a $theme's info that throws an exception if it doesn't exist
How is this not a dupe?
hahah, nice :)
Comment #14
znerol CreditAttribution: znerol commentedCurrently does not apply.
I'm unable to find the spot where this is initialized (and used). Also misses docs.
Great!
Oops, nice catch.
Comment #15
dawehnerThank you for the reviews!
Comment #16
znerol CreditAttribution: znerol commentedI see #13 and #14 being addressed in #15. In order to figure out whether the patch covers all the router-entries I did the following:
find . -name '*.routing.yml' | xargs cat | python filter-routes.py
(python script attached for reference). I've examined the remaining 28 routes and found two (ignoring those in theme test module) which neither have '_title' on the route nor they specify '#title' in the returned render-array:If I'm not completely mistaken there is no reason that those two routes need to set a title. The first one handles Ajax form updates while the second renders the preview in views. Therefore I'm confident that this is ready.
Comment #17
webchickA couple of DX/maintenance-related comments...
IMO we should just make this a required property and throw an error when it's not provided. Otherwise we'll only re-introduce these missing titles over time, and need to continue committing patches like this that require careful manual inspection like custom python scripts. :) While those two routes don't strictly need a title, it doesn't hurt anything if they provide one either, and then that makes the API simpler for people.
It would save a lot of annoying typing if _entity_form just did a standard drupal_set_title() (whatever that is in D8 now) for CRUD which individual routing definitions could override if they wanted to. Is that on the table at all?
Comment #18
tim.plunkettWe could absolutely enforce this, but I'm not sure if we have consensus there.
Ideally we'd continue that discussion in a new issue.
See #2239299: Form errors should only be set during validation and #2241735: Throw an exception when form errors are set outside of validation for a similar approach. We fixed it everywhere in one, and are enforcing it in another.
As for the second point, I don't think that this is really truly translatable:
$build['#title'] = $this->t(ucfirst($operation));
Which is what you're implying.
Comment #19
tim.plunkettOpened #2244845: Consider enforcing that all non-controller page routes have a title for the follow-up.
Comment #20
YesCT CreditAttribution: YesCT commentedmanually checked, does need a reroll. doing this now.
Comment #21
YesCT CreditAttribution: YesCT commentedautomatic 3-way merge. no conflicts.
( local: phpunit tests: OK (4823 tests, 9536 assertions) )
Comment #22
webchickOk, makes sense to move enforcement to another issue, thanks.
Committed and pushed to 8.x. Thanks!