Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
8.92 KB
3.47 KB

Let's see whether this works.

pwolanin’s picture

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

dawehner’s picture

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

damiankloip’s picture

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

dawehner’s picture

FileSize
15.68 KB

Just tracking some work on some unit tests.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.44 KB
17.77 KB

There we go.

dawehner’s picture

Issue tags: +PHPUnit

add tag

damiankloip’s picture

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

fubhy’s picture

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

dawehner’s picture

FileSize
6.39 KB
17.53 KB

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

fubhy’s picture

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

dawehner’s picture

FileSize
4.91 KB
18.58 KB

There we go.

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

pwolanin’s picture

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.

pwolanin’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
19.52 KB

Fixed the unit tests.

Crell’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.35 KB

Rerolled ...

dawehner’s picture

FileSize
19.38 KB

Yet another rerole.

dawehner’s picture

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

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

Berdir’s picture

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

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

tim.plunkett’s picture

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!

catch’s picture

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!

catch’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated to describe the actual approach.