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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
1.19 KB

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

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