Problem/Motivation
Blocks #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).
#1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient introduce a custom approach to caching rendered menus used in the administration toolbar. Recently, work on #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees has progressed. Once the menu caching patch lands, we should refactor Toolbar to use Core's menu caching strategy.
Proposed resolution
Use Core's menu caching strategy as developed in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.
User interface changes
None.
API changes
None.
Original report by @jessebeach
Comment | File | Size | Author |
---|---|---|---|
#15 | 2217985-remove-toolbar-get-user-cid.patch | 694 bytes | Dave Reid |
#10 | 2217985-10-interdiff.txt | 1.37 KB | Berdir |
#10 | 2217985-10.patch | 14.83 KB | Berdir |
Comments
Comment #1
Wim LeersBlocked on #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.
Comment #2
Wim LeersThis is now unblocked, with both #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees and #2254865: toolbar_pre_render() runs on every page and is responsible for ~15ms/17000 function calls having landed.
Comment #3
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #4
Wim LeersThis now blocks #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).
Comment #5
Wim LeersUses a technique similar to #2450897: Cache Field views row output.
Comment #7
BerdirI went through the failing tests and they all no longer make sense. Most were about testing if the cache response has a tag for the user, which it no longer has (so I removed a complete test method.. there was nothing useful left after I removed those asserts). Another one was about the langcode argument that is no longer used directly but only indirectly through the standard negotiation and cache context.
And we have still a test that makes sure that different languages result in different hashs.
Comment #9
Fabianx CreditAttribution: Fabianx as a volunteer commentedExcept for failing tests looks really good to me, like really-near-RTBC.
Comment #10
BerdirYes, forgot to actually remove the part I was talking about.
Before being RTBC'd, I think this probably needs some more manual testing. I'm not sure how much testing was done by Wim, but since there's a fair amount of JS involved, it seems like manual testing wouldn't hurt. Also of the multilingual scenario with multiple languages, making sure it switches the language properly. We have tests, but I'm not 100% sure those tests actually confirm that it is working.
Comment #11
Wim LeersI'll do a round of manual testing.
Comment #12
Wim LeersAfter a solid round of manual testing, I can confirm that the manual testing agrees with the automated testing: the language switching works perfectly. (I can toggle between
http://example.com/
andhttp://example.com/nl
, and the toolbar subtrees hash keeps changing along, and the menu translation is toggled along.)Comment #13
catchThis cleans things up a lot.
I had to remind myself why we still do the js replacement here, but that's valuable to keep response sizes down.
Committed/pushed to 8.0.x, thanks!
Comment #15
Dave ReidThis left an orphaned function _toolbar_get_user_cid() which is no longer used.
Comment #16
Wim Leers+1 for #15. But note that a committer may ask to do this in a follow-up issue.
Comment #17
catchYes please, just keeps git blame tidier.
Comment #18
BerdirSomething seems to be broken. Seeing 403 forbidden errors in console, no subtrees, someone in IRC reported the same. Not sure if we should reopen/revert this or figure it out in a follow-up?
Comment #19
Dave ReidPosted follow-up in #2534856: Remove unused _toolbar_get_user_cid(). Hope it doesn't get ignored.
@Berdir: Anyone might be using admin_toolbar.module? If so, the module has to be updated to work with the latest core since this was committed.
Comment #20
Wim Leers#19: reviewed.
#18: for me, the toolbar is just plainly not showing up. I don't get 403s. But… I did reproduce the toolbar being broken :( Incomplete test coverage-- :(
Assigning to me, will file a follow-up issue to fix this.
Comment #21
Wim LeersActually, somebody has already opened an issue for it, let's just use that: #2534830: Toolbar subtrees not working.
Comment #22
Wim LeersNow there's a patch with test coverage at #2534830-2: Toolbar subtrees not working. See you all there.
Comment #23
effulgentsia CreditAttribution: effulgentsia at Acquia commented