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
Related Issues
Inspired by #2082499: Uninstalling action module removes actions created by the user module
Comment | File | Size | Author |
---|---|---|---|
#22 | menu-2084197-21.patch | 32.24 KB | tim.plunkett |
#22 | interdiff.txt | 646 bytes | tim.plunkett |
#20 | interdiff.txt | 6.55 KB | tim.plunkett |
#20 | menu-2084197-20.patch | 32.24 KB | tim.plunkett |
#13 | menu-2084197-13.patch | 32.77 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettOkay, 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 nowmenu_menu_block:FOO
.This also blocks #2083871: Allow menu blocks to be placed from the menu page
Comment #2
aspilicious CreditAttribution: aspilicious commentedThis looks strange. Isn't this the same as $properties['locked'] = $this->get(locked)
OR
$properties['locked'] = isLocked()
Comment #3
tim.plunkettYes it is, but other properties should be protected too, like description, they're just out of scope.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedi 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.
Comment #5
tim.plunkett@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
Comment #6
pwolanin CreditAttribution: pwolanin commentedIt would make more sense to use system_menu for the IDs since all the storage is system.menu
Comment #7
tim.plunkettGood point.
Comment #9
tim.plunkett#7: menu-2084197-7.patch queued for re-testing.
Comment #10
andypostExcept strange code in
getExportProperties()
that needs inline comment - it's readyAny reason to make it so complex?
This needs comment and I think better simplify this to
$properties['locked'] = $this->get('locked')
no way to get rid of it?
Comment #11
tim.plunkett1) 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.
Comment #12
andypostI 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
Comment #13
tim.plunkettAdded 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.
Comment #14
andypostI think it's rtbc for the scope but leave others for review of locking
Comment #16
tim.plunkett#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
failures...
Comment #17
andypostFiled separate issue to discuss locking #2084567: Implement a EntityLockedInterface and service to allow locking entities
Comment #18
catchHaven't looked the patch but tagging with upgrade path.
Comment #19
EclipseGc CreditAttribution: EclipseGc commentedI 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
Comment #19.0
tim.plunkettUpdated
Comment #20
tim.plunkettRerolled after some suggestions from xjm in IRC.
Comment #21
andypostSuppose bot agree too!
Comment #22
tim.plunkettReferenced the wrong issue there, should have been #2030645: Expand Menu with methods
Comment #23
andypostMakes sense
Comment #24
xjmI 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:
admin/structure/menu/add
and creating a "Bunnies" menu....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
Comment #25
andypost@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.
Comment #26
tim.plunkettI agree with #25.
@andypost opened #2085243: Rename Menu module into Menu UI module, thanks
Comment #27
tim.plunkettThis blocks #2083871: Allow menu blocks to be placed from the menu page, so tagging to keep track.
Comment #28
webchickThis 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!
Comment #29
andypost@webchick I still think that better to use interface as we done in #2044583: Add EntityChangedInterface to allow entities with "changed" field to be properly cached
So here's a follow-up #2084567: Implement a EntityLockedInterface and service to allow locking entities
Comment #30
catchComment #31
EclipseGc CreditAttribution: EclipseGc commentedOr 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
Comment #32
YesCT CreditAttribution: YesCT commentedcaused #2085603: Admin path of blocks changed, menus not prefixed with menu- anymore in config_translation
Comment #33
tim.plunkettAdded https://drupal.org/node/2086185
Comment #34
tim.plunkettComment #35.0
(not verified) CreditAttribution: commentedUpdated