Refactor LocalTaskManager to be more readable

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.

The last submitted patch, 56: 2653144-56-tests-only.patch, failed testing. View results →

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.