Instead of finding all the tasks on each page run, you could also cache the result per routename in a cache backend.
#2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu()

Files: 
CommentFileSizeAuthor
#22 local_task-2032303-22.patch19.41 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,131 pass(es).
[ View ]
#22 interdiff.txt659 bytesdawehner
#21 local_task-2032303-21.patch19.38 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,107 pass(es).
[ View ]
#20 local_task-2032303-20.patch19.35 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,193 pass(es).
[ View ]
#17 local_task-2032303-16.patch19.52 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2032303-16_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 interdiff.txt2.86 KBdawehner
#16 local_task-2032303-16.patch18.74 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 57,376 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#16 local_task-2032303-16-small.patch15.06 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2032303-16-small.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 2032303-13-16.increment.txt5.42 KBpwolanin
#13 local_task-2032303-13.patch18.58 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,155 pass(es).
[ View ]
#13 interdiff.txt4.91 KBdawehner
#11 local_task-2032303-11.patch17.53 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,813 pass(es).
[ View ]
#11 interdiff.txt6.39 KBdawehner
#7 local_task-2032303-7.patch17.77 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,170 pass(es).
[ View ]
#7 interdiff.txt6.44 KBdawehner
#5 local_task-2032303-4.patch15.68 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,225 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 local_task-2032303-3.patch8.92 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#3 local_task-2032303-3-small.patch3.47 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2032303-3-small.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 interdiff.txt3.59 KBdawehner
#1 local-task-2032303-1-small.patch3.47 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local-task-2032303-1-small.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 local-task-2032303-1.patch8.92 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.92 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new3.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local-task-2032303-1-small.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let's see whether this works.

I don't think we should burden the route itself with this - caching per route seems reasonable.

StatusFileSize
new3.59 KB
new3.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2032303-3-small.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new8.92 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Used the build in cache backend of the default plugin manager.

Getting that initial patch in like that seems a bit rash anyway, so this seems pretty important.

StatusFileSize
new15.68 KB
FAILED: [[SimpleTest]]: [MySQL] 57,225 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Just tracking some work on some unit tests.

Status:Needs review» Needs work

The last submitted patch, local_task-2032303-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.44 KB
new17.77 KB
PASSED: [[SimpleTest]]: [MySQL] 57,170 pass(es).
[ View ]

There we go.

Issue tags:+phpunit

add tag

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -64,13 +74,19 @@ class LocalTaskManager extends DefaultPluginManager {
+

Why the newline? Doesn't seem like a logic split. Sorry, that is SUPER NITPICKY!

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -118,63 +134,71 @@ public function getPath(LocalTaskInterface $local_task) {
+        // @todo - optimize this lookup by compiling or caching.

I guess this issue is to remove this?

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.phpundefined
@@ -141,6 +131,99 @@ public function testGetLocalTasksForRouteSingleLevelTitle() {
+    $this->factory->expects($this->any())

We could use exactly() here? is it worth it?

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.phpundefined
@@ -201,6 +284,49 @@ protected function setupLocalTaskManager() {
+    $property = new \ReflectionProperty($this->manager, 'cacheLangcodeSuffix');
+    $property->setAccessible(TRUE);
+    $property->setValue($this->manager, 'en');

This doesn't seem needed if we are mocking the getLanguage method above? that should provide the correct code for us?

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -64,13 +74,19 @@ class LocalTaskManager extends DefaultPluginManager {
+   * @param \Drupal\Core\Language\LanguageManager $language_manager
    */

misses docs.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -64,13 +74,19 @@ class LocalTaskManager extends DefaultPluginManager {
+    $this->cacheLangcodeSuffix = $language_manager->getLanguage(Language::TYPE_INTERFACE)->id;

Oh, Language does not have proper methods yet.

Also, can we move that into a $this->setCacheBackend override maybe? It should belong there as it's bound to the language manager.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -118,63 +134,71 @@ public function getPath(LocalTaskInterface $local_task) {
+        // @todo - optimize this lookup by compiling or caching.

Yeah, that can be removed now :)

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.phpundefined
@@ -118,63 +134,71 @@ public function getPath(LocalTaskInterface $local_task) {
+            // integer depth.  Create instances for each plugin along the way.

Two spaces in front of "Create". Yeah, super important to fix that.

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.phpundefined
@@ -82,6 +91,9 @@ protected function setUp() {
+    $this->cacheBackend = $this->getMockBuilder('Drupal\Core\Cache\CacheBackendInterface')
+      ->getMock();

Why not just ->getMock() if you don't change anything?

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.phpundefined
@@ -201,6 +284,49 @@ protected function setupLocalTaskManager() {
+    $language_manager->expects($this->any())
+      ->method('getLanguage')
+      ->will($this->returnValue(new Language(array('id' => 'en'))));
+
+    $property = new \ReflectionProperty($this->manager, 'cacheLangcodeSuffix');
+    $property->setAccessible(TRUE);
+    $property->setValue($this->manager, 'en');

If we put the language id reading into the setCacheBackend override we don't have to write the langCodeSuffix property with reflection.

StatusFileSize
new6.39 KB
new17.53 KB
PASSED: [[SimpleTest]]: [MySQL] 56,813 pass(es).
[ View ]

Thanks for the good reviews! Let's use $this->cacheKey as prefix now.

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.phpundefined
@@ -141,6 +129,99 @@ public function testGetLocalTasksForRouteSingleLevelTitle() {
+    $mock_plugin = $this->getMock('Drupal\Core\Menu\LocalTaskInterface');
+
+    $map = array(
+      array('menu_local_task_test_tasks_settings', array(), $mock_plugin),
+      array('menu_local_task_test_tasks_edit', array(), $mock_plugin),
+      array('menu_local_task_test_tasks_view', array(), $mock_plugin),
+    );
+
+    $this->factory->expects($this->any())
+      ->method('createInstance')
+      ->will($this->returnValueMap($map));
+
+    $this->setupLocalTaskManager();
+
+    $result = array(
+      0 => array(
+        'menu_local_task_test_tasks_settings' => $mock_plugin,
+        'menu_local_task_test_tasks_view' => $mock_plugin,
+        'menu_local_task_test_tasks_edit' => $mock_plugin,
+      )

You using the same results and map arrays at least in two places. what about a short setup method that does that for you?

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.phpundefined
@@ -201,6 +282,45 @@ protected function setupLocalTaskManager() {
+   * @return array

That could use a short docblock.

Patch looks good otherwise.

StatusFileSize
new4.91 KB
new18.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,155 pass(es).
[ View ]

There we go.

Status:Needs review» Reviewed & tested by the community

Thanks

Status:Reviewed & tested by the community» Needs work
Issue tags:+MenuSystemRevamp, +WSCCI

I don't think we should be caching the actual instances?

+      if ($cache = $this->cacheBackend->get($this->cacheKey . ':' . $route_name)) {
+        $this->instances[$route_name] = $cache->data;
       }

We should cache a step before that and create the instances on demand I think.

StatusFileSize
new5.42 KB
new15.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2032303-16-small.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new18.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,376 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This fixes the pluigin manager, but the changed tests are broken still.

Status:Needs work» Needs review
StatusFileSize
new2.86 KB
new19.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch local_task-2032303-16_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed the unit tests.

Issue tags:-phpunit, -MenuSystemRevamp, -WSCCI

#17: local_task-2032303-16.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+phpunit, +MenuSystemRevamp, +WSCCI

The last submitted patch, local_task-2032303-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,193 pass(es).
[ View ]

Rerolled ...

StatusFileSize
new19.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,107 pass(es).
[ View ]

Yet another rerole.

StatusFileSize
new659 bytes
new19.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,131 pass(es).
[ View ]

Status:Needs review» Needs work
Issue tags:-phpunit, -MenuSystemRevamp, -WSCCI

The last submitted patch, local_task-2032303-22.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+phpunit, +MenuSystemRevamp, +WSCCI

#22: local_task-2032303-22.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

I reviewed the fix (-w helped!), +1.

The test looks good, and when I ran it without the fix locally it failed as expected (no point in uploading it standalone to the bot as all phpunit tests fail monolithically there).

Thanks!

Status:Reviewed & tested by the community» Fixed

This takes 15ms of local task building on my system according to XHProf. It also cut function calls down from 48,989 to 46,364 on the front page with standard profile, and file_exists() calls down from 73-odd to 20-odd.

Committed/pushed to 8.x, thanks!

Actually looking at #2046565: Cache the local action plugins that appear per route I'm wondering a bit about the 'per route' aspect of this - was this compared with just caching the plugin discovery? I realise there's potentially a lot of local tasks on a real site though so it might be fine, but would be good to discuss.

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

Issue summary:View changes

Updated to describe the actual approach.