Instead of loading each local task route object manually it would be possible to just load all of them parallel.

Files: 
CommentFileSizeAuthor
#9 interdiff.txt831 bytesdawehner
#9 local_task-2032311-9.patch1.25 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,884 pass(es). View
#6 local_task-2032311-6.patch1.21 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,253 pass(es). View
#4 local_task-2032311-4.patch1.27 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,638 pass(es). View
#1 local_task-2032311-1.patch1.19 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
1.19 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

This should hopefully work.

Crell’s picture

This seems like a reasonable performance optimization.

Status: Needs review » Needs work

The last submitted patch, local_task-2032311-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
PASSED: [[SimpleTest]]: [MySQL] 56,638 pass(es). View
pwolanin’s picture

so, it looks like in the current implementation this would eliminate potentially several DB queries, at the cost of an extra foreach loop, which seems liek a reasonable tradeoff.

I might prefer a ternary instead of if/else here:

+    if ($route_names) {
+      $routes = $this->routeProvider->getRoutesByNames($route_names);
+    }
+    else {
+      $routes = array();
+    }
dawehner’s picture

FileSize
1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,253 pass(es). View

There we go.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! Thanks @dawehner.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -192,11 +192,21 @@ public function getLocalTasksForRoute($route_name) {
+    // Fetch a bunch of routes at once.

Perhaps rather than the enjoyably flowery language of a "bunch of routes" we should change this to // Prefetch routes.

... if we need a comment at all :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 56,884 pass(es). View
831 bytes

There we go.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2f0e07f and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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