Updated: Comment 0

Problem/Motivation

If you have a local task which has upcasted parameters like a translate tab on some config entites
there is an "Error" shown as title.

Proposed resolution

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#28 local_task-2095139-26-28-interdiff.txt633 byteskfritsche
#28 local_task-2095139-28.patch7.97 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 59,029 pass(es).
[ View ]
#26 local_task-2095139-26.patch8 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,446 pass(es), 110 fail(s), and 25 exception(s).
[ View ]
#26 interdiff.txt2.99 KBdawehner
#25 local_task-20951390-25.patch7.24 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 59,108 pass(es).
[ View ]
#25 20951390-24-25.increment.txt1.03 KBpwolanin
#24 local_task-20951390-24.patch7.09 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#24 20951390-10-24.increment.txt4.09 KBpwolanin
#19 local_task-20951390-19.patch6.24 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,972 pass(es).
[ View ]
#19 local_task-20951390-17-interdiff.txt1.8 KBtstoeckler
#17 local_task-20951390-17.patch6.13 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 58,576 pass(es).
[ View ]
#17 interdiff-2095139-10-17.txt2.24 KBYesCT
#17 interdiff-2095139-14-17.txt7.45 KBYesCT
#14 local_task-20951390-10-14-interdiff.txt7.65 KBkfritsche
#14 local_task-20951390-14.patch9.85 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 58,965 pass(es).
[ View ]
#10 interdiff.txt621 bytesdawehner
#10 local_task-2095139-FAIL.patch4.23 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,564 pass(es), 10 fail(s), and 8 exception(s).
[ View ]
#10 local_task-20951390-10.patch5.9 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,852 pass(es).
[ View ]
#5 local_task-2095139-5.patch5.93 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,553 pass(es).
[ View ]
#1 local_task-2095139-1.patch994 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,028 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new994 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,028 pass(es).
[ View ]

.

We need to insure that attribute exists, at least, before calling a method on it.

Priority:Normal» Major

... this blocks many conversions.

StatusFileSize
new5.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,553 pass(es).
[ View ]

Here is a test.

Status:Needs review» Reviewed & tested by the community

looks good.

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LocalTasksTest.php
@@ -198,6 +198,34 @@ public function testPluginLocalTask() {
+    debug($entity->id());

Extra debug.

Also, can you upload the test-only in the next one

Status:Reviewed & tested by the community» Needs work

Issue tags:+blocker

Status:Needs work» Needs review
Issue tags:-blocker
StatusFileSize
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,852 pass(es).
[ View ]
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,564 pass(es), 10 fail(s), and 8 exception(s).
[ View ]
new621 bytes

There we go.

Issue tags:+blocker

Issuetag monster

Status:Needs review» Needs work
  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LocalTasksTest.php
    @@ -198,6 +198,33 @@ public function testPluginLocalTask() {
    +

    comment here about what testing for.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LocalTasksTest.php
    @@ -198,6 +198,33 @@ public function testPluginLocalTask() {
    +    $this->assertEqual(1, count($result), 'There are tabs active on both levels.');

    I think this one is checking to see if there is only one active tab.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LocalTasksTest.php
    @@ -198,6 +198,33 @@ public function testPluginLocalTask() {
    +    $this->assertEqual('upcasting sub1', (string) $result[0]->a, 'The settings tab is active.');

    I think this is seeing if sub1 is active (not if settings tab is active).

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LocalTasksTest.php
    @@ -198,6 +198,33 @@ public function testPluginLocalTask() {
    +    $this->drupalGet('menu-local-task-test-upcasting/1/sub2');

    a comment before this second section would be nice.

    I think it is:
    Switching to the second tab, sub2, and checking that sub1 does not still have the active class, by checking there is only one active tab and it is sub2.

  5. +++ b/core/modules/system/tests/modules/menu_test/menu_test.local_tasks.yml
    @@ -35,3 +35,20 @@ menu_local_task_test_tasks_settings_derived:
    +menu_local_task_test.placeholder_sub1:
    ...
    +menu_local_task_test_placeholder_sub2:

    why _ in the first placeholder sub1,
    but . in the second placeholder sub2.

    and then again for the upcasting.

    also, I thought the pattern was module_name.something_unique

    this should be menu_test.placeholder_sub1:

  6. +++ b/core/modules/system/tests/modules/menu_test/menu_test.routing.yml
    @@ -138,6 +138,20 @@ menu_test.local_task_test_placeholder_sub2:
    +menu_test.local_task_test_upcasting_sub1:

    wait. I think it was meant to have the dot over so the unique part is local_task_test_upcasting_sub1

@attrib is going to work on this.

[edit adding links to docs]
https://drupal.org/node/2044515

when the dot moved to just after the module name, the local task name was the same as the route, the local task tests failed. Let's try putting _tab on the end of the plugin in for the local tasks.

Assigned:Unassigned» kfritsche

working on it now.

Assigned:kfritsche» Unassigned
Status:Needs work» Needs review
StatusFileSize
new9.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,965 pass(es).
[ View ]
new7.65 KB

Changed the assert messages and added comments, see interdiff.

Like pointed out by YesCT we should use the correct pattern here. Also we couldn't named the tabs like the route name so I added "_tab":

menu_local_task_test_tasks_view:
menu_test.local_task_test_tasks_view_tab:

This introduced a lot of changes in menu_test.local_tasks.yml and I replaced the usages in LocalTasksTest. Also searched for other uses, but didn't found any.

Absolutely NONE of those route patterns were in scope to be changed here. There is a dedicated issue for that. This was just about adding ->get('_raw_variables') on one line...

Status:Needs review» Needs work

yeah, I was thinking the changes would only be in those lines already changed/added in this patch.
but ... it has reached too far.

Sorry, I'll take it out.

Status:Needs work» Needs review
StatusFileSize
new7.45 KB
new2.24 KB
new6.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,576 pass(es).
[ View ]

I guess maybe the second half of the local tasks is new (not moved) and it could have gotten the current pattern, but I left it.

So this is just the comment changes, and the corrections to the assertion messages.

Status:Needs review» Reviewed & tested by the community

Reviewed and all looks good except a minor issue.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -270,7 +270,7 @@ public function getTasksBuild($current_route_name) {
+          $active = (($current_route_name == $route_name && (array_intersect_assoc($route_parameters, $this->request->attributes->get('_raw_variables')->all()) == $route_parameters)) || $child->getActive());

Minor: Can we split this into two lines to make it more readable?

This is very minor. So RTBC from my side.

StatusFileSize
new1.8 KB
new6.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,972 pass(es).
[ View ]

I wanted to re-roll this in order to bring this forward. Because of the nested AND/OR the line gets very verbose, though, so I am not 100% sure this is an improvement. I'll let the committers decide.

Sorry, the interdiff shows a deleted file, which was already deleted before, I messed that up. The patch is fine though, as far as I can see.

@tstoeckler - that formatting seems odd. I don't think that conforms to code style for multi-line.

https://drupal.org/coding-standards#linelength

Status:Reviewed & tested by the community» Needs work

Can we *please* just have this issue fix the bug? The multiline boolean is already in HEAD, this shouldn't change that.

I'm re-rolling based on #10.

i don't think that was RTBC since it needs to verify it got something for _raw_variables.

Status:Needs work» Needs review
StatusFileSize
new4.09 KB
new7.09 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here's a patch that validates the presence of the attribute and also fixes the test text.

StatusFileSize
new1.03 KB
new7.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,108 pass(es).
[ View ]

Feedback from Tim in IRC, we should fallback to the request attributes, as we do in other code.

StatusFileSize
new2.99 KB
new8 KB
FAILED: [[SimpleTest]]: [MySQL] 58,446 pass(es), 110 fail(s), and 25 exception(s).
[ View ]

Let's move that to a easy to understand method to unclutter the code here.

Status:Needs review» Needs work

The last submitted patch, local_task-2095139-26.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new7.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,029 pass(es).
[ View ]
new633 bytes

Last change seems fine. Tested it locally again and it works also the test are green, no clue what was wrong with the testbot.
Just a very very minor issue, there are two returns which is unnecessary. Removed it.

If testbots agree, RTBC again.

Good catch!

Status:Reviewed & tested by the community» Fixed

Committed 375fadf and pushed to 8.x. Thanks!

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

Issue summary:View changes

adding the related