Files: 
CommentFileSizeAuthor
#114 local-task-2102125-108-plus-104.patch16.18 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,107 pass(es).
[ View ]
#108 local-task-2102125-108.patch15.1 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,494 pass(es).
[ View ]
#107 local-task-2102125-107.patch3.75 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,428 pass(es).
[ View ]
#106 local-task-2102125-106.patch1.13 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,162 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#104 parent-task.patch1.08 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,546 pass(es).
[ View ]
#97 local-task-2102125-97.patch112.7 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,080 pass(es).
[ View ]
#97 interdiff.txt443 bytestim.plunkett
#96 local_tasks-2102125-96.patch112.67 KBTim Bozeman
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#93 interdiff.txt6.83 KBdawehner
#91 local_tasks-2102125-90.patch112.67 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,553 pass(es).
[ View ]
#91 interdiff.txt9.49 KBdawehner
#86 local_tasks-2102125-86.patch105.6 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 60,281 pass(es), 0 fail(s), and 15 exception(s).
[ View ]
#86 interdiff.txt9.49 KBdawehner
#84 local_task-2102125-84.patch105.36 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#84 interdiff.txt12.24 KBdawehner
#81 drupal8.menu-system.2102125-81.patch95.89 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.menu-system.2102125-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#81 interdiff.txt913 bytesneclimdul
#78 drupal8.menu-system.2102125-77.patch95.88 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 59,179 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#78 interdiff.txt6.55 KBneclimdul
#75 local_tasks-2102125-75.patch89.32 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 59,212 pass(es).
[ View ]
#75 interdiff-66-75.txt1.11 KBYesCT
#75 node-tabs-convert.png244.09 KBYesCT
#72 no-descriptions.png358.38 KBYesCT
#66 local_tasks-2102125-66.patch89.39 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,344 pass(es).
[ View ]
#66 interdiff.txt460 bytesdawehner
#64 local_tasks-2102125-64.patch88.94 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#64 interdiff.txt3.56 KBdawehner
#61 local_task-2102125-61.patch88.25 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,636 pass(es).
[ View ]
#61 interdiff.txt1.98 KBdawehner
#58 local_task-2102125-58.patch87.71 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#58 interdiff.txt3.2 KBdawehner
#56 local_task-2102125-56.patch87.81 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 58,596 pass(es), 18 fail(s), and 3 exception(s).
[ View ]
#56 interdiff-49-56.txt20.49 KBYesCT
#49 local_task-2102125-49.patch88.63 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,912 pass(es), 18 fail(s), and 3 exception(s).
[ View ]
#49 2102125-47-49.increment.txt1015 bytespwolanin
#47 local_task-2102125-47.patch88.18 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,606 pass(es), 20 fail(s), and 3 exception(s).
[ View ]
#47 2102125-46-47.increment.txt4.61 KBpwolanin
#46 local_task-2102125-46.patch86.6 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,922 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#46 2102125-41-46.increment.txt11.2 KBpwolanin
#41 local_task-2102125-41.patch81.94 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,560 pass(es).
[ View ]
#41 interdiff.txt13.73 KBdawehner
#38 local-tasks-2102125-38.patch80.5 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,910 pass(es).
[ View ]
#38 interdiff.txt743 bytestim.plunkett
#34 local-tasks-2102125-34.patch79.78 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,175 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#34 interdiff.txt623 bytestim.plunkett
#32 local_task-2102125-30.patch81.76 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,829 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#32 interdiff.txt10.31 KBdawehner
#30 2102125-30.patch83.35 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,458 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#30 interdiff-2102125-30.txt7.92 KBdamiankloip
#28 local_task-2102125-28.patch75.49 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,418 pass(es), 45 fail(s), and 4 exception(s).
[ View ]
#28 interdiff.txt3.86 KBtim.plunkett
#25 local_task-2102125-25.patch74.85 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,400 pass(es), 31 fail(s), and 5 exception(s).
[ View ]
#25 interdiff.txt4.18 KBdawehner
#23 local-tasks-2102125-23.patch72.8 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,729 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt516 bytestim.plunkett
#21 local-tasks-2102125-21.patch72.8 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#21 interdiff.txt3.1 KBtim.plunkett
#20 interdiff.txt3.32 KBdawehner
#20 local_task-2102125-18.patch72.62 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,479 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#15 local-tasks-2102125-15.patch71.69 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,041 pass(es), 66 fail(s), and 14 exception(s).
[ View ]
#15 interdiff.txt5.69 KBtim.plunkett
#13 interdiff.txt3.92 KBdawehner
#13 local_task-2102125-13.patch66.51 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,647 pass(es), 106 fail(s), and 24 exception(s).
[ View ]
#10 drupal8.menu-system.2102125-10.patch66.2 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 57,043 pass(es), 107 fail(s), and 24 exception(s).
[ View ]
#8 drupal8.menu-system.2102125-8.patch68.15 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] 57,045 pass(es), 108 fail(s), and 24 exception(s).
[ View ]
#5 local_task-2102125-5.patch68.69 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#5 interdiff.txt4.26 KBdawehner
#2 interdiff-2102125.txt2.37 KBneclimdul
#2 drupal8.menu-system.2102125-2.patch64.43 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
big_local_tasks_conversion.patch64.23 KBneclimdul
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, big_local_tasks_conversion.patch, failed testing.

neclimdul’s picture

StatusFileSize
new64.43 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
new2.37 KB

noticed user/login had the wrong local tasks. fix and tests.

Tests aren't going to be able to login because user/1 throws an exception that was part of the reason I wanted to post this. Disappointing I can't see the other tests but that's life.

Crell’s picture

Issue tags:+WSCCI

Tagging

dawehner’s picture

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.26 KB
new68.69 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Some work on that.

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-5.patch, failed testing.

tim.plunkett’s picture

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

neclimdul’s picture

Status:Needs work» Needs review
StatusFileSize
new68.15 KB
FAILED: [[SimpleTest]]: [MySQL] 57,045 pass(es), 108 fail(s), and 24 exception(s).
[ View ]

#2103145: ParameterConverter mangles raw values is the fix for #5. applied and merged conflicts with HEAD. Committed local tasks where are related to entities so I hadn't converted them yet anyways(including what tim mentioned)

Status:Needs review» Needs work

The last submitted patch, drupal8.menu-system.2102125-8.patch, failed testing.

neclimdul’s picture

StatusFileSize
new66.2 KB
FAILED: [[SimpleTest]]: [MySQL] 57,043 pass(es), 107 fail(s), and 24 exception(s).
[ View ]

reroll with enhancer fix that was commited.

pwolanin’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.menu-system.2102125-10.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new66.51 KB
FAILED: [[SimpleTest]]: [MySQL] 56,647 pass(es), 106 fail(s), and 24 exception(s).
[ View ]
new3.92 KB

Fixed a couple of bugs.

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-13.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new5.69 KB
new71.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,041 pass(es), 66 fail(s), and 14 exception(s).
[ View ]

More fixes.

Status:Needs review» Needs work

The last submitted patch, local-tasks-2102125-15.patch, failed testing.

dawehner’s picture

+++ 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

tim.plunkett’s picture

Assigned:neclimdul» tim.plunkett

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.

dawehner’s picture

Assigned:tim.plunkett» neclimdul
Status:Needs work» Needs review
StatusFileSize
new72.62 KB
FAILED: [[SimpleTest]]: [MySQL] 58,479 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
new3.32 KB
  • The item for node/% is needed at least for books
  • Many local task removables did not added the titles to routes.
tim.plunkett’s picture

Assigned:neclimdul» tim.plunkett
StatusFileSize
new3.1 KB
new72.8 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, local-tasks-2102125-21.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new516 bytes
new72.8 KB
FAILED: [[SimpleTest]]: [MySQL] 58,729 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, local-tasks-2102125-23.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.18 KB
new74.85 KB
FAILED: [[SimpleTest]]: [MySQL] 58,400 pass(es), 31 fail(s), and 5 exception(s).
[ View ]

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

tim.plunkett’s picture

+++ 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

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-25.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new3.86 KB
new75.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,418 pass(es), 45 fail(s), and 4 exception(s).
[ View ]

More fixes

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-28.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new7.92 KB
new83.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,458 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Some more fixes.

Status:Needs review» Needs work

The last submitted patch, 2102125-30.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new10.31 KB
new81.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,829 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Back to one failure.

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-30.patch, failed testing.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new623 bytes
new79.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,175 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

webchick’s picture

Issue tags:+8.x-alpha4

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

damiankloip’s picture

Easy :-)

Status:Needs review» Needs work

The last submitted patch, local-tasks-2102125-34.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new743 bytes
new80.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,910 pass(es).
[ View ]
+++ 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.

Status:Needs review» Needs work
Issue tags:-MenuSystemRevamp, -WSCCI, -8.x-alpha4

The last submitted patch, local-tasks-2102125-38.patch, failed testing.

neclimdul’s picture

Status:Needs work» Needs review
Issue tags:+MenuSystemRevamp, +WSCCI, +8.x-alpha4

#38: local-tasks-2102125-38.patch queued for re-testing.

dawehner’s picture

StatusFileSize
new13.73 KB
new81.94 KB
PASSED: [[SimpleTest]]: [MySQL] 58,560 pass(es).
[ View ]
  • 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.
neclimdul’s picture

+++ b/core/modules/block/tests/Drupal/block/Tests/Menu/BlockLocalTasksTest.php
@@ -34,40 +31,64 @@ public static function getInfo() {
+
+namespace {
+  if (!function_exists('list_themes')) {
+    function list_themes() {
+      $themes = array();
+      $themes['test_a'] = (object) array(
+        'status' => 0,
+      );
+      $themes['test_b'] = (object) array(
...
+        'info' => array(
+          'name' => 'test_b',
+        ),
+      );
+      $themes['test_c'] = (object) array(
+        'status' => 1,
+        'info' => array(
+          'name' => 'test_c',
+        ),
+      );
+
+      return $themes;
+    }
+  }

When I ran this things blew up all over but seems to work now. We should probably @todo this and I don't know if we can still but it'd be nice if we could open a follow up to put this in a service for testing if nothing else. As soon as something else wants to phpunit test something with listing themes we're dead.

dawehner’s picture

pwolanin’s picture

Status:Needs review» Needs work

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?

tim.plunkett’s picture

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.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new11.2 KB
new86.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,922 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

pwolanin’s picture

StatusFileSize
new4.61 KB
new88.18 KB
FAILED: [[SimpleTest]]: [MySQL] 58,606 pass(es), 20 fail(s), and 3 exception(s).
[ View ]

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

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

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-47.patch, failed testing.

pwolanin’s picture

StatusFileSize
new1015 bytes
new88.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,912 pass(es), 18 fail(s), and 3 exception(s).
[ View ]

This fixes the 2 fails in #47.

tim.plunkett’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-49.patch, failed testing.

dawehner’s picture

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

YesCT’s picture

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.

neclimdul’s picture

I disagree with they're use. If we add them here or don't touch them here or what ever, they'll currently get removed in #2107531: Improve DX of local task YAML definitions.

YesCT’s picture

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

YesCT’s picture

Status:Needs work» Needs review
StatusFileSize
new20.49 KB
new87.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,596 pass(es), 18 fail(s), and 3 exception(s).
[ View ]
  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?

YesCT’s picture

Issue summary:View changes

used template and listed related issues. listed remaining tasks.

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-56.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
new87.71 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-58.patch, failed testing.

YesCT’s picture

+++ 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

YesCT’s picture

Issue summary:View changes

noting updated as of comment

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.98 KB
new88.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,636 pass(es).
[ View ]

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

neclimdul’s picture

I don't understand the point of supplying cache tags. I consciously didn't supply them because we aren't testing that and the only reason we're adding a cache backend is so it will return empty and trigger the parsing code we're trying to test. If cache tags where required for that it would actually be a bug but more importantly, there is literally no implementation to use them because we are mocking a empty implementation.

pwolanin’s picture

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

dawehner’s picture

StatusFileSize
new3.56 KB
new88.94 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, local_tasks-2102125-64.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new460 bytes
new89.39 KB
PASSED: [[SimpleTest]]: [MySQL] 59,344 pass(es).
[ View ]

Shame on me.

Status:Needs review» Needs work
Issue tags:-MenuSystemRevamp, -WSCCI, -8.x-alpha4

The last submitted patch, local_tasks-2102125-66.patch, failed testing.

neclimdul’s picture

Status:Needs work» Needs review
Issue tags:+MenuSystemRevamp, +WSCCI, +8.x-alpha4

#66: local_tasks-2102125-66.patch queued for re-testing.

neclimdul’s picture

Issue summary:View changes

added issue related to the breadcrumbs

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

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.

hass’s picture

Status:Reviewed & tested by the community» Needs work

Where are all the menu descriptions gone?

dawehner’s picture

Status:Needs work» Reviewed & tested by the community

Where are all the menu descriptions gone?

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

YesCT’s picture

StatusFileSize
new358.38 KB

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.
no-descriptions.png

YesCT’s picture

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.

YesCT’s picture

Issue summary:View changes

update long outdated summary.

neclimdul’s picture

Status:Reviewed & tested by the community» Needs review

There are a couple todos in the patch but tim is fixing one in #2099363: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7) and the others are about expanding testing which is currently sufficient for this patch but should be expanded to replace some web tests.

YesCT’s picture

StatusFileSize
new244.09 KB
new1.11 KB
new89.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,212 pass(es).
[ View ]

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:

node-tabs-convert.png

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

YesCT’s picture

Status:Needs review» Needs work
andypost’s picture

+++ 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

neclimdul’s picture

Status:Needs work» Needs review
StatusFileSize
new6.55 KB
new95.88 KB
FAILED: [[SimpleTest]]: [MySQL] 59,179 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

Don't know what's going on with the entity argument(?entity=1) but it seems to be an existing problem with router weirdness. The extra tab was a hack tim added to get tests passing until this issue happened and we missed it. Updated with test to fix it.

New follow up task for changes made to support testing #2112575: Add a DerivativeBase in the Core namespace with t() as a helper method.

Status:Needs review» Needs work

The last submitted patch, drupal8.menu-system.2102125-77.patch, failed testing.

neclimdul’s picture

Don't have time to finish this up this evening I don't think but that edit task is apparently for when editing under a language so in some form it'll need to get added back probably with some sort of requirement for its visibility on the route that doesn't exist currently.

neclimdul’s picture

StatusFileSize
new913 bytes
new95.89 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.menu-system.2102125-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Still haven't sorted out the Edit tab. This fixes the language failures. I'm ashamed.

Leaving as NW rather then adding another failed return because there's nothing interesting to see. ContentTranslationTestBase fails around that edit tab still.

markcarver’s picture

tim.plunkett’s picture

Issue tags:-8.x-alpha4

Didn't make it :(

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new12.24 KB
new105.36 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, local_task-2102125-84.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new9.49 KB
new105.6 KB
FAILED: [[SimpleTest]]: [MySQL] 60,281 pass(es), 0 fail(s), and 15 exception(s).
[ View ]

This also fixes the content translation integration test.

Status:Needs review» Needs work

The last submitted patch, local_tasks-2102125-86.patch, failed testing.

andypost’s picture

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

YesCT’s picture

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.

pwolanin’s picture

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']);
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new9.49 KB
new112.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,553 pass(es).
[ View ]

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.

pwolanin’s picture

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+
dawehner’s picture

StatusFileSize
new6.83 KB

This time with the proper interdiff.

tim.plunkett’s picture

Priority:Normal» Major
Status:Needs review» Reviewed & tested by the community

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

Tim Bozeman’s picture

Assigned:Unassigned» Tim Bozeman
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Rerolling!

Tim Bozeman’s picture

Assigned:Tim Bozeman» Unassigned
Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new112.67 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new443 bytes
new112.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,080 pass(es).
[ View ]

Missed the _title that was recently added.

Gábor Hojtsy’s picture

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

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

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.

dawehner’s picture

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?

Gábor Hojtsy’s picture

Sounds like 4 core modules is common enough? :)

pwolanin’s picture

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

Gábor Hojtsy’s picture

Status:Fixed» Needs review
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 59,546 pass(es).
[ View ]
+++ 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.

alexpott’s picture

Status:Needs review» Needs work

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] 59,162 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

tim.plunkett’s picture

StatusFileSize
new3.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,428 pass(es).
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new15.1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,494 pass(es).
[ View ]

neclimdul found the actual bug here.

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

Mile23’s picture

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.

dawehner’s picture

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

Gábor Hojtsy’s picture

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

dawehner’s picture

Status:Reviewed & tested by the community» Needs work

Oh right.

Gábor Hojtsy’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new16.18 KB
PASSED: [[SimpleTest]]: [MySQL] 59,107 pass(es).
[ View ]

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

dawehner’s picture

+1

Mile23’s picture

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

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

Xano’s picture

Xano’s picture

Issue summary:View changes

added related issue for one of the @todos

eliza411’s picture

Issue summary:View changes
eliza411’s picture

dawehner’s picture

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

xjm’s picture

YesCT’s picture

YesCT’s picture

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

Status:Fixed» Closed (fixed)

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

donquixote’s picture

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.

sun’s picture