We have these two functions, menu_list_system_menus() and menu_ui_get_menus(), which just sorta return lists of the menus (i.e., menu entities) known to the system. These were needed in D7, since menus weren't config, and we used to call them a lot more. Now that menus are just config entities, we can replace these functions with standard entity system calls.
Proposed Resolution
Deprecate both functions for removal in D10.
Remaining Tasks
Commit the merge request.
API changes
Two old functions will be deprecated.
UI changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|
Issue fork drupal-1882552
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
sunRelated: #1882218: Remove static menu link creation for menus in menu_enable() and elsewhere
Comment #2
andypostThere's general discussion #1966298: Introduce menu link bundles per menus happens
Also related
#1937384: Remove 'bundles' key from the MenuLink class annotation
#1945226: Add language selector on menus
Comment #3
andypostRelated issue #1831928: Support a 'locked' property on ConfigEntities from #1983548: Convert contact message entities to the new Entity Field API
Comment #4
andypostRelated #1842850: Untangle menu_link access control from _menu_link_translate() and friends just moves to access controller
Comment #5
YesCT CreditAttribution: YesCT commentedrelated #2019647: Use EntityListController for menus
Comment #6
andypostSuppose this logic could be moved to MenuAccessController after #2012916: Implement access controller for the menu and menu link entity
Comment #7
tim.plunkett#2084197: Uninstalling menu module removes all custom menus doesn't remove this, but does relegate it to only be used when we actually care that these are from system (system_preprocess_block, menu_set_active_menu_names, etc)
Comment #14
andypostComment #15
volegerComment #16
andypostIt needs CR when there's agreement on final place
I think it could be placed to Menu entity
After replacing usage I think
WizardPluginBase
needs follow-up to get rid of condition (dependency on muni_ui module) because Menu entity defined in system moduleAlso http://grep.xnddx.ru/search?text=menu_ui_get_menus widely used in contrib and probably needs another follow-up
Comment #17
andypostFiled patch #3021804: Remove optional dependency on menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase
Comment #18
volegerFilled CR https://www.drupal.org/node/3027453
Comment #19
andypostI still not sure that it needed at all.
Contrib mostly using
menu_ui_get_menus()
and not many places, so probably better to inline this array thereComment #20
alexpottWe never call this with something other than TRUE. We should deprecate that. And remove the menu_list_system_menus() completely. It's not necessary for core. Before doing this we should have a look in contrib to see how it is used.
In fact thinking about this some more perhaps we can remove this too and replace with Menu::loadMultiple() and tell people to sort it themselves. The asort() doesn't look that useful here anyway.
The only usage with FALSE I have in my contrib is devel but the could be replaced by a
Menu::loadMultiple()
.Comment #21
dawehnerI talked with @alexpott about this specific issue and we came up with a 3 step plam:
menu_list_system_menus()
intomenu_ui_get_menus()
and deprecated it: This issuemenu_ui_get_menus()
in #3021804: Remove optional dependency on menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase as its not neededmenu_ui_get_menus
as its kind of broken: #3028612: Deprecate menu_ui_get_menus() and replace all of its usagesComment #22
alexpottComment #23
andypostLooks it blocked on #3021804: Remove optional dependency on menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase
Also CR should tell that this function should not be used and require sorting of menus
Comment #26
andypostreroll & fix
Comment #27
andypostComment #29
volegerRerolled against 9.0.x
Comment #32
andypostRe-roll for 9.3 and added deprecation test
Comment #33
andypostGetting in the #3021804: Remove optional dependency on menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase as first one still looks preferable
Comment #34
andypostComment #36
daffie CreditAttribution: daffie commentedComment #37
andypostRe-roll after #3021804: Remove optional dependency on menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase
Maybe it makes sense to combine it now with #3028612: Deprecate menu_ui_get_menus() and replace all of its usages
But it should not stop plan from #21
Comment #38
longwaveThis is simple now the Views wizard issue is resolved.
Comment #39
volegerYes, it is the easy one and is ready. +1 for RTBC
Comment #40
alexpottI think we can re-write this in a better way that we will result in less throwing away of objects... by using an entity query:
Comment #41
volegerComment #42
volegerComment #44
volegerAddressed #40
Comment #45
phenaproximaCouple of nitpicks, and one thing that I think may be incorrect and needing explicit test coverage. Sorry!
Also, can we update the issue summary? Its proposed resolution seems to be pretty far out of date, as far as I can tell...
Comment #47
alexpottFWIW there is another option here I like. We never call menu_ui_get_menus(FALSE) so we could and probably should deprecate the $all argument and therefore we don't need to make any changes to menu_ui_get_menus() other than to add the @trigger_error()!
Comment #48
alexpottA big advantage to #47 is that in Drupal 10 we won't be hardcoding menu IDs anywhere.
Comment #49
alexpottThe only use of menu_ui_get_menus(FALSE) in contrib looks like devel - see http://codcontrib.hank.vps-private.net/search?text=menu_ui_get_menus - imo that's not enough to keep it.
The one construction that is interesting is:
This code is flawed because it assumes the menu config is only around when menu_ui is enabled. So that points to the possibility of completely deprecating menu_ui_get_menus() too - and suggesting people do:
instead.
Comment #50
andypost@alexpott that's exactly what I did in related issue
The usage is https://git.drupalcode.org/project/devel/-/blob/4.x/devel_generate/src/P...
Comment #51
tedbowre #49 @alexpott I think that seems like a good idea. Done
Comment #52
phenaproximaI've updated the change record to reflect the fact that these functions are deprecated in Drupal 9.3.0. Otherwise, I think it is accurate as best I can tell. Removing the tag.
Comment #53
phenaproximaRewrote the IS to reflect what we're actually up to.
Comment #54
alexpottMarked #3028612: Deprecate menu_ui_get_menus() and replace all of its usages as a duplicate of this one since this one is now deprecating menu_ui_get_menus() too.
Comment #55
tim.plunkettThanks @phenaproxima! This ended up being very straightforward, thanks @alexpott for the clean suggestion
Comment #56
phenaproximaComment #57
alexpottComment #58
alexpottStill need to fix
menu_ui_form_node_type_form_alter()
to not usemenu_ui_get_menus()
- see #3028612: Deprecate menu_ui_get_menus() and replace all of its usages for how.Comment #59
phenaproximaComment #60
andypost@phenaproxima See #3028612-16: Deprecate menu_ui_get_menus() and replace all of its usages why formatting fails
Comment #61
andypostFixed code-style and added
asort()
as old code doing - it means a lack of test for sorting for menus (probably needs follow-up)PS: unpublished patches as MR used now
Comment #62
tedbow@andypost got catch on the
asort()
!re
Did we actually have test for this before? I don't see an existing test for
menu_ui_get_menus()
?Since we are deprecating
menu_ui_get_menus()
and we just have suggested code in the change record for how to replace it I don't think we need test coverage for that in particular.But maybe we need to add a follow-up to assert the menu in the Node Type form so then the
asort()
could not be accidently removed inmenu_ui_form_node_type_form_alter()
I couldn't find any test that covers
menu_ui_form_node_type_form_alter()
now but I could be wrong.Comment #63
tedbowAdded the follow up #3221493: Add test coverage for menu order in node type form
I think this looks good now!
Comment #64
alexpottCommitted 7bc0a6d and pushed to 9.3.x. Thanks!
Comment #66
andypostFiled MR to remove usage in devel https://gitlab.com/drupalspoons/devel/-/merge_requests/68