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: Get rid of menu_list_system_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

Files: 
CommentFileSizeAuthor
#22 menu-2084197-21.patch32.24 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,635 pass(es).
[ View ]
#22 interdiff.txt646 bytestim.plunkett
#20 interdiff.txt6.55 KBtim.plunkett
#20 menu-2084197-20.patch32.24 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#13 menu-2084197-13.patch32.77 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,893 pass(es).
[ View ]
#13 interdiff.txt563 bytestim.plunkett
#7 menu-2084197-7.patch32.7 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,955 pass(es).
[ View ]
#7 interdiff.txt11.7 KBtim.plunkett
#5 menu-2084197-5-FAIL.patch1.19 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,588 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#5 menu-2084197-5-PASS.patch29.47 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,925 pass(es).
[ View ]
#1 menu-2084197-1.patch28.28 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,539 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+Needs tests, +blocker
StatusFileSize
new28.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,539 pass(es).
[ View ]

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

+++ 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()

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

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.

Issue tags:-Needs tests+beta blocker
StatusFileSize
new29.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,925 pass(es).
[ View ]
new1.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,588 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

@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

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

StatusFileSize
new11.7 KB
new32.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,955 pass(es).
[ View ]

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.

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

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

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?

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: Get rid of menu_list_system_menus() to figure out the rest.

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

StatusFileSize
new563 bytes
new32.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,893 pass(es).
[ View ]

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.

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.

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

Issue tags:+D8 upgrade path

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

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

Issue summary:View changes

Updated

StatusFileSize
new32.24 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new6.55 KB

Rerolled after some suggestions from xjm in IRC.

Status:Needs review» Reviewed & tested by the community

Suppose bot agree too!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new646 bytes
new32.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,635 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Makes sense

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: Get rid of menu_list_system_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 content or config entities' soft dependencies and use this to present a confirm form on module uninstall

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

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

Issue tags:+Block UI overhaul

This blocks #2083871: Allow menu blocks to be placed from the menu page, so tagging to keep track.

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!

Title:Uninstalling menu module removes all custom menusChange notice: Uninstalling menu module removes all custom menus

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

Title:Change notice: Uninstalling menu module removes all custom menusUninstalling menu module removes all custom menus
Category:task» bug
Priority:Major» Critical
Issue tags:-Needs change record, -Needs followup

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

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

Issue summary:View changes

Updated