Deprecate menu_list_system_menus() and menu_ui_get_menus()

Related: #1882218: Remove static menu link creation for menus in menu_enable() and elsewhere

related #2019647: Use EntityListController for menus

#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)

Filled CR https://www.drupal.org/node/3027453

+++ b/core/modules/menu_ui/menu_ui.module
@@ -440,7 +440,7 @@ function menu_ui_form_node_type_form_builder($entity_type, NodeTypeInterface $ty
 function menu_ui_get_menus($all = TRUE) {

We 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().

I talked with @alexpott about this specific issue and we came up with a 3 step plam:

1. Inline menu_list_system_menus() into menu_ui_get_menus() and deprecated it: This issue
2. Get rid of the usage of menu_ui_get_menus() in #3021804: Remove optional dependency on menu_ui module in \Drupal\views\Plugin\views\wizard\WizardPluginBase as its not needed
3. Get rid of menu_ui_get_menus as its kind of broken: #3028612: Deprecate menu_ui_get_menus() and replace all of its usages

Rerolled against 9.0.x

This is simple now the Views wizard issue is resolved.

Yes, it is the easy one and is ready. +1 for RTBC

+++ b/core/modules/menu_ui/menu_ui.module
@@ -433,7 +433,8 @@ function menu_ui_form_node_type_form_builder($entity_type, NodeTypeInterface $ty
 function menu_ui_get_menus($all = TRUE) {
   if ($custom_menus = Menu::loadMultiple()) {
     if (!$all) {
-      $custom_menus = array_diff_key($custom_menus, menu_list_system_menus());
+      $menus = ['tools', 'admin', 'account', 'main', 'footer'];
+      $custom_menus = array_diff_key($custom_menus, array_combine($menus, $menus));
     }
     foreach ($custom_menus as $menu_name => $menu) {
       $custom_menus[$menu_name] = $menu->label();

I 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:

  if (!$all) {
    $menus_to_load = \Drupal::entityTypeManager()->getStorage('menu')->getQuery()->condition('id', ['tools', 'admin', 'account', 'main', 'footer'], 'NOT IN')->execute();
  }
  if ($custom_menus = Menu::loadMultiple($menus_to_load ?? NULL)) {
    foreach ($custom_menus as $menu_name => $menu) {
      $custom_menus[$menu_name] = $menu->label();
    }
    asort($custom_menus);
  }
  return $custom_menus;

Addressed #40

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

FWIW 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()!

A big advantage to #47 is that in Drupal 10 we won't be hardcoding menu IDs anywhere.

The 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:

  $menu_enabled = $this->moduleHandler->moduleExists('menu_ui');
  $menus = $menu_enabled ? menu_ui_get_menus() : menu_list_system_menus();

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:

$menus =  array_map(function ($menu) { return $menu->label(); },  \Drupal\system\Entity\Menu::loadMultiple());

instead.

re #49 @alexpott I think that seems like a good idea. Done

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

Rewrote the IS to reflect what we're actually up to.

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

Thanks @phenaproxima! This ended up being very straightforward, thanks @alexpott for the clean suggestion

Still need to fix menu_ui_form_node_type_form_alter() to not use menu_ui_get_menus() - see #3028612: Deprecate menu_ui_get_menus() and replace all of its usages for how.

@andypost got catch on the asort()!

re

it means a lack of test for sorting for menus (probably needs follow-up)

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

Added the follow up #3221493: Add test coverage for menu order in node type form

I think this looks good now!

Committed 7bc0a6d and pushed to 9.3.x. Thanks!