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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

dawehner’s picture

Status: Active » Needs review
Parent issue: #2194843: [meta] Write tests to make sure that pages have titles »
FileSize
12.01 KB

Went through all the _content and _form ones. Note: many of the ones which are mixing use #title instead.

Status: Needs review » Needs work

The last submitted patch, 2: tittle-2194823-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.62 KB
2.06 KB

Fixed the remaining failure.

YesCT’s picture

needed a reroll. https://drupal.org/contributor-tasks/reroll

conflict:

block.admin_display_theme:
  path: 'admin/structure/block/list/{theme}'
  defaults:
    _content: '\Drupal\block\Controller\BlockListController::listing'
<<<<<<< HEAD
    _title: 'Block layout'
=======
    _title_callback: 'theme_handler:getName'
>>>>>>> 4
  requirements:
    _access_theme: 'TRUE'
    _permission: 'administer blocks'

I took out the _title that was added in #2193477: No page title on admin/structure/block/list/{theme}
And used the callback instead.

Status: Needs review » Needs work

The last submitted patch, 5: title-2194823-5.patch, failed testing.

Mixologic’s picture

Appears 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

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.1 KB

Well, Block layout seemds to be the prefered way.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.06 KB

Sure.

YesCT’s picture

Issue tags: -Needs reroll

still applies

dawehner’s picture

10: title-2194823-10.patch queued for re-testing.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
    @@ -110,6 +110,11 @@
    +   * @var
    

    @var what?

  2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -448,6 +448,14 @@ protected function resetSystem() {
    +    $themes = $this->listInfo();
    +    return String::checkPlain($themes[$theme]->info['name']);
    

    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

  3. +++ b/core/modules/block/lib/Drupal/block/Controller/BlockController.php
    @@ -47,4 +47,18 @@ public function demo($theme) {
    +  public function getTitle($theme) {
    +    $themes = $this->themeHandler->listInfo();
    +    return String::checkPlain($themes[$theme]->info['name']);
    +  }
    

    How is this not a dupe?

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -137,7 +137,7 @@ function testBreadCrumbs() {
    -      "admin/config/content/formats/manage/$format_id" => Unicode::ucfirst(Unicode::strtolower($format->name)),
    +      "admin/config/content/formats/manage/$format_id" => $format->label(),
    

    hahah, nice :)

znerol’s picture

Issue tags: +Needs reroll

Currently does not apply.

  1. +++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
    @@ -110,6 +110,11 @@
       /**
    +   * @var
    +   */
    +  protected $themeHandler;
    +
    

    I'm unable to find the spot where this is initialized (and used). Also misses docs.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
    @@ -137,7 +137,7 @@ function testBreadCrumbs() {
    -      "admin/config/content/formats/manage/$format_id" => Unicode::ucfirst(Unicode::strtolower($format->name)),
    +      "admin/config/content/formats/manage/$format_id" => $format->label(),
    

    Great!

  3. +++ b/core/modules/user/user.routing.yml
    @@ -110,7 +111,7 @@ user.role_delete:
    -    _title: 'Edit role'
    +    _title: 'Delete role'
    

    Oops, nice catch.

dawehner’s picture

FileSize
13.87 KB
3.25 KB

Thank you for the reviews!

znerol’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
569 bytes

I 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:

  • system.ajax
  • views_ui.preview

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

A couple of DX/maintenance-related comments...

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.

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.

+++ b/core/modules/action/action.routing.yml
@@ -24,6 +26,7 @@ action.delete:
     _entity_form: 'action.delete'
+    _title: 'Delete'

+++ b/core/modules/block/custom_block/custom_block.routing.yml
@@ -48,6 +49,7 @@ custom_block.delete:
     _entity_form: 'custom_block.delete'
+    _title: 'Delete'

+++ b/core/modules/node/node.routing.yml
@@ -56,6 +56,7 @@ node.delete_confirm:
     _entity_form: 'node.delete'
+    _title: 'Delete'

+++ b/core/modules/search/search.routing.yml
@@ -57,6 +57,7 @@ search.delete:
     _entity_form: 'search_page.delete'
+    _title: 'Delete'

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?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

YesCT’s picture

manually checked, does need a reroll. doing this now.

YesCT’s picture

Issue tags: -Needs reroll
FileSize
13.87 KB

automatic 3-way merge. no conflicts.
( local: phpunit tests: OK (4823 tests, 9536 assertions) )

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, makes sense to move enforcement to another issue, thanks.

Committed and pushed to 8.x. Thanks!

  • Commit ed38d9e on 8.x by webchick:
    Issue #2194823 by znerol, YesCT, dawehner: Ensure that all content/page...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.