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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: Replace the custom menu caching strategy in Toolbar with Core's standard caching. » [PP-1] Replace the custom menu caching strategy in Toolbar with Core's standard caching.
Status: Active » Postponed
Wim Leers’s picture

Title: [PP-1] Replace the custom menu caching strategy in Toolbar with Core's standard caching. » Replace the custom menu caching strategy in Toolbar with Core's standard caching.
Status: Postponed » Active
Issue tags: +D8 cacheability
Wim Leers’s picture

Wim Leers’s picture

Status: Active » Needs review
FileSize
6.91 KB

Uses a technique similar to #2450897: Cache Field views row output.

Status: Needs review » Needs work

The last submitted patch, 5: 2217985-5.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +D8 Accelerate London
FileSize
13.73 KB
7.07 KB

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

Status: Needs review » Needs work

The last submitted patch, 7: 2217985-7.patch, failed testing.

Fabianx’s picture

Except for failing tests looks really good to me, like really-near-RTBC.

Berdir’s picture

Status: Needs work » Needs review
FileSize
14.83 KB
1.37 KB

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

Wim Leers’s picture

I'll do a round of manual testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

After 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/ and http://example.com/nl, and the toolbar subtrees hash keeps changing along, and the menu translation is toggled along.)

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed cbcbfc1 on 8.0.x
    Issue #2217985 by Berdir, Wim Leers: Replace the custom menu caching...
Dave Reid’s picture

Status: Fixed » Needs review
FileSize
694 bytes

This left an orphaned function _toolbar_get_user_cid() which is no longer used.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

+1 for #15. But note that a committer may ask to do this in a follow-up issue.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yes please, just keeps git blame tidier.

Berdir’s picture

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

Dave Reid’s picture

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

Wim Leers’s picture

Assigned: Unassigned » 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.

Wim Leers’s picture

Actually, somebody has already opened an issue for it, let's just use that: #2534830: Toolbar subtrees not working.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

Now there's a patch with test coverage at #2534830-2: Toolbar subtrees not working. See you all there.

effulgentsia’s picture

Status: Fixed » Closed (fixed)

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