Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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()
Comment | File | Size | Author |
---|---|---|---|
#22 | local_task-2032303-22.patch | 19.41 KB | dawehner |
#22 | interdiff.txt | 659 bytes | dawehner |
#21 | local_task-2032303-21.patch | 19.38 KB | dawehner |
#20 | local_task-2032303-20.patch | 19.35 KB | dawehner |
#17 | local_task-2032303-16.patch | 19.52 KB | dawehner |
Comments
Comment #1
dawehnerLet's see whether this works.
Comment #2
pwolanin CreditAttribution: pwolanin commentedI don't think we should burden the route itself with this - caching per route seems reasonable.
Comment #3
dawehnerUsed the build in cache backend of the default plugin manager.
Comment #4
damiankloip CreditAttribution: damiankloip commentedGetting that initial patch in like that seems a bit rash anyway, so this seems pretty important.
Comment #5
dawehnerJust tracking some work on some unit tests.
Comment #7
dawehnerThere we go.
Comment #8
dawehneradd tag
Comment #9
damiankloip CreditAttribution: damiankloip commentedWhy the newline? Doesn't seem like a logic split. Sorry, that is SUPER NITPICKY!
I guess this issue is to remove this?
We could use exactly() here? is it worth it?
This doesn't seem needed if we are mocking the getLanguage method above? that should provide the correct code for us?
Comment #10
fubhy CreditAttribution: fubhy commentedmisses docs.
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.
Yeah, that can be removed now :)
Two spaces in front of "Create". Yeah, super important to fix that.
Why not just ->getMock() if you don't change anything?
If we put the language id reading into the setCacheBackend override we don't have to write the langCodeSuffix property with reflection.
Comment #11
dawehnerThanks for the good reviews! Let's use $this->cacheKey as prefix now.
Comment #12
fubhy CreditAttribution: fubhy commentedYou using the same results and map arrays at least in two places. what about a short setup method that does that for you?
That could use a short docblock.
Patch looks good otherwise.
Comment #13
dawehnerThere we go.
Comment #14
fubhy CreditAttribution: fubhy commentedThanks
Comment #15
pwolanin CreditAttribution: pwolanin commentedI don't think we should be caching the actual instances?
We should cache a step before that and create the instances on demand I think.
Comment #16
pwolanin CreditAttribution: pwolanin commentedThis fixes the pluigin manager, but the changed tests are broken still.
Comment #17
dawehnerFixed the unit tests.
Comment #18
Crell CreditAttribution: Crell commented#17: local_task-2032303-16.patch queued for re-testing.
Comment #20
dawehnerRerolled ...
Comment #21
dawehnerYet another rerole.
Comment #22
dawehnerRerolled based upon berdir feedback #2046565-15: Cache the local action plugins that appear per route
Comment #24
Berdir#22: local_task-2032303-22.patch queued for re-testing.
Comment #25
tim.plunkettI 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!
Comment #26
catchThis 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!
Comment #27
catchActually 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.
Comment #28.0
(not verified) CreditAttribution: commentedUpdated to describe the actual approach.