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.

#1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient

Original report by @jessebeach

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Status: Active » Needs review
Issue tags: +sprint, +Spark, +toolbar-followup
FileSize
4.75 KB

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

jessebeach’s picture

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

jessebeach’s picture

Title: Only load the admin menu subtree items if the toolbar tray is oriented vertically » Only load the admin menu subtrees if the toolbar tray is oriented vertically; only request subtrees when local cache is stale

Title update.

jessebeach’s picture

FileSize
4.44 KB

Interdiff for #2.

Status: Needs review » Needs work

The last submitted patch, delay-menu-subtrees-2077279-2.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/toolbar/toolbar.module
@@ -647,3 +651,35 @@ function _toolbar_get_subtree_hash() {
+/**
+ * Implements hook_ENTITY_TYPE_update().
+ */
+function toolbar_menu_update(MenuInterface $menu) {
+  if ($menu->id() === 'admin') {
+    _clear_toolbar_cache();
+  }

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

jessebeach’s picture

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

function toolbar_modules_installed($modules) {}
function toolbar_modules_enabled($modules) {}
function toolbar_modules_disabled($modules) {}
function toolbar_modules_uninstalled($modules) {}
function toolbar_menu_link_update(MenuLinkInterface $menu_link) {}
function toolbar_user_update(UserInterface $user) {}
function toolbar_user_role_update(RoleInterface $role) {}

I'm working on tests for each invalidation case right now and will have those posted shortly.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
15.66 KB

Now with tests! I eliminated the hook_modules_installed and hook_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:

function toolbar_modules_enabled($modules) {}
function toolbar_modules_disabled($modules) {}
function toolbar_menu_link_update(MenuLinkInterface $menu_link) {}
function toolbar_user_update(UserInterface $user) {}
function toolbar_user_role_update(RoleInterface $role) {}

Status: Needs review » Needs work

The last submitted patch, delay-menu-subtrees-2077279-8.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
9.19 KB

Interdiff for #8.

jessebeach’s picture

Status: Needs review » Needs work

The last submitted patch, delay-menu-subtrees-2077279-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
615 bytes
14.55 KB

Fixing the language id() method invocation the new tests pass for me. Next up: review :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
    @@ -0,0 +1,194 @@
    +  protected $admin_user;
    +  protected $rid;
    

    Let's document these.

  2. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
    @@ -0,0 +1,194 @@
    +  /**
    +   * Tests for a tab and tray provided by a module implementing hook_toolbar().
    +   */
    +  function testToolbarAdminMenuSubtreeCache() {
    

    This comment sounds like copy-paste from a prior test.

  3. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
    @@ -0,0 +1,194 @@
    +    // Request a new page to refresh the drupalSettings object.
    +    $this->drupalGet('test-page');
    +    $this->assertResponse(200);
    +    $new_subtree_hash = $this->getSubtreesHash();
    +
    +    // Assert that the old admin menu subtree hash and the new admin menu
    +    // subtree hash are different.
    +    $this->assertNotEqual($original_subtree_hash, $new_subtree_hash, 'The user-specific subtree menu hash has been updated.');
    +
    +    // Save the new subtree hash as the original.
    +    $original_subtree_hash = $new_subtree_hash;
    

    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.

  4. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
    @@ -0,0 +1,194 @@
    +    // Test a user update.
    +    $edit = array();
    +    $edit[$rid . '[administer taxonomy]'] = FALSE;
    

    // Test a permission update.

  5. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
    @@ -0,0 +1,194 @@
    +    // Test a role update.
    +    $rid = $this->drupalCreateRole(array('administer content types'));
    +
    +    // Assign the role to the user.
    +    $this->drupalPost('user/' . $this->admin_user->id() . '/edit', array("roles[$rid]" => $rid), t('Save'));
    +    $this->assertText(t('The changes have been saved.'));
    

    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.

  6. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Tests/ToolbarAdminMenuTest.php
    @@ -0,0 +1,194 @@
    +    return substr($settings['toolbar']['subtreesPath'], 18);
    

    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.

  7. +++ b/core/modules/toolbar/toolbar.module
    @@ -642,3 +651,49 @@ function _toolbar_get_subtree_hash() {
    +/**
    + * Implements hook_ENTITY_TYPE_update().
    + */
    +function toolbar_user_update(UserInterface $user) {
    +  _clear_toolbar_cache();
    +}
    

    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.

  8. +++ b/core/modules/toolbar/toolbar.module
    @@ -642,3 +651,49 @@ function _toolbar_get_subtree_hash() {
    +/**
    + * Implements hook_ENTITY_TYPE_update().
    + */
    +function toolbar_user_role_update(RoleInterface $role) {
    +  _clear_toolbar_cache();
    +}
    

    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.

  9. +++ b/core/modules/toolbar/toolbar.module
    @@ -642,3 +651,49 @@ function _toolbar_get_subtree_hash() {
    +/**
    + * Clears the Toolbar cache.
    + */
    +function _clear_toolbar_cache() {
    +  if (!cache('toolbar')->isEmpty()) {
    +    cache('toolbar')->deleteAll();
    +  }
    +}
    

    Respect Drupal namespacing by starting this function off as _toolbar_*(), not _clear_*(). So _toolbar_clear_cache().

jessebeach’s picture

Status: Needs work » Needs review
FileSize
12.33 KB
16.64 KB

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.

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.

Without implementing hook_user_update, changing the permission on a role fails:

// Test a permission change.
$edit = array();
$edit[$this->rid . '[administer taxonomy]'] = FALSE;
$this->drupalPost('admin/people/permissions', $edit, t('Save permissions'));

// Assert that the subtrees hash has been altered because the subtrees
// structure changed.
$this->assertDifferentHash();
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.

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 that drupalSettings 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.

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.

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.

Status: Needs review » Needs work
Issue tags: -sprint, -Spark, -toolbar-followup

The last submitted patch, delay-menu-subtrees-2077279-15.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Spark, +toolbar-followup
Gábor Hojtsy’s picture

I could not reproduce the installation problem reported by the testbot on the UI, so sent for a retest. In code review, found this:

+++ b/core/modules/toolbar/toolbar.module
@@ -642,3 +650,66 @@ function _toolbar_get_subtree_hash() {
+/**
+ * Implements hook_ENTITY_TYPE_update().
+ */
+function toolbar_user_update(UserInterface $user) {
+  $cid = _toolbar_get_user_cid();
+  _toolbar_clear_user_cache($cid);
+}
...
+function _toolbar_get_user_cid() {
+  $user = Drupal::currentUser();
+  return $user->id() . ':' . Drupal::languageManager()->getLanguage(Language::TYPE_INTERFACE)->id;
+}

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.

Status: Needs review » Needs work

The last submitted patch, delay-menu-subtrees-2077279-15.patch, failed testing.

jessebeach’s picture

Addressing #18, the patch now clears the toolbar cache based on a cache tag associated with the user ID.

function _toolbar_clear_user_cache($cid = NULL) {
  $cache = cache('toolbar');
  if (!$cache->isEmpty()) {
    // Clear by the 'user' tag in order to delete all caches, in any language,
    // associated with this user.
    if (isset($cid) && $cache->get($cid)) {
      $cache->deleteTags(array('user' => array(_toolbar_get_user_id())));
    } else {
      $cache->deleteAll();
    }
  }
}

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.

jessebeach’s picture

Status: Needs work » Needs review

go bot go.

Status: Needs review » Needs work

The last submitted patch, delay-menu-subtrees-2077279-20.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
22.03 KB
722 bytes

Ack, forgot two function comments.

Status: Needs review » Needs work

The last submitted patch, delay-menu-subtrees-2077279-22.patch, failed testing.

Wim Leers’s picture

"Installing: failed to complete installation by setting admin username/password/etc."

-> retest

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -sprint, -Spark, -toolbar-followup

Status: Needs review » Needs work
Issue tags: +sprint, +Spark, +toolbar-followup

The last submitted patch, delay-menu-subtrees-2077279-22.patch, failed testing.

Wim Leers’s picture

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

Gábor Hojtsy’s picture

+++ b/core/modules/toolbar/toolbar.module
@@ -628,17 +638,98 @@ function toolbar_library_info() {
+/**
+ * Implements hook_ENTITY_TYPE_update().
+ */
+function toolbar_user_update(UserInterface $user) {
+  $cid = _toolbar_get_user_cid();
+  _toolbar_clear_user_cache($cid);
+}
...
+function _toolbar_clear_user_cache($cid = NULL) {
+  $cache = cache('toolbar');
+  if (!$cache->isEmpty()) {
+    // Clear by the 'user' tag in order to delete all caches, in any language,
+    // associated with this user.
+    if (isset($cid) && $cache->get($cid)) {
+      $cache->deleteTags(array('user' => array(_toolbar_get_user_id())));
+    } else {
+      $cache->deleteAll();
+    }
+  }
+}

This 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).

jessebeach’s picture

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

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

jessebeach’s picture

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

jessebeach’s picture

The 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().

/**
 * Implements hook_ENTITY_TYPE_update().
 */
function toolbar_user_update(UserInterface $user) {
  _toolbar_clear_user_cache($user->id());
}

/**
 * Returns a cache ID from the user and language IDs.
 *
 * @param string $uid
 *   A user ID.
 *
 * @return string
 *   A unique cache ID for the user.
 */
function _toolbar_get_user_cid($uid) {
  return $uid . ':' . \Drupal::languageManager()->getLanguage(Language::TYPE_INTERFACE)->id;
}

/**
 * Clears the Toolbar user cache.
 *
 * @param string $uid
 *   (optional) The user ID to clear.
 */
function _toolbar_clear_user_cache($uid = NULL) {
  $cache = cache('toolbar');
  if (!$cache->isEmpty()) {
    // Clear by the 'user' tag in order to delete all caches, in any language,
    // associated with this user.
    if (isset($uid)) {
      $cid = _toolbar_get_user_cid($uid);
    }
    if (isset($cid) && $cache->get($cid)) {
      $cache->deleteTags(array('user' => array($uid)));
    } else {
      $cache->deleteAll();
    }
  }
}

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.

jessebeach’s picture

Status: Needs work » Needs review

Go bot go. Be kind this time and install. I know you can do it.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/toolbar/toolbar.module
@@ -628,17 +638,94 @@ function toolbar_library_info() {
+function _toolbar_clear_user_cache($uid = NULL) {
+  $cache = cache('toolbar');
+  if (!$cache->isEmpty()) {
+    // Clear by the 'user' tag in order to delete all caches, in any language,
+    // associated with this user.
+    if (isset($uid)) {
+      $cid = _toolbar_get_user_cid($uid);
+    }
+    if (isset($cid) && $cache->get($cid)) {
+      $cache->deleteTags(array('user' => array($uid)));
+    } else {
+      $cache->deleteAll();
+    }

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

jessebeach’s picture

Status: Needs work » Needs review
FileSize
604 bytes
23.83 KB

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.

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

Gábor Hojtsy’s picture

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

jessebeach’s picture

Status: Needs review » Closed (duplicate)
Issue tags: -sprint

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