Problem/Motivation

  1. Menu UI module refers to the menu admin library but not with the right name. This results in the UI not updating the available parent menu items when restricting menu placement for a content type (and displaying all menu items on the site at all times). Of course the form works, but...
  2. You can save an allowed menu item that is not in the allowed menus no problem. Uh! Of course when the JS works well, you cannot get to this point normally, but without the JS as is before the patch you can.

Proposed resolution

  1. Fix the reference to the library. This is the easy one, because it is a menu => menu_ui problem twice on the same code line. One line patch, done.
  2. Add validation to the menu item selection. If an item was selected that is not in the menus selected, throw an error. Add test coverage.

Remaining tasks

Review.

User interface changes

Menu item selection on node types will be sane again. If for some reason you have no JS, you cannot select an invalid combination of menus / item anymore.

API changes

None.

CommentFileSizeAuthor
#9 interdiff.txt735 bytesgábor hojtsy
#9 2316459-menu-ui-parent-9.patch3.3 KBgábor hojtsy
#5 interdiff.txt431 bytesgábor hojtsy
#5 2316459-menu-ui-parent-5.patch3.32 KBgábor hojtsy
#2 2316459-menu-ui-parent-test-only-WILL-FAIL.patch1.26 KBgábor hojtsy
#1 2316459-menu-ui-parent.patch3.32 KBgábor hojtsy
MenuItemSelector.png242.68 KBgábor hojtsy

Comments

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new 2316459-menu-ui-parent.patch3.32 KB

And the patch with a complete solution and test. Needs review.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new 2316459-menu-ui-parent-test-only-WILL-FAIL.patch1.26 KB

Here is a test only patch that WILL FAIL. The test is almost identical to the one added in #1, I just removed the assertion for the error message added in the patch, since that would of course fail. Now this proves that there is in fact a success message for saving the form with an invalid combination of menus/items.

The last submitted patch, 1: 2316459-menu-ui-parent.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2316459-menu-ui-parent-test-only-WILL-FAIL.patch, failed testing.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review
StatusFileSize
new 2316459-menu-ui-parent-5.patch3.32 KB
new interdiff.txt431 bytes

Last minute changes after verifying tests are not a win. Let's set the right variable empty when it would not be valid.

pwolanin’s picture

pwolanin commented

Looks reasonable, though I'm not sure that we want to use t() in the test code?

pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

checked with people about t() and seems it should be used if the original text is also going through t()

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/menu_ui/menu_ui.module
@@ -493,6 +494,26 @@ function menu_ui_form_node_type_form_alter(&$form, FormStateInterface $form_stat
+  $available_menus = array_values(array_filter($form_state['values']['menu_options']));

Why's the array_values necessary? I don't see a need to re-key.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new 2316459-menu-ui-parent-9.patch3.3 KB
new interdiff.txt735 bytes
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • catch committed 0f3327f on 8.0.x 
    Issue #2316459 by Gábor Hojtsy: Fixed Menu ui module refers to...
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +Drupalaton 2014

Provided the fix yesterday from Drupalaton but forgot to tag.

Status: Fixed » Closed (fixed)

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