Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
994 bytes

.

pwolanin’s picture

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

dawehner’s picture

Priority: Normal » Major

... this blocks many conversions.

tim.plunkett’s picture

dawehner’s picture

FileSize
5.93 KB

Here is a test.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

looks good.

tim.plunkett’s picture

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Gábor Hojtsy’s picture

Issue tags: +blocker
dawehner’s picture

Status: Needs work » Needs review
Issue tags: -blocker
FileSize
5.9 KB
4.23 KB
621 bytes

There we go.

dawehner’s picture

Issue tags: +blocker

Issuetag monster

YesCT’s picture

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.

kfritsche’s picture

Assigned: Unassigned » kfritsche

working on it now.

kfritsche’s picture

Assigned: kfritsche » Unassigned
Status: Needs work » Needs review
FileSize
9.85 KB
7.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.

tim.plunkett’s picture

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

YesCT’s picture

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.

YesCT’s picture

Status: Needs work » Needs review
FileSize
7.45 KB
2.24 KB
6.13 KB

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.

vijaycs85’s picture

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.

tstoeckler’s picture

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.

tstoeckler’s picture

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.

pwolanin’s picture

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

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

tim.plunkett’s picture

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.

pwolanin’s picture

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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.09 KB
7.09 KB

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

pwolanin’s picture

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

dawehner’s picture

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.

kfritsche’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.97 KB
633 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.

dawehner’s picture

Good catch!

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

adding the related