Refactor LocalTaskManager to be more readable

dawehner++

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -211,79 +211,15 @@ public function getLocalTasksForRoute($route_name) {
   +        $this->setLocalTasksCache($route_name, $base_routes, $parents, $children);
   

   I would structure this differently. Rather, we want a getLocalTasks() method, which internally is just a caching/memoizing wrapper around a generateLocalTasks() method (or similar). Then just use the get method when we actually need it, and the caching all happens behind the scenes.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -415,4 +351,141 @@ protected function isRouteActive($current_route_name, $route_name, $route_parame
   +    foreach ($definitions as $plugin_id => $task_info) {
   +      if (!empty($base_routes[$task_info['base_route']]) && (empty($task_info['parent_id']) || !empty($parents[$task_info['parent_id']]))) {
   +        // Concat '> ' with root ID for the parent of top-level tabs.
   +        $parent = empty($task_info['parent_id']) ? '> ' . $task_info['base_route'] : $task_info['parent_id'];
   +        $children[$parent][$plugin_id] = $task_info;
   +      }
   

   This would be clearer as an array_filter call, followed by an array_map. (Mainly because that encourages breaking off defined, named methods for what the filter criteria are and what we're actually doing with it.)

3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -415,4 +351,141 @@ protected function isRouteActive($current_route_name, $route_name, $route_parame
   +        // Populate the definitions we use in the next loop. Using a
   +        // reference like &$task_info causes bugs.
   +        $definitions[$plugin_id]['base_route'] = $definitions[$task_info['parent_id']]['base_route'];
   

   I do not fully understand the purpose of this code, but the comment is enough to make me curl up in a ball in fear. Modify information for the next foreach iteration? Er... that sounds highly scary.

Internal re-org seems fine, if you have the energy

Refactored the existing code. In the process, I took care of the @todo to allow for more than 2 levels of routes.

In explanation

• I used the findDefinitions to generate children before the definitions were cached but after definitions were able to be altered – this allows the children definitions to always have up-to-date info.
• I then used a task to find the current route's breadcrumb path.
• Then built the current route's task tree around the breadcrumb, unsetting children on routes that were unneeded.

Benefits of refactoring, using tree, and caching the children:

• Sets and allows local tasks that have more than 2 levels.
• Allows the two top levels to always have correct active_paths, regardless of if more than 2 levels are rendered
• Caches a data-intensive function that finds children, which saves time for every route look up
• Cleans up parent-child tree look-up, breaking it into discernible tasks.

Two questions though:

1. Should we still set the $parents, $children, and $base_routes info in the cached data array and merely mark them as deprecated to be removed in 9? They're now unneeded in in this class, but they'd be easy to generate using array_keys and a maybe a foreach loop.
2. Do we need a generateTaskTreeData to call inside of the getTaskTreeData?

Let me know what you guys think.

Re-rolling the patch. Updated the documentation. Please see https://www.drupal.org/node/2653144#comment-12084135 for original comment.

@dawehner That's my fault for reusing terminology. The thought is that it checks the route that is passed in, and parses through the plugin definitions for that route's parent plugin definition IDs (breadcrumbs). This way, it can use the parent plugin IDs (breadcrumbs) to then move down the local task menu tier by tier to allow more than first and secondary menus (to take care of the @todo that has been in there for a while.) This additionally lets modules to continue to show active path in first and secondary menus without necessarily having the current page on a visible local task.

In doing so, I attempted to also refactor the class to break out certain methods and make it more readable.

Here is a new patch. Note: I undid the part about caching children and made some updates since the last patch.

These tests aren't failing locally, so here's another shot in the dark. I'm currently trying to determine why Drupal\Tests\system\Functional\Menu\MenuAccessTest erred, since the message response is accurate (that the link exists).

So this patch addresses two issues: refactoring the code class to make it easier to manage, and taking care of the @todo to allow more than two levels of depth.

Changes

It set up 3 new methods on the LocalTaskManager test:

• getBreadcrumbsForRoute
• generateChildrenForDefinitions
• getTaskTreeData

I modified one test, LocalTaskManagerTest, to change the test cached response.

Possible changes

@dawehner, I could change getBreadCrumbsForRoute to getActiveTrailForRoute.

Also, I considered setting up a method getTreeByActiveTrail to create the task tree from passed-in $definitions and $activeTrail, to make this class more reusable, and then removing generateChildrenForDefinitions.

Modified a bit and renamed to active trail.

@dawehner My thought is so that these methods could be used elsewhere, I.E. if someone would want use the manager in their own module/block/alter to load different definitions, modify the active trail, or use a full menu-tree setup (nested unordered lists) compared to the multi-level (first, secondary) setup that local tasks currently uses by default.

@dawehner Oh, alright. I modified the methods and made them protected and removed them from the interface class. Also, sorry about that; I'm new to contributing and I'm still getting the hang of testing and creating patches. Thanks for sending over the interdiff link!

I made an interdiff between the last patch that passed (#22) and the new one.

Rerolled for 8.6.

There is a couple of coding standards issues in the patch.

core/lib/Drupal/Core/Menu/LocalTaskManager.php
line 220	Expected 1 space after WHILE keyword; 0 found
228	   Expected newline after closing brace
254	   Expected 1 space after IF keyword; 0 found

Those should be fixed, I'd love otherwise RTBC this patch but I don't feel sufficiently sure about this, it looks like this breaks the current behavior, because the test also needed to be changed.

Back to needs work for those coding standards.

Coding standard issues are solved in updated patch mentioned in #33 & also added an interdiff.

Working on this issue

#35 apply cleanly to 9.5.x.
Added the screenshot after applying please review and let us if anything else needs to be updated.

Thanks

Thank you for reviewing this issue @akshay_d

The automated testing infrastructure tells us whether the patch applies, so we do not need people to review that, and also not needed to post a screenshot for that.

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

When you do post a review, be sure to describe what you reviewed and how, this helps other reviewers.

Attached Reroll patch against Drupal 9.5.x

Rerolled patch #35 with Drupal 9.5.x

Rerolled patch #49 with Drupal 9.5.x

This looks like a useful thing to do -- smaller and simpler methods are easier to understand, debug, and maintain!

However, the docs aren't clear in places.

I'm also concerned that the tests are being changed in this patch. If this is just a refactoring, then tests should not change -- instead, they should remain the same to show us they still pass and so the functionality is unchanged by the refactoring!

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,184 @@ public function getDefinitions() {
   +   * Return active-trail of task definition plugin ids for the current route.
   

   This would be a great place to explain what the active-trail means.

   Also, if there is a $route_name parameter, then it's not the 'current' route, it's the 'given' route.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,184 @@ public function getDefinitions() {
   +   *   The plugin definitions list.
   

   This needs to say the key is expected.

3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,184 @@ public function getDefinitions() {
   +   *   The modified plugin definitions list with children sub-arrays.
   

   This needs more detail.

4. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,184 @@ public function getDefinitions() {
   +   * Build a nested tree of plugin definitions based on the active_trail.
   

   Should be 'Builds'. Same for all the other methods.

   What's 'active_trail'?

5. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,184 @@ public function getDefinitions() {
   +   * @return array
   

   Missing blank line.

6. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,184 @@ public function getDefinitions() {
   +   * Return an array containing tree and breadcrumb data for the current route.
   

   Same - given route, not current route.

@Ankit.Gupta and @pooja saraah
You can check for build errors make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...

@Ankit.Gupta
Please upload an interdiff so we can see the changes being made between patches.

Fixed up the patch. Attempted to address the points in #52 but sure it will need tweaking. Tech writing is not my strongest.

Just nits:

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,186 @@ public function getDefinitions() {
    
   +  /**
   +   * Return active-trail (specific task definition plugins) for the given route.
   +   *
   

   I don't know what (specific task definition plugins) means - I think this should move under the summary with a bit more detail.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,186 @@ public function getDefinitions() {
   +      // Create children array on definition for uniformity.
   +      if (!array_key_exists('children', $definition)) {
   +        $definition['children'] = [];
   

   array_key_exists() should be fully qualified (\array_key_exists()) for performance - PHP has an opcode implementation.

3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,186 @@ public function getDefinitions() {
   +        $definition['children'] = [];
   +      }
   +      // Add definition pointer to parent's child array (if not its own parent).
   +      if (isset($definitions[$parent_id]) && $parent_id !== $id) {
   

   We could reverse the sentence to remove the parentheses:

   If the definition isn't the parent of itself, add a definition pointer to the parent's child array.

1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,186 @@ public function getDefinitions() {
   +      $task_id = end($definition_ids);
   

   I think this should be 'upwards'? Not entirely sure if that's a British vs. Americanism though.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -191,94 +191,186 @@ public function getDefinitions() {
   +    $active_top_level = current($active_trail);
   +    $base_route = ($active_top_level && array_key_exists($active_top_level, $definitions)) ? $definitions[$active_top_level]['base_route'] : NULL;
   +    // Unset if both not top-level active-trail and one of the following:
   

   Same thing with array_key_exists()

One more question - there's some test coverage changes, is that definitely all additions, or is it changing the expectations of the test. Would be good to have a 'patch only patch' to see whether it doesn't break the existing test coverage without those changes.

#54
1 = Updated with a more clear version from a previous patch
2 = updated both instances
3 = Updated
4 = not sure I follow
5 = Updated

Also uploading test-only patch.

Drive by review, but we should be able to add type-hints to the new functions - both for the params and the return types

Updated with typehints.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -191,94 +191,187 @@ public function getDefinitions() {
+      // Start ancestors array with last-available non-NULL active task id
+      // gathered from route and work upward.
+      $task_id = end($definition_ids);

RE #56 4. was about the word upward in the comment here. Which needs to be changed to upwards I guess.

@BramDriesen,
Made changes according to #60.
Needs review.

See error below for the failed test run.

FILE: /var/www/html/core/lib/Drupal/Core/Menu/LocalTaskManager.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 267 | ERROR | Doc comment short description must be on a single
     |       | line, further text should be a separate paragraph
     |       | (Drupal.Commenting.DocComment.ShortSingleLine)
----------------------------------------------------------------------

Time: 10.19 secs; Memory: 6MB


PHPCS: failed

Addressed #62 and Raised an MR. Please review.