Updated: Comment #19

Problem/Motivation

The Menu entity type is provided by the system module. The menu module acts merely as a UI to add custom menus (not unlike action.module, views_ui.module, field_ui.module)
However, the config prefix for menu is menu.menu, not system.menu. This means all menus will be deleted when menu.module is uninstalled.

The system menus have their own black magic to bypass all of this, and #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus() discusses killing parts of that.

Proposed resolution

Change the config prefix to match the owner of the entity type
Unify the two menu Block plugins
Add an isLocked() method to menus

Remaining tasks

N/A

User interface changes

N/A

API changes

API changes:

menu.menu.*.yml is now system.menu.*.yml
SystemMenuBlock and MenuBlock plugins are merged
Plugin ID for menu blocks is now system_menu_block:FOO, used to be either system_menu_block:menu-FOO or menu_menu_block:FOO

API additions:

MenuInterface::isLocked()
system.menu.*.yml files can ship with locked: 1 to prevent deletion

Inspired by #2082499: Uninstalling action module removes actions created by the user module

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +blocker
FileSize
28.28 KB

Okay, here is a summary of changes:

Added MenuInterface::isLocked(), exactly like NodeTypeInterface and DateFormatInterface.
menu_list_system_menus() served two purposes, one to identify the system menus, and one to identify which menus could not be deleted. The latter use case is replaced by $menu->isLocked()
The separation between MenuBlock and SystemMenuBlock is no more. All plugins that were system_menu_block:menu-FOO are now menu_menu_block:FOO.

This also blocks #2083871: Allow menu blocks to be placed from the menu page

aspilicious’s picture

+++ b/core/modules/system/lib/Drupal/system/Entity/Menu.php
@@ -61,4 +61,32 @@ class Menu extends ConfigEntityBase implements MenuInterface {
+    $names = array(
+      'locked',
+    );
+    foreach ($names as $name) {
+      $properties[$name] = $this->get($name);
+    }

This looks strange. Isn't this the same as $properties['locked'] = $this->get(locked)

OR

$properties['locked'] = isLocked()

tim.plunkett’s picture

Yes it is, but other properties should be protected too, like description, they're just out of scope.

Anonymous’s picture

i looked through the patch, and most of it i don't grok, but that's ok.

this may be by design, but thought i'd check - $entity->isLocked() doesn't seem to stop these menu config thingies from being deleted at the API level? it's only used in forms.

please ignore me if that's how it's meant to be.

tim.plunkett’s picture

@beejeebus You are correct, that's how I intended it. It's also how node types and date formats work.

If we don't get this in before beta, we're going to have a new hellish upgrade path to write.

Added a test similar to ActionUninstallTest

pwolanin’s picture

It would make more sense to use system_menu for the IDs since all the storage is system.menu

tim.plunkett’s picture

FileSize
11.7 KB
32.7 KB

Good point.

Status: Needs review » Needs work
Issue tags: -API change, -beta blocker, -Configuration system, -Configurables, -blocker

The last submitted patch, menu-2084197-7.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change, +beta blocker, +Configuration system, +Configurables, +blocker

#7: menu-2084197-7.patch queued for re-testing.

andypost’s picture

Except strange code in getExportProperties() that needs inline comment - it's ready

  1. +++ b/core/modules/system/lib/Drupal/system/Entity/Menu.php
    @@ -61,4 +61,32 @@ class Menu extends ConfigEntityBase implements MenuInterface {
    +    $names = array(
    +      'locked',
    +    );
    +    foreach ($names as $name) {
    +      $properties[$name] = $this->get($name);
    +    }
    

    Any reason to make it so complex?
    This needs comment and I think better simplify this to $properties['locked'] = $this->get('locked')

  2. +++ b/core/modules/system/system.module
    @@ -2283,17 +2283,23 @@ function system_user_timezone(&$form, &$form_state) {
    +      $menus = menu_list_system_menus();
    +      if (isset($menus[$derivative])) {
    

    no way to get rid of it?

tim.plunkett’s picture

1) I just answered this in #3. Eventually we'll make $description protected as well and we'll need that loop. It's straight from every other config entity with protected properties, I see no reason not to leave it.

2) We don't want to get rid of menu_list_system_menus() completely, and that specific usecase is only applied to the 5 system menus, regardless of their locked state. We have #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus() to figure out the rest.

andypost’s picture

I still sure that it needs comment and @todo with follow-up issue for other protected properties.

Also having 2 kinds of "locking" (property and menu_list_system_menus()) brings more confusion

tim.plunkett’s picture

FileSize
563 bytes
32.77 KB

Added a @todo.

As of this patch, menu_list_system_menus() has nothing to do with a locked menu at all. It is still used for the default list of menus, the menus to add aria roles to, etc. As I've stated 3 times, there is a pre-existing issue to handle that.

andypost’s picture

I think it's rtbc for the scope but leave others for review of locking

Status: Needs review » Needs work
Issue tags: -API change, -beta blocker, -Configuration system, -Configurables, -blocker

The last submitted patch, menu-2084197-13.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change, +beta blocker, +Configuration system, +Configurables, +blocker

#13: menu-2084197-13.patch queued for re-testing.

The locking is 100% identical to DateFormat, and the same method as NodeType.

The wonderful random

ExpandDrupal\search\Tests\SearchCommentTest
ExpandDrupal\system\Tests\System\AccessDeniedTest
ExpandDrupal\system\Tests\System\PageNotFoundTest

failures...

andypost’s picture

catch’s picture

Issue tags: +D8 upgrade path

Haven't looked the patch but tagging with upgrade path.

EclipseGc’s picture

I didn't get to review this at length, but nothing stands out to me as being offensive in the slightest. There might be a few consistency issues, I didn't have time to crosscheck everything I saw, but consider me generally +1 to this.

Eclipse

tim.plunkett’s picture

Issue summary: View changes

Updated

tim.plunkett’s picture

FileSize
32.24 KB
6.55 KB

Rerolled after some suggestions from xjm in IRC.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Suppose bot agree too!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
646 bytes
32.24 KB

Referenced the wrong issue there, should have been #2030645: Expand Menu with methods

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense

xjm’s picture

I reviewed the patch as well and everything seems sound to me. We already have precedent for the locking pattern, and everything else I would have called out is already covered by:
#2030645: Expand Menu with methods
#1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()

Test coverage is exposed in #5. I manually tested by:

  1. Installing 8.x standard.
  2. Going to admin/structure/menu/add and creating a "Bunnies" menu.
  3. For fun, adding a link to it and placing a block for it on the page. Yay, the blocks UI is getting so much better.
  4. Disabling the menu module. In HEAD, my menu disappears. With the patch, it remains.
  5. Uninstalling the menu module.
  6. Re-enabling the menu module. In HEAD:

    Drupal\Component\Plugin\Exception\PluginException: The plugin (menu_menu_block:menu-bunnies) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 62 of /home/s0ede74483c7115a/www/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php).

    ...and my site is broken forever.

    With the patch all is well.

The only thing I'd recommend is that we consider explicitly renaming the menu module to be the menu UI module and updating its help text etc. accordingly. Otherwise, if it's the "custom menu" module, then removal of its data on uninstallation would be expected, and this might not be the correct solution. Thoughts?

Also loosely related:
#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall

andypost’s picture

@xjm, menu module is jsut a menu_ui module for menus and menu link entities now. I think it's awesome to add some menus via ui in development and disable menu module at production server.

tim.plunkett’s picture

I agree with #25.
@andypost opened #2085243: Rename Menu module into Menu UI module, thanks

tim.plunkett’s picture

Issue tags: +Block UI overhaul
webchick’s picture

Category: bug » task
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: -beta blocker +Needs change record, +Needs followup

This looks good to me, and seems to be consistent with the way we use isLocked() in other places.

One suggestion that came up from Dave Reid in IRC is to file a follow-up to move the isLocked() stuff to the base ConfigEntity class, rather than implementing it on a per-entity basis. I think this makes sense. Tagging for that.

This will also need a change notice due to the removal of the "menu-" prefix on these plugin IDs. Tagging for that, too.

Committed and pushed to 8.x. Thanks!

andypost’s picture

catch’s picture

Title: Uninstalling menu module removes all custom menus » Change notice: Uninstalling menu module removes all custom menus
EclipseGc’s picture

Or just provide an additional abstract subclass that includes the typical functionality. Then you can extend ConfigEntity if you don't need locking, or LockableConfigEntity if you need locking (or whatever). And adding that functionality to an existing config entity becomes really really easy if it conforms to the needs of the majority.

Eclipse

YesCT’s picture

tim.plunkett’s picture

Title: Change notice: Uninstalling menu module removes all custom menus » Uninstalling menu module removes all custom menus
Category: task » bug
Priority: Major » Critical
Issue tags: -Needs change record, -Needs followup
tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Active » Fixed

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

Anonymous’s picture

Issue summary: View changes

Updated