Problem/Motivation
When the Toolbar tray is oriented horizontally, second level menu items are not available to the end user. These second-level items are only available when the admin menu is presented in a vertical orientation. So, we can save an AJAX request to get these second plus level items if we wait for the menu to be set to vertical before trigger their fetch.
Eventually, we will further optimize the toolbar by caching the second plus level menu items locally rather than triggering an AJAX request if nothing about those links has changed. The issue chain for that work is as follows:
#1605290: Enable entity render caching with cache tag support
#2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached
#2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash
#1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient
Proposed resolution
Delay the request to get second plus level menu items until they are needed.
Remaining tasks
Review the patch
User interface changes
None.
API changes
None.
Related Issues
Original report by @jessebeach
Comment | File | Size | Author |
---|---|---|---|
#35 | delay-menu-subtrees-2077279-35.patch | 23.83 KB | jessebeach |
#35 | interdiff.txt | 604 bytes | jessebeach |
#32 | delay-menu-subtrees-2077279-32.patch | 23.92 KB | jessebeach |
#32 | interdiff.txt | 10.78 KB | jessebeach |
#23 | interdiff.txt | 722 bytes | jessebeach |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedRather than simply loading the JSONP callback that fetches the second plus level menu items on page load, this patch just sends the URL to that callback in
drupalSettings.toolbar
. The Toolbar determines when to invoke that URL to load the subtree data.Comment #2
jessebeach CreditAttribution: jessebeach commentedThis patch introduces client-side caching of submenu items.
The client uses a locally-stored hash of the submenu items until the menu, user, or role entities invoke their
hook_ENTITY_TYPE_update
hooks. Then the toolbar cache is invalidated and on the next page load, the client requests new subtrees.The only thing I'm not sure of is how ham-fisted the cache clearing might be with this approach. What if there are a million users?
Comment #3
jessebeach CreditAttribution: jessebeach commentedTitle update.
Comment #4
jessebeach CreditAttribution: jessebeach commentedInterdiff for #2.
Comment #6
Gábor HojtsyI think you want to react to menu_item updates. Menus are just the containers and a change in the container does not really make any items change(?).
Also maybe react on module enable/disable/install?
Comment #7
jessebeach CreditAttribution: jessebeach commentedThis patch completes the cases that we need to cover in order to ensure that a change to a the admin menu will will result in a change hash of the subtrees and thus invalidate the client-side cache representation of the subtrees.
The cases are covered by the following hooks:
I'm working on tests for each invalidation case right now and will have those posted shortly.
Comment #8
jessebeach CreditAttribution: jessebeach commentedNow with tests! I eliminated the
hook_modules_installed
andhook_modules_uninstalled
implementations because, as I later realized, any module that is installed with also go through enable and any module that is uninstalled will first go through disable, so implementing these hooks was unnecessary. The tests cover these implementations:Comment #10
jessebeach CreditAttribution: jessebeach commentedInterdiff for #8.
Comment #11
jessebeach CreditAttribution: jessebeach commentedRerolled because of #2061897: Remove calls to deprecated global $user in toolbar module.
Comment #13
Gábor HojtsyFixing the language id() method invocation the new tests pass for me. Next up: review :)
Comment #14
Gábor HojtsyLet's document these.
This comment sounds like copy-paste from a prior test.
This repeats several times. It may be best to put this into a helper method (which returns the new subtree hash), so you can just do something like:
$hash = $this->assertDifferentHash($hash);
(passing in the old hash and getting back the new hash, that you can pass in again as the old hash in the next case).
Can avoid lots of copy-paste.
// Test a permission update.
Sounds like this would *both* do a role update and a user update, so clears the toolbar cache twice? Not sure what can change about a role that you need to clear the cache on btw. I sense the role update would be invoked when the *name* of the role changes. Is it invoked when the permissions of the role are changed? (In that case, the above role update already tests this?)
Overall, if a role update is only invoked when the role itself changes (I guess its name), then we may not need a cache clear for that. For creating new roles, until they are associated with a user, we don't care about them, and when they are associated, the user update is invoked.
This looks pretty much magic :D I'd document how the path is structured or take the hash for some reusable function, eg. make toolbar_subtree_hash() a public function (without an underscore prefix) and use that.
This looks a bit aggressive. When any user is saved (eg. registers on the site, updates profile), all user's toolbar caches are deleted. I'd introduce a uid argument on the clearing function and if passed, only clear the cids prefixed with that uid. This hook looks like the most invoked one on a site. Menu link updates on the admin menu or module or role changes are not very often. But user changes are common on a living site.
As said above this may not be needed? Unless this covers permissions changes. If this does not cover permissions changes, we should figure out what covers that though.
Respect Drupal namespacing by starting this function off as _toolbar_*(), not _clear_*(). So _toolbar_clear_cache().
Comment #15
jessebeach CreditAttribution: jessebeach commentedWithout implementing
hook_user_update
, changing the permission on a role fails:I'll just document it better. I tried turning
_toolbar_subtree_hash()
into a public function by providing an optional$user
argument. It turns out that you need a request to properly construct the subtrees due to access checking that require dependency injection, services, etc. etc. etc. It's just not possible to get the rendered subtrees and thus the hash outside of a request. I spent at least 6 hours trying to find that hash value in a dependable way and it was only after printing$this->container
through debug in the test and combing over the contents that I discovered thatdrupalSettings
is provided. This is how the client knows what the path to retrieve the subtree menu items is, so it's a good value for the tests to use as well.Right, the current code is way too aggressive. I've added an optional
$cid
argument to the cache clearing functioning that will limit the scope of the clear. I added assertions to the tests to verify that only the cache for a single user is changed in this case and not all users.All other feedback addressed in the new patch without further need for comment.
Comment #17
Gábor Hojtsy#15: delay-menu-subtrees-2077279-15.patch queued for re-testing.
Comment #18
Gábor HojtsyI could not reproduce the installation problem reported by the testbot on the UI, so sent for a retest. In code review, found this:
This looks problematic, because only the cached version for the *current user* for the *current language* will be cleared, while the user change likely changes the menu tree for all languages.
I'd clear all the caches with the
$uid . ':'
prefix in toolbar, so all caches related to this user are cleared.Comment #20
jessebeach CreditAttribution: jessebeach commentedAddressing #18, the patch now clears the toolbar cache based on a cache tag associated with the user ID.
Tests for this case have been added. The tests assure that clearing the cache of one user with a tag does not clear the cache of a second user.
Comment #21
jessebeach CreditAttribution: jessebeach commentedgo bot go.
Comment #23
jessebeach CreditAttribution: jessebeach commentedAck, forgot two function comments.
Comment #25
Wim Leers"Installing: failed to complete installation by setting admin username/password/etc."
-> retest
Comment #26
Wim Leers#23: delay-menu-subtrees-2077279-22.patch queued for re-testing.
Comment #28
Wim LeersHrm, same error as in #25. I guess it *is* true then that this breaks the installer. I just don't see how that is possible.
Comment #29
Gábor HojtsyThis is problematic in a multi-user and/or multilingual scenario.
Let me explain. User A edits User B. The hook is invoked for User B, since user B is changed. The cid is created, yet the cache is cleared for User A, because that is the current user, not User B, who was edited. Advice: you should pass on the user id for the user and use that to delete for a tag, not the current user.
Problem with a multilingual scenario. The cid passed on refers to a specific user with a specific language. If User B was edited, but User B did not have a cached admin menu in the language User A is using to edit User B, none of the language caches will be removed for that use. The delete only happens if there was a cache entry for the user in the *current page language* which may or may not happen on a multilingual site. Advice: ignore getting the cached version for the cid (you'll not even get the cid, you'll get the uid as per above).
Comment #30
jessebeach CreditAttribution: jessebeach commentedOy, excellent points! Easily fixed. Shouldn't take long to update the tests, either. I'll add another test for User A editing User B's entity as well.
Comment #31
jessebeach CreditAttribution: jessebeach commentedThat patch in #23 loads just fine on simplytest.me
http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files...
And I can't reproduce the failed install locally yet. I hope this is just testbot being temporarily cranky.
Comment #32
jessebeach CreditAttribution: jessebeach commentedThe toolbar user cache is now cleared according to a user ID, not a cache ID. So if a specific user ID is passed to the function, just the tag associated with that ID will be cleared. Otherwise, the toolbar user cache is wiped.
The ID for the user entity indicated by
hook_user_update()
is now passed to_toolbar_clear_user_cache()
.A test is added for verifying that the toolbar user cache for User B, when the account for User B is changed by User A, is cleared, whereas the toolbar user cache for User A is not cleared.
The assertions became numerous, so I refactored them into separate test functions as well.
Comment #33
jessebeach CreditAttribution: jessebeach commentedGo bot go. Be kind this time and install. I know you can do it.
Comment #34
Gábor HojtsyI think this nicely covers the separate user scenario, but not the separate language scenario.
Once again I'd totally ignore the cid. Why? Let me try to explain this in a more concrete example :)
Imagine, your admin uses the German UI of the site, and your user being updated only ever visited English pages. So that user only has English menu cached. The admin using the German UI updates that user's roles. This code checks if the updated user had *German* cached menus. The user *does not* have that, so no cache is cleared.
What I would do is to totally ignore looking at a cid and just deleteTags on the uid if the uid was provided or delete all if the uid was not provided. No additional checking needed IMHO. I doubt deleteTags() would barf if there was nothing to delete with that tag, that would be an issue with deleteTags(), not with this code.
Comment #35
jessebeach CreditAttribution: jessebeach commentedRight, makes sense. I'm being totally dense about this!
We're ready to move this patch to #1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient.
Comment #36
Gábor HojtsyI think it may be fine to move it there. It is equally fine IMHO to close that down as a duplicate and elevate this to critical if we consider this fixes all of that issue and no other issues are required.
Comment #37
jessebeach CreditAttribution: jessebeach commentedI've moved this patch to #1927174-20: Complete the work to make the Toolbar administration menu sub tree rendering more efficient.
That issue has more followers and will get more input during reviews.
Removed the
sprint
tag.