Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- 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...
- 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
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#9 | interdiff.txt | 735 bytes | Gábor Hojtsy |
#9 | 2316459-menu-ui-parent-9.patch | 3.3 KB | Gábor Hojtsy |
#5 | interdiff.txt | 431 bytes | Gábor Hojtsy |
#5 | 2316459-menu-ui-parent-5.patch | 3.32 KB | Gábor Hojtsy |
#2 | 2316459-menu-ui-parent-test-only-WILL-FAIL.patch | 1.26 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyAnd the patch with a complete solution and test. Needs review.
Comment #2
Gábor HojtsyHere 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.
Comment #5
Gábor HojtsyLast minute changes after verifying tests are not a win. Let's set the right variable empty when it would not be valid.
Comment #6
pwolanin CreditAttribution: pwolanin commentedLooks reasonable, though I'm not sure that we want to use t() in the test code?
Comment #7
pwolanin CreditAttribution: pwolanin commentedchecked with people about t() and seems it should be used if the original text is also going through t()
Comment #8
catchWhy's the array_values necessary? I don't see a need to re-key.
Comment #9
Gábor HojtsyComment #10
Gábor HojtsyComment #11
catchCommitted/pushed to 8.x, thanks!
Comment #13
Gábor HojtsyProvided the fix yesterday from Drupalaton but forgot to tag.