Big Local Task Conversion

Tagging

Shameless tagging and cross-linking: #2100073: Convert local_actions to the new local action plugins

Some work on that.

FYI, #2091691: Convert test non-form page callbacks to routes and controllers had to include the local task for content_translation

Fixed a couple of bugs.

More fixes.

+++ b/core/includes/menu.inc
@@ -2335,6 +2335,15 @@ function menu_get_local_actions() {
+  // Allow modules to dynamically add further tasks.
+  $module_handler = \Drupal::moduleHandler();
+  foreach ($module_handler->getImplementations('menu_local_tasks') as $module) {
+    $function = $module . '_menu_local_tasks';
+    $function($links, $route_name);
+  }
+  // Allow modules to alter local tasks.
+  $module_handler->alter('menu_local_tasks', $links, $route_name);
   return $links['actions'];

So in local actions, we dynamically generate local_tasks? Additional you can just call invokeAll

I just c/p'd from menu_local_tasks().

This is actually a nasty hack, and should be removed. I'll see what I can do.

marking #2100213: take local tasks out of hook_menu so it uses comment.local_tasks.yml with the recommended route naming conventions dup.

• The item for node/% is needed at least for books
• Many local task removables did not added the titles to routes.

This removes the menu.inc hack and the weirdness in custom_block.

I must have hit a key while switching through open files in my IDE...

Sad enough #2100073: Convert local_actions to the new local action plugins has the same remaining test failure.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Menu/LocalAction/CustomBlockAddLocalAction.php
@@ -0,0 +1,30 @@
+    if ($request->attributes->has('theme_name')) {
+      $options['query']['theme'] = $request->attributes->get('theme_name');

+++ b/core/modules/block/block.routing.yml
@@ -31,9 +31,11 @@ block.admin_display:
-  path: 'admin/structure/block/list/{theme_name}'
+  path: 'admin/structure/block/list/{theme}'

That will need a fix

More fixes

Some more fixes.

Back to one failure.

This is actually a bogus test. We *want* the title to be "Manage fields", it was just being ignored previously.

Now that this actually passes, we need to clean up the patch. There is stuff commented out and inline comments abound.

It may be a long shot, but it would be SO NICE to have this done by alpha4. (next Friday)

Easy :-)

+++ b/core/modules/block/custom_block/custom_block.module
@@ -115,14 +88,13 @@ function custom_block_menu() {
+    'type' => MENU_DEFAULT_LOCAL_TASK, // Not a local task. Breadcrumb management.
...
+    'type' => MENU_DEFAULT_LOCAL_TASK, // Not a local task. Breadcrumb management.

+++ b/core/modules/block/tests/Drupal/block/Tests/Menu/BlockLocalTasksTest.php
@@ -0,0 +1,73 @@
+    $this->markTestIncomplete(
+      'This test has not been implemented yet. list_theme() and Drupal::config() calls fail.'
+    );
+    //$this->assertLocalTasks('block.admin_edit', array(array('block.admin_edit')));
...
+    $this->markTestIncomplete(
+      'This test has not been implemented yet. list_theme() and Drupal::config() calls fail.'
+    );
+    //
+//    $this->assertLocalTasks($route, array(
+//      0 => array('aggregator.category_view', 'aggregator.categorize_feed_form', 'aggregator.feed_configure'),
+//    ));
...
+      // theme_list()

+++ b/core/modules/config/config.module
@@ -64,37 +64,29 @@ function config_menu() {
+  #this page doesn't really exist. It was an old task that collapsed up.

+++ b/core/modules/node/node.module
@@ -999,7 +999,7 @@ function node_menu() {
+    'type' => MENU_DEFAULT_LOCAL_TASK, // Entity base

+++ b/core/modules/user/tests/Drupal/user/Tests/Menu/UserLocalTasksTest.php
@@ -0,0 +1,101 @@
+//  /**
+//   * Test local task existence.
+//   *
+//   * @dataProvider getUserAdminRoutes
+//   */
+//  public function tesstUserAdminLocalTasks() {
+//  }
+//
+//  public function getUserAdminRoutes() {
+//    return array(
+//      array('user.page', 'user.register', 'user.pass'),
+//    );
+//  }
+//

This needs to be addressed.

• Removed the docs ... i think we have to clean them up later anyway.
• Replace the non existing page with the other title/description as this is the stuff which is shown anyway on /admin
• Fixed the remaining tests.

Opened a follow up: #2109287: Replace list_themes() with a service.

I don't see why the variable name change $theme_name to $theme is needed for this patch.

This is some discrepancy in how the cache tags are written. Should it be array('local_task' => 1) or array('local_task' => TRUE)?

Looks like a couple places we could remove MENU_VISIBLE_IN_BREADCRUMB entries from hook_menu?

There's a change in the ViewSubscriber to look at _title from the request - should it also handle the _title_callback?

For the 'theme_name' switch, code like ThemeAccessCheck is expecting the theme name to be {theme}, and that's a reasonable thing to standardize on.

This converts some more tab, fixes the missing Views UI listing of fields, reverts some minor changes to local tasks to 8.x (no need to change them), and removes cases where the description field was incorrectly copied to the YAML.

Some more tweaks to move things out of hook_menu and define needed tabs.

Also, _access_mode: 'ALL' is now the default, right?

This fixes the 2 fails in #47.

+++ b/core/modules/block/custom_block/custom_block.local_tasks.yml
@@ -0,0 +1,29 @@
+custom_block.type_edit_tab:

+++ b/core/modules/language/language.local_tasks.yml
@@ -0,0 +1,19 @@
+system.date_format_language_overview_tab:
...
+language.admin_overview_tab:
...
+language.negotiation_tab:
...
+language.edit_tab:

+++ b/core/modules/system/system.local_tasks.yml
@@ -12,3 +12,55 @@ system.site_information_settings_tab:
+system.date_format_list_tab:

Most other conversions in this patch don't use the _tab suffix. This is kind of a horrible idea from my point of view as this makes it appear to be more magic.

I've seen _tab in local tasks before... for example system.local_tasks.yml

system.rss_feeds_settings_tab:
  route_name: system.rss_feeds_settings
  title: Settings
  tab_root_id: system.rss_feeds_settings_tab

system.site_maintenance_mode_tab:
  route_name: system.site_maintenance_mode
  title: Settings
  tab_root_id: system.site_maintenance_mode_tab

system.site_information_settings_tab:
  route_name: system.site_information_settings
  title: Settings
  tab_root_id: system.site_information_settings_tab

and user.local_tasks.yml
and views_ui.local_tasks.yml
etc.

ok, I'll make a new patch taking out the _tab from the local tasks, and also do an update to the issue summary

1. +++ b/core/modules/action/action.module
   @@ -53,26 +53,6 @@ function action_menu() {
   -  $items['admin/config/system/actions/add'] = array(
   ...
   -  $items['admin/config/system/actions/configure'] = array(
   ...
   -  $items['admin/config/system/actions/configure/%/delete'] = array(
   

   these are not moved to local tasks, so where did these move to? maybe they are just left over from another conversion?

   add is in routing, but what was the breadcrumb thing for? are we just taking those out?

   configure/% and configure/%/delete are already in routing.yml

   core/modules/action/action.routing.yml
   16: path: '/admin/config/system/actions/configure/{action}'
   23: path: '/admin/config/system/actions/configure/{action}/delete'

2. +++ b/core/modules/action/tests/Drupal/action/Tests/Menu/ActionLocalTasksTest.php
   @@ -0,0 +1,39 @@
   + * Contains \Drupal\action\Tests\Menu\ActionLocalTasksTest
   

   nit: some need a period to make it a sentence. https://drupal.org/node/1354#file

3. +++ b/core/modules/action/tests/Drupal/action/Tests/Menu/ActionLocalTasksTest.php
   @@ -0,0 +1,39 @@
   +   * Test local task existence.
   

   some function one liners needs third person verbs.

   "For most functions (see exceptions below), summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list.""

4. +++ b/core/modules/aggregator/aggregator.local_tasks.yml
   @@ -0,0 +1,36 @@
   +aggregator.admin_settings:
   +  route_name: aggregator.admin_settings
   +  title: 'Settings'
   +  weight: 100
   +  tab_root_id: aggregator.admin_overview
   ...
   +aggregator.category_edit:
   +  route_name: aggregator.category_edit
   +  tab_root_id: aggregator.category_view
   +  title: Configure
   

   I repeat to myself, order does not matter. But I do notice that usually title comes before tab_root_id

5. +++ b/core/modules/aggregator/tests/Drupal/aggregator/Tests/Menu/AggregatorLocalTasksTest.php
   @@ -0,0 +1,103 @@
   +  }
   +}
   

   also nit: fixing some classes to have newline before closing brace.

6. +++ b/core/modules/block/custom_block/custom_block.module
   @@ -44,69 +44,19 @@ function custom_block_menu_local_tasks(&$data, $route_name) {
   -  $items['admin/structure/block/custom-blocks/types/add'] = array(
   ...
   -  $items['admin/structure/block/custom-blocks/manage/%custom_block_type/edit'] = array(
   ...
   -  $items['block/add'] = array(
   ...
   -  $items['block/add/%custom_block_type'] = array(
   

   Where did these move to?

7. +++ b/core/modules/block/tests/Drupal/block/Tests/Menu/BlockLocalTasksTest.php
   @@ -0,0 +1,94 @@
   +  /**
   +   * @{inheritdoc}
   +   */
   +  public function setUp() {
   

   https://drupal.org/node/325974
   "There is no PHPDoc on this function since it is an inherited method."

   but movement on #338403: Use {@inheritdoc} on all class methods (including tests) might help.

8. +++ b/core/modules/config/config.local_tasks.yml
   @@ -0,0 +1,22 @@
   +config.export:
   +  route_name: config.export
   +  tab_root_id: config.sync
   +  title: Export
   
   +++ b/core/modules/config/config.module
   @@ -62,39 +62,25 @@ function config_file_download($uri) {
      $items['admin/config/development/configuration/export'] = array(
        'title' => 'Export',
        'description' => 'Export your site configuration',
        'route_name' => 'config.export',
   -    'type' => MENU_LOCAL_TASK,
   -    'weight' => 1,
      );
   

   why not take out the whole array item?

9. +++ b/core/modules/config/config.local_tasks.yml
   @@ -0,0 +1,22 @@
   +# todo I'm not sure why there are two imports. is there history here?
   +config.sync_import:
   

   noting.

10. +++ b/core/modules/system/system.local_tasks.yml
    @@ -12,3 +12,55 @@ system.site_information_settings_tab:
    +  title: 'List'
    +  tab_root_id: system.themes_page
    +
    +system.theme_settings:
    +  route_name: system.theme_settings
    +  title: 'Settings'
    +  tab_root_id: system.themes_page
    +  weight: 100
    +
    +system.theme_settings_global:
    

    I think the newlines are usually used to separate groups, grouped by root_id.

11. +++ b/core/modules/views_ui/views_ui.local_tasks.yml
    @@ -1,22 +1,27 @@
    -  title: Settings
    +  title: 'Settings'
    

    these are out of scope changes. I dont think we have a standard for one word titles and the use of quotes. also, elsewhere in this patch, some are added without the quotes, so no need to add these here.

12. +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php
    @@ -0,0 +1,135 @@
    + * @todo help do access checking.
    + * @todo help with url building, title and other data checks.
    

    are these todo's for this patch, or should they be separate issues?

13. +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php
    @@ -0,0 +1,135 @@
    +    $manager->setCacheBackend($cache_backend, $language_manager, 'local_task');
    
    +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.php
    @@ -166,7 +166,7 @@ public function testGetLocalTaskForRouteWithEmptyCache() {
    -      ->with('local_task:en:menu_local_task_test_tasks_view', $expected_set, CacheBackendInterface::CACHE_PERMANENT, array('local_task'));
    +      ->with('local_task:en:menu_local_task_test_tasks_view', $expected_set, CacheBackendInterface::CACHE_PERMANENT, array('local_task' => 1));
    
    @@ -254,7 +254,7 @@ protected function setupLocalTaskManager() {
    -    $this->manager->setCacheBackend($this->cacheBackend, $language_manager, 'local_task');
    +    $this->manager->setCacheBackend($this->cacheBackend, $language_manager, 'local_task', array('local_task' => 1));
    

    in one spot no cache_tags are set.

    in another, they are added.

    why?

used template and listed related issues. listed remaining tasks.

these are not moved to local tasks, so where did these move to? maybe they are just left over from another conversion?

In Drupal 7 the default breadcrumb was built using the menu system. MENU_IN_BREADCRUMB is a entry in the menu system which appears in the BREADCRUMB
but not as menu item nor local task etc. As the BREADCRUMB is now built based upon the path and not the menu system, these entries are not needed anymore.

The other example is %/delete which was not marked as something so it is MENU_NORMAL_ITEM aka a menu link entity. It does though not make sense to
have a default menu link with a placeholder as you can't show that anywhere. A menu link entity needs static variables.

some function one liners needs third person verbs.

"For most functions (see exceptions below), summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list.""

Just linking to a discussion about documentation of tests methods. https://drupal.org/node/2108785

Where did these move to?

Some of these indeed had a wrong change. I went through the full patch and removed all of this accidentally changes.

in one spot no cache_tags are set.

in another, they are added.
why?

There is indeed no real reason to not to, though all these tests don't deal with actual caching anyway. Fixed it.

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php
@@ -97,7 +97,7 @@ protected function getLocalTaskManager($modules, $route_name, $route_params) {
-    $manager->setCacheBackend($cache_backend, $language_manager, 'local_task');
+    $manager->setCacheBackend($cache_backend, $language_manager, 'local_task', array('local_task' => TRUE));

=> TRUE or => 1

@pwolanin brought up this same thing in #44

noting updated as of comment

Good catch. I also changed the place where I coped it from.

@dawehner - I think using something like "_tab" or something to distinguish the plugin ID from the route name is helpful.

I don't see why that would make it look like it has any magic? To me it's the opposite - naming it the same as the route name is way more confusing if you are just looking at the file and trying to understand what each string actually means.

There are other places in the ViewSubscriber which should use the title resolver, though this will be fixed by #2068471: Normalize Controller/View-listener behavior with a Page object

Shame on me.

I realize there's more to be converted to use the title resolver, but glad to see us at least covering the new use.

With that, plus thorough review by YesCT and removal of a lot of hook_menu cruft I think this is looking ready.

Where are all the menu descriptions gone?

Where are all the menu descriptions gone?

Menu descriptions are just shown on menu links, which are not touched by this patch.

I was actually wondering the same thing about descriptions. I asked in irc the other day, and didn't get an answer... today @dawehner and I discussed it in irc, and now I have checked.

+++ b/core/modules/config/config.module
@@ -64,37 +64,8 @@ function config_menu() {
-  $items['admin/config/development/configuration/import'] = array(
...
-    'description' => 'Import configuration for your site',

I checked, for example here. And before the patch there is no description used there anyway. (Not on the page, and not on hover.)

So I think that these descriptions where never used anywhere anyway. So it is ok to remove them.

re #56 9,

+++ b/core/modules/config/config.local_tasks.yml
@@ -0,0 +1,22 @@
+# todo I'm not sure why there are two imports. is there history here?
+config.sync_import:

#2099363: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7) will probably handle that @todo question.

update long outdated summary.

took out the @todo question about importing, that will be covered by the config issue.
and linked in the issue for the other todo.

I checked several more pages like forums, config.
There are some small differences, like there is no-longer a admin/config/development/configuration/sync since that tab is now the root it is just admin/config/development/configuration
and
on the languages add page, there are no tabs for list, detection and selection. But that is probably ok, as it does still get the language in the breadcrumb which takes people back to where the list tab would.

One of the reasons I'm really interested in getting this in soon, is it fixes the problem that tabs are missing from the node pages. For example:

oh crud. I was hoping to show how this is needed to solve this... but it shows a problem also.

Before:
one edit tab that leads to: node/1?entity=1
after:
first edit tab that leads to: node/1/edit
second edit tab leads to: node/1?entity=1 (which is not actually an edit page)

Also, the translate for node 1.. does not have all the tabs it should (like view, outline, the correct edit tab, etc.)

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -358,8 +358,8 @@ public function submit(array $form, array &$form_state) {
-    drupal_set_title(t('Preview comment'), PASS_THROUGH);
...
+    $form_state['comment_preview']['#title'] = $this->t('Preview comment');

Collision with #2102437: Remove drupal_set_title in comment module controllers and different implementation

x-ref #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation was marked as a dup of this issue

Didn't make it :(

The problem is the following: the content translation local tasks assume that the route name matches with the local task plugin ID
in order to set the tab_root_id. As @neclimdul suggested before, we should use the route name here, which is more save, though here is a fix without that change.

Therefore we needed also proper $entity_type parameters in the entity_test routes as well as actual implemented local tasks for entity_test.

This also fixes the content translation integration test.

There's issue #1834002: Configuration delete operations are all over the place that could help to minimize the patch size here by unifying local tasks for content entities

We dont even know where to start on that delete operations issue for content yet. So I dont think we should wait on it.

Looks like the next thing here is to investigate the fails.

looking at the 1st set of exceptions, it seems as though there is an empty string or NULL for $root_id which means someplace there is such a value for $task_info['tab_root_id']

trying to find the problem locally, I'm seeing it first on path /block/1/translations

The code in \Drupal\content_translation\Plugin\Derivative\ContentTranslationLocalTasks looks pretty problematic - I think changing to groupin gto be route is critical to make this more reliable.

This also seems to be fallout from re-deifining the path in the entity info rather than referencing a route:

$path = '/' . preg_replace('/%(.*)/', '{$1}', $entity_info['menu_base_path']);

Checking regular expressions on the route provider caused some issues, so I removed the regex on custom block, see #2116111: When looking up a route based on its path (pattern with slugs) we don't need to apply regex validation or other processing as a followup.

This also adds a couple of tests.

I'm not sure that's the correct interdiff.

From IRC, the relevant change to stop the test fail is:

+++ b/core/modules/block/custom_block/custom_block.routing.yml
@@ -33,7 +33,6 @@ custom_block.edit:
     _entity_form: 'custom_block.edit'
   requirements:
     _entity_access: 'custom_block.update'
-    custom_block: \d+

This time with the proper interdiff.

This blocks a half a dozen things, let's get it in!

Rerolling!

I removed the conflict:

config.sync:
path: '/admin/config/development/configuration/sync'
defaults:
_form: '\Drupal\config\Form\ConfigSync'
_title: 'Synchronize'
requirements:
_permission: 'synchronize configuration'

from core/modules/config/config.routing.yml

Missed the _title that was recently added.

Great stuff! @dawehner pointed out elsewhere that the $route_name + '_tab' naming is not to be assumed anymore for default local tasks. The arguments around the _tab suffix died in/after #63, but the existing tabs that used that naming scheme were left as-is. It would be great to have a followup then to clean up the existing ones so there is consistency? :) NOT in the scope of this patch IMHO, looking forward to the commit :)

Committed/pushed to 8.x, thanks!

FYI working on patch for config_translation deducted from the content_translation procedural alter + static create + alter method approach from the patch at #2119497: Default local tasks are not to be assumed $route_name . '_tab'. This seems to be the kind of code people need if they want to alter a tab in where they don't know the tab name of the tab root.

FYI working on patch for config_translation deducted from the content_translation procedural alter + static create + alter method approach from the patch at #2119497: Default local tasks are not to be assumed $route_name . '_tab'. This seems to be the kind of code people need if they want to alter a tab in where they don't know the tab name of the tab root.

Indeed we need the same procedure in content_translation, field_ui as well as views.

Maybe we could provide some common base class, but I am not sure whether these altering in by path is a common usecase for other modules, what do you think?

Sounds like 4 core modules is common enough? :)

We need to get rid of root and use the route name where they appear to group them

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/Derivative/ContentTranslationLocalTasks.php
@@ -62,31 +73,72 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+   * @return bool|string
+   *   Returns the local task ID of the parent task, otherwise return FALSE.
+   */
+  protected function getTaskFromRoute($route_name, &$local_tasks) {
+    $local_task = FALSE;
+    foreach ($local_tasks as $plugin_id => $local_task) {
+      if ($local_task['route_name'] == $route_name) {
+        $local_task = $plugin_id;
+        break;
+      }
+    }
+
+    return $local_task;
+  }

I blindly copy-pasted this function for use in #2119497: Default local tasks are not to be assumed $route_name . '_tab' (in config translation) and assumed it would work. Then got very surprised I was getting arrays as return values. Then looked closer, and see the $local task variable is FALSE, then gets assigned an array from foreach, then if the route matches, it will be a string. If no tab matches, you get the array back though :)

May be a followup issue or a followup patch here.

This issue has caused PHPUnit to start printing out info.yml files...

PHPUnit 3.7.21 by Sebastian Bergmann.

Configuration read from /Volumes/devdisk/dev/drupal/core/phpunit.xml.dist

...............................................................  63 / 988 (  6%)
............................................................... 126 / 988 ( 12%)
............................................................... 189 / 988 ( 19%)
............................................................... 252 / 988 ( 25%)
............................................................... 315 / 988 ( 31%)
............................................................... 378 / 988 ( 38%)
............................................................... 441 / 988 ( 44%)
............................................................... 504 / 988 ( 51%)
............................................................... 567 / 988 ( 57%)
............................................................... 630 / 988 ( 63%)
............................................................... 693 / 988 ( 70%)
............................................................... 756 / 988 ( 76%)
..........................................name: Actions
type: module
description: 'Perform tasks on specific events triggered within the system.'
package: Core
version: VERSION
core: 8.x
configure: action.admin
.name: Aggregator
type: module
description: 'Aggregates syndicated content (RSS, RDF, and Atom feeds) from external sources.'
package: Core
version: VERSION
core: 8.x
configure: aggregator.admin_settings
dependencies:
  - file
...............name: Block
type: module
description: 'Controls the visual building blocks a page is constructed with. Blocks are boxes of content rendered into an area, or region, of a web page.'
package: Core
version: VERSION
core: 8.x
configure: block.admin_display
..... 819 / 988 ( 82%)
name: Book
type: module
description: 'Allows users to create and organize related content in an outline.'
package: Core
version: VERSION
core: 8.x
dependencies:
  - menu_link
  - node
configure: book.settings
name: Node
type: module
description: 'Allows content to be submitted to the site and displayed on pages.'
package: Core
version: VERSION
core: 8.x
configure: node.overview_types
.........name: 'Configuration Manager'
type: module
description: 'Allows administrators to manage configuration changes.'
package: Core
version: VERSION
core: 8.x
configure: config.sync
...name: 'Content Translation'
type: module
description: 'Allows users to translate content entities.'
dependencies:
  - language
package: Multilingual
version: VERSION
core: 8.x
configure: language.content_settings_page
....................name: 'Interface Translation'
type: module
description: 'Translates the built-in user interface.'
package: Multilingual
version: VERSION
core: 8.x
dependencies:
  - language
  - file
...................name: Shortcut
type: module
description: 'Allows users to manage customizable lists of shortcut links.'
package: Core
version: VERSION
core: 8.x
dependencies:
  - menu_link
configure: shortcut.set_admin
name: User
type: module
description: 'Manages the user registration and login system.'
package: Core
version: VERSION
core: 8.x
required: true
configure: user.admin_index
............ 882 / 988 ( 89%)
.....name: Taxonomy
type: module
description: 'Enables the categorization of content.'
package: Core
version: VERSION
core: 8.x
dependencies:
  - options
configure: taxonomy.vocabulary_list
.......................................................... 945 / 988 ( 95%)

Here's where --strict helps in phpunit.

So I tracked it down to ModuleHandler::load(), the include_once is printing out the file contents somehow... Not sure why.

In LocalTaskIntegrationTest::getLocalTaskManager(), why aren't we mocking the ModuleHandler? We can cheat on the module dirs, and this just sidesteps the whole thing.

That's not going to work, thanks content_translation :)

neclimdul found the actual bug here.

The constant needs to move since block.module loads after BlockFormControllerTest.

This is a nice way to get in this kind of small bits done :)

Here's where --strict helps in phpunit.

+1

Also, once again, I remind folks that we could mock entire filesystems from a dataprovider with vfsStream. :-) #2095037: Add vfsStream as a dependency in composer.json.

Also, once again, I remind folks that we could mock entire filesystems from a dataprovider with vfsStream. :-)

Well, the point here is to have integration tests not unit tests ... so mocking everything don't really help here :)

Well, my fix from #104 did not make it into the RTBC patch. Do people agree its an issue? The existing tests obviously don't prove it, but IMHO it should be fixed :)

Oh right.

Here is a roll with #108 and #104 combined, so I put my keyboard where my mouth is. IMHO still RTBC :)

+1

+1 for #114 because it applies and makes the yaml output go away. Also works with --strict.

Committed 270708d and pushed to 8.x. Thanks!

The patch caused #2121461: \Drupal\Core\EventSubscriber\ViewSubscriber::onHtml() does not allow controllers to return string content.

added related issue for one of the @todos

@eliza411
Wait, you just reuploaded the patch from https://drupal.org/comment/7998221#comment-7998221 ... is that maybe just a bug on d.o?

added #2135777: Follow-up [meta] local task conversion, move out of hook_menu() and to local_tasks.yml

ah, re #121, it wasn't reuploaded, just hidden and then shown.
d.o d7 upgrade: #2125741: The comment marking a file as hidden shows the file in the comment as though you uploaded it

CustomBlockLocalTasksTest ended up in the wrong folder :)

Expected: core/modules/block/custom_block/tests/Drupal/custom_block/Tests/Menu/CustomBlockLocalTasksTest.php
Actual:   core/modules/block/custom_block/tests/Drupal/custom_blocks/Tests/Menu/CustomBlockLocalTasksTest.php

PHPUnit does not care about PSR-0, it just scans the file contents. This is why this did not break.

I'm going to fix this as part of #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 with format-patch.

Created #2271801: CustomBlockLocalTasksTest is located in wrong directory