Problem/Motivation

I have ~190 menu config entities and menu_ui_form_node_form_alter() calls menu_ui_form_node_form_alter() even though it actually has a fixed list of ID's and only needs those.

We also don't need them to be sorted as the order isn't actually used (maybe it should be, but it isn't).

Proposed resolution

Just do a Menu::loadMultiple($type_menus) and loop over that and use $menu->id()/$menu_label() as key/value.

$type_menus can also be empty, so we can probably already do a early return there?

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 core-2998802-11.patch1.13 KBQuentin Massez
#6 core-2998802-6.patch1.09 KBQuentin Massez
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

msankhala’s picture

msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commented

I have ~190 menu config entities and menu_ui_form_node_form_alter() calls menu_ui_form_node_form_alter() even though it actually has a fixed list of ID's and only needs those.

Do you mean menu_ui_form_node_form_alter() calls menu_ui_get_menus() ?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Yes, that's what I mean.

Quentin Massez’s picture

Quentin Massez CreditAttribution: Quentin Massez at Insite commented

I'm working on it as part of Drupal Europe sprint

GaëlG’s picture

GaëlG
he/him
Primary language French
Location Lille, France
CreditAttribution: GaëlG at Insite commented

And I'm mentoring Quentin :)

Quentin Massez’s picture

Quentin Massez CreditAttribution: Quentin Massez at Insite commented
Status: Active » Needs review
FileSize
core-2998802-6.patch1.09 KB
GaëlG’s picture

GaëlG
he/him
Primary language French
Location Lille, France
CreditAttribution: GaëlG at Insite commented
Issue tags: +DrupalEurope
Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented

I'm at DrupalEurope and going to review this patch.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/menu_ui/menu_ui.module
@@ -222,10 +222,15 @@ function menu_ui_form_node_form_alter(&$form, FormStateInterface $form_state) {
   $menu_parent_selector = \Drupal::service('menu.parent_form_selector');
   $menu_names = menu_ui_get_menus();
-  $type_menus = $node_type->getThirdPartySetting('menu_ui', 'available_menus', ['main']);

    you no longer use $menu_names now, but the call is still there. 

  2. +++ b/core/modules/menu_ui/menu_ui.module
@@ -222,10 +222,15 @@ function menu_ui_form_node_form_alter(&$form, FormStateInterface $form_state) {
+  /** @var \Drupal\system\Entity\Menu[] $type_menus */
+  $type_menus = Menu::loadMultiple($type_menus_ids);

    If we do add a @var then it should probably be on MenuInterface.

    Not sure what the guidelines in core about this is now whether it should be here at all or not.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented
Status: Needs work » Needs review

... stepping down.

Quentin Massez’s picture

Quentin Massez CreditAttribution: Quentin Massez as a volunteer and at Insite commented
Status: Needs review » Needs work
FileSize
core-2998802-11.patch1.13 KB

Here's the new patch.

I did the @var on MenuInterface.
I kept it because there are others in the file and I didn't find any documentation about it.
I found a documentation but only for classes
https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

Quentin Massez’s picture

Quentin Massez CreditAttribution: Quentin Massez as a volunteer and at Insite commented
Status: Needs work » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

  • catch committed 3346d45 on 8.7.x 
    Issue #2998802 by Quentin Massez, Berdir: menu_ui_form_node_form_alter...

  • catch committed 14f93ef on 8.6.x 
    Issue #2998802 by Quentin Massez, Berdir: menu_ui_form_node_form_alter...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

After this, menu_ui_get_menus() is only used twice in core. We could deprecate it, and also menu_list_system_menus() which is only called by that function.

https://api.drupal.org/api/drupal/core%21modules%21menu_ui%21menu_ui.mod...

Opened a follow-up #3000242: Deprecate menu_ui_get_menus() and menu_list_system_menus().

Status: Fixed » Closed (fixed)

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