Ensure that all content/page routes have an _title

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.

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

Tagging for reroll.

still applies

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

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.

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.

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?

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.

Opened #2244845: Consider enforcing that all non-controller page routes have a title for the follow-up.

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

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

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

Committed and pushed to 8.x. Thanks!