Problem/Motivation

The Menu UI module implements several entity hooks for the Menu entity. But actually these hook implementations are clearing caches that need to be cleared even if the Menu UI module is not enabled.

The easiest way to see this is to install the minimal profile from a configuration export (as this does not have the Menu UI module installed):

  1. Install the minimal profile
  2. Make some configuration changes - for example the site name
  3. Use drush config-export to export your configuration to the sync directory
  4. drop your database and re-create an empty on with the same name
  5. Visit your site - you'll be taken to the installer and on the profile select screen you will be able to choose "Use existing configuration"

The menus will be broken.

Proposed resolution

Move the hook implementations into the Menu entity.

Remaining tasks

User interface changes

None

API changes

menu_cache_clear_all() is deprecated in this issue as these are the only calls in core. The replacement is to call \Drupal::cache('menu')->invalidateAll(); directly.

Data model changes

None

CommentFileSizeAuthor
#9 2983603-9.patch5.18 KBalexpott
#9 4-9-interdiff.txt1.75 KBalexpott
#4 2983603-4.patch4.35 KBalexpott
#4 2983603-4.test-only.patch984 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Doing a cache rebuild fixes the issue.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
2983603-4.test-only.patch984 bytes
2983603-4.patch4.35 KB

Here's a test and a fix. Turns out menu_ui is doing things core should be.

The last submitted patch, 4: 2983603-4.test-only.patch, failed testing. View results

bircher’s picture

bircher
Location 🇨🇿
CreditAttribution: bircher as a volunteer and at Nuvole commented
Status: Needs review » Reviewed & tested by the community

The only reason I would leave this as "normal" priority is that cache clearing solves the symptoms. But this means menu_ui is needed for core tor work properly.

The patch just moves the code without restructuring the way things work. Ideally we would put the cache invalidation in the menu block plugin deriver but that can also be a followup.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented 
+++ b/core/modules/system/src/Entity/Menu.php
@@ -78,4 +79,43 @@ public function isLocked() {
+    menu_cache_clear_all();

Given menu_cache_clear_all() is a one-liner, could we do the cache clear directly? It's only used in the three places which are deleted in this patch, meaning we could deprecate it in a follow-up (or here) too.

https://api.drupal.org/api/drupal/core%21includes%21menu.inc/function/me...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Makes sense these are the only usages. I'll update the issue summary and title to better reflection the problem and not the symptom.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: Minimal menus are broken after a configuration install » Move menu_ui_menu_* entity hook implementations to the Menu entity
Issue summary: View changes
FileSize
4-9-interdiff.txt1.75 KB
2983603-9.patch5.18 KB

Thanks for the review @catch.

  • catch committed b2891b8 on 8.7.x 
    Issue #2983603 by alexpott: Move menu_ui_menu_* entity hook...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

  • alexpott committed 37613b4 on 8.6.x authored by catch 
    Issue #2983603 by alexpott: Move menu_ui_menu_* entity hook...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Related issues: +#2633878: Finalize cacheability for plugins, +#2989461: Menu entity should clear plugin.manager.block even when block module not installed

The follow-up for derivative cache tags already exists - see #2633878: Finalize cacheability for plugins

Also created #2989461: Menu entity should clear plugin.manager.block even when block module not installed as a result of re-reviewing this issue.

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

publish the change record