Problem/Motivation
Change on #3053689 allows using a button on a menu link via route:<button>
Issue #3153394 updates helper text explaining how to add it.
After creating a link, if we try to update the link, we get an error that it can't be saved due to the link field having the value of <button>
. The primary motivation is to make this process easier.
Steps to reproduce
Create a link with a link value of route:<button>
Go back to the link, edit the field. The value is now <button>
If you update the label and resave the link, you get an error that <button>
is not a valid link.
Proposed resolution
Allow <button>
to be a valid link. It's an exception the same way we add <nolink>
.
Remaining tasks
Update helper text.
User interface changes
Issue #3153394 updates helper text explaining how to add route:<button
. As part of this, it gets updated to use <button>
Comment | File | Size | Author |
---|---|---|---|
#27 | 3202166-25.patch | 4.22 KB | xjm |
#27 | 3202166-25-FAIL.patch | 1.15 KB | xjm |
#25 | interdiff-25.txt | 733 bytes | xjm |
#25 | 3202166-25.patch | 4.22 KB | xjm |
#25 | 3202166-25-FAIL.patch | 4.22 KB | xjm |
Issue fork drupal-3202166
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
rubenvarela CreditAttribution: rubenvarela commentedComment #3
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commented3202166-1.patch has been queued for custom testing for 9.2.x
Comment #4
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedPatch #1, Works fine for me, Adding an after-patch screenshot for reference.
RTBC+1
Comment #5
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedComment #6
catchThis could use some test coverage.
Comment #8
vakulrai CreditAttribution: vakulrai as a volunteer and at QED42 for QED42 commentedAdded test for route: as link.
Comment #9
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #8 and it works fine.
Before patch:
After patch;
RTBC +1
Comment #10
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedComment #11
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedVerified and tested patch #8.
Patch applied successfully and looks good to me.
Testing Steps:
# Goto: admin/structure/menu/item/1/edit?destination=/admin/structure/menu
# Add into link field
# Save it.
Expected Results:
# User should be able to save the created menu with link
# User should see "Menu linked has been saved" message once clicking on the Save button.
Actual Results:
# User is not able to save the created menu with link and able to see an error.
Looks good to me.
Can be a move to RTBC.
Comment #13
larowlanThis now has tests
Comment #14
larowlanThere are two places in LinkWidget that reference
route:<button>
but we're only updating one of them in this patch.Comment #15
Neslee Canil PintoUpdate as per #14
Comment #16
Madhu kumar CreditAttribution: Madhu kumar as a volunteer and at Zyxware Technologies commentedApplied patch #8 cleanly and working as expected, able to add button in the menu section.
Screenshot for the reference.
Comment #17
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Zyxware Technologies commentedVerified and tested patch#15 on the drupal 9.3.x-dev version. Patch applied successfully and looks good to me.adding screenshot for the reference
Comment #18
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Zyxware Technologies commentedComment #19
paulocs$this->drupalPostForm()
is deprecated.Replace
$this->drupalPostForm('/entity_test/add', $edit, 'Save');
to
Comment #20
paulocsComment #21
paulocsAttaching a new patch.
Comment #22
kleiton_rodrigues CreditAttribution: kleiton_rodrigues at CI&T commentedComment #23
kleiton_rodrigues CreditAttribution: kleiton_rodrigues at CI&T commentedThe patch applies cleanly and works as expected.
Comment #24
paulocsComment #25
xjmJust uploading a test-only patch to expose the test coverage. Also added a missing article to the code comment. Leaving RTBC since the change is trivial and I would have fixed it on commit if there had already been a test-only patch tested.
Comment #26
xjmThanks everyone for fixing and manually testing this change.
I'm removing issue credit for @kleiton_rodrigues, @Rinku Jacob 13, @Madhu kumar, and @chetanbharambe, since your colleagues already manually tested the patch and provided screenshots without any intervening changes to the functionality that was being screenshotted. In the future, it's more helpful if you manually test patches that have not already been tested by someone else (especially someone from the same organization). I did retain credit for @Abhijith S because the "after" illustration added additional information not present in @Gauravmahlawat's screenshots that helped me understand the change. Thanks for your efforts!
Comment #27
xjmIt'd help if my test-only patch were not just the same as the actual patch.
Comment #31
xjmOK, that fails as expected. (Although the debug is pretty terrible, "Undefined array key 1". But that's not this test's fault.)
Committed to 9.3.x, and cherry-picked to 9.2.x as a non-disruptive bugfix. Thanks!