Problem/Motivation

_toolbar_clear_user_cache() added in #2077279: Only load the admin menu subtrees if the toolbar tray is oriented vertically; only request subtrees when local cache is stale is called from four hook implementations to delete the cached hash of per-user toolbar subtrees.

Proposed resolution

Remove _toolbar_clear_user_cache() and:
toolbar_modules_installed/uninstalled() - toolbar cache bin is automatically cleared on module install/uninstall.
toolbar_user_update() - user id cache tag is already invalidated in Entity::postSave()
toolbar_user_role_update() - Add a user_role list cache tag to the cached hash so it is deleted Entity::postSave().

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
_toolbar_clear_user_cache-removed.patch2.01 KBolli
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Great cleanups!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 2283584 and pushed to 8.0.x. Thanks!

  • alexpott committed 2283584 on 8.0.x 
    Issue #2312657 by olli: Remove _toolbar_clear_user_cache().
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

This is indeed a wonderful clean-up — thanks, olli!

I have only one concern:

+++ b/core/modules/toolbar/toolbar.module
@@ -525,40 +525,12 @@ function _toolbar_get_subtrees_hash($langcode) {
-function toolbar_modules_installed($modules) {
-  _toolbar_clear_user_cache();
-}
-
-/**
- * Implements hook_modules_uninstalled().
- */
-function toolbar_modules_uninstalled($modules) {
-  _toolbar_clear_user_cache();
-}

Shouldn't the removal of these two hooks be replaced by setting the array('extension' => TRUE) cache tag?

It's strictly speaking unnecessary, but only because (un)installing modules clears all caches. It's better to be explicit about the dependencies.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Closed (fixed) » Active

I'd still love feedback regarding #4.

olli’s picture

olli CreditAttribution: olli commented

Good question. I found only views using that tag and that it gets invalidated when theme info is refreshed which happens on module/theme (un)install but the code comments refer to clearing library info which does not use it. It's also deleted by views on module (un)install. This happens on every module/theme (un)install while these hooks were invoked after the modules were (un)installed. It's not clear to me when should I use that tag. I think we could:
a) remove the tag
b) add/improve docs for it
c) improve docs for testModuleStatusChangeSubtreesHashCacheClear in case module (un)install stops clearing all bins.

What do you think?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

The reason those hooks were listening for module (un)installation, is because modules can implement hook_toolbar(), which can add things to the toolbar.

However, I didn't realize we had ToolbarAdminMenuTest::testModuleStatusChangeSubtreesHashCacheClear(), which indeed proves that it's still working fine, because module (un)installation clears all caches. In which case I think we should do #7.c) :)

Thanks!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Active » Closed (fixed)