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
At the moment of add a menu item from the edit article page, the save process doesn't save the programmed menu, even the name title, description, parent item and weight. saving is not working, i have to do it from the structure button from the edit menu.
#5:
Menu ui binds its submit handler to the 'Save' button:
$form['actions']['submit']['#submit'][] = 'menu_ui_form_node_form_submit';
However, the 'save' button is disabled for users with 'administer nodes' permission in favor of 'Save and publish' / 'Save and keep published' etc. This situation is not tested in MenuNodeTest so that's why all tests are passing.
Proposed resolution
Remaining tasks
User interface changes
API changes
Original report by @Frankencio
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2423153-16.patch | 3.19 KB | idebr |
| #16 | interdiff-16-12.txt | 2.34 KB | idebr |
| #16 | 2423153-16.fail_.patch | 2.41 KB | idebr |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
jibranWe can't ship with this bug so it is a critical.
Comment #3
webchickHuh. How do we not have tests for this?
Comment #4
tim.plunkettComment #5
idebr CreditAttribution: idebr commentedThis bug was likely introduced in #2406103: Remove hook_node_validate() and hook_node_submit() because they bypass the entity API
Comment #6
dawehnerWow, I'll have a look at it.
Comment #7
idebr CreditAttribution: idebr commentedMenu ui binds its submit handler to the 'Save' button:
However, the 'save' button is disabled for users with 'administer nodes' permission in favor of 'Save and publish' / 'Save and keep published' etc. This situation is not tested in
MenuNodeTestso that's why all tests are passing.Is the proposed resolution to bind the submit handler to all submit buttons or is there a more elegant way?
Comment #8
idebr CreditAttribution: idebr commentedAttached patch binds the submit handler to every submit handler on the node form. I hope there is a more elegant way?
I also updated
MenuNodeTestto indicate the problem.Comment #10
xjmThanks @idebr and @Frankencio!
I think in #8, rather than removing existing test coverage and replacing it with the new coverage, we should be adding additional test coverage.
Comment #11
dawehnerIn the meantime I tried to write proper new test coverage, but was not yet able to make it pass.
Honestly I think its quite a WTF, that you have to do it.
Comment #12
dawehnerAlright, there was a bug in the test :)
Comment #13
catchWe have a major issue from two years ago exactly for this problem #1901216: Modules cannot reliably enhance or alter the node form anymore.
Comment #14
dawehnerComment #15
idebr CreditAttribution: idebr commented@dawehner Thanks for working on the tests. The test-only patch passed the testbot, so I'll have a look if I can make them fail properly.
Comment #16
idebr CreditAttribution: idebr commentedThe tests that were supposed to fail still found the menu link that was added by the 'editor' before. I moved the part with the 'admin_user' in front, so the tests assertions fail correctly.
Comment #17
idebr CreditAttribution: idebr commentedComment #18
dawehnerHa, good catch!
Comment #20
dawehnerI like how this patch looks now
Comment #22
webchickHm, yeah it's definitely unfortunate that we need this sort of logic just because of different button names. I guess #1901216: Modules cannot reliably enhance or alter the node form anymore is the place to talk about fixing that up. The approach looks generic enough though to account for the fact that contrib might add other dropbutton options here, and the test coverage is nice and thorough.
Committed and pushed to 8.0.x. Thanks!