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>

CommentFileSizeAuthor
#27 3202166-25.patch4.22 KBxjm
#27 3202166-25-FAIL.patch1.15 KBxjm
#25 interdiff-25.txt733 bytesxjm
#25 3202166-25.patch4.22 KBxjm
#25 3202166-25-FAIL.patch4.22 KBxjm
#23 after_menu_save_3202166-23.jpg178.26 KBkleiton_rodrigues
#23 after_site_3202166-23.jpg311.98 KBkleiton_rodrigues
#23 patch_applied_3202166-23.jpg113.05 KBkleiton_rodrigues
#21 3202166-21.patch4.22 KBpaulocs
#21 interdiff-15-21.txt612 bytespaulocs
#18 afterpatch.mp4691.5 KBRinku Jacob 13
#17 afterpatch.png82.42 KBRinku Jacob 13
#17 beforepatch.png97.22 KBRinku Jacob 13
#16 After-patch-button.png29.12 KBMadhu kumar
#16 Before-patch.png1.12 MBMadhu kumar
#16 Appling-patch.png50.94 KBMadhu kumar
#15 interdiff_8-15.txt1.52 KBNeslee Canil Pinto
#15 3202166-15.patch4.2 KBNeslee Canil Pinto
#11 Edit menu link _ drupal9.2.x.mp43.04 MBchetanbharambe
#11 After Patch 3202166 One.png303.64 KBchetanbharambe
#11 Before Patch 3202166.png333.91 KBchetanbharambe
#9 3202166-after_patch.gif12.01 MBAbhijith S
#9 3202166-before_patch.png109.5 KBAbhijith S
#8 interdiff_3202166-2-8.txt1.13 KBvakulrai
#8 saving_button_link-3202166-8.patch3.08 KBvakulrai
#4 Screenshot 2021-03-08 at 10.38.10.png115.97 KBGauravvvv
#4 Screenshot 2021-03-08 at 10.35.21.png33.7 KBGauravvvv
#2 3202166-1.patch1.94 KBrubenvarela

Issue fork drupal-3202166

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rubenvarela created an issue. See original summary.

rubenvarela’s picture

Gauravvvv’s picture

3202166-1.patch has been queued for custom testing for 9.2.x

Gauravvvv’s picture

Patch #1, Works fine for me, Adding an after-patch screenshot for reference.
RTBC+1

Gauravvvv’s picture

Status: Active » Reviewed & tested by the community
Issue tags: +Usability
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This could use some test coverage.

vakulrai made their first commit to this issue’s fork.

vakulrai’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
1.13 KB

Added test for route: as link.

Abhijith S’s picture

Applied patch #8 and it works fine.

Before patch:
before

After patch;
after

RTBC +1

chetanbharambe’s picture

Assigned: Unassigned » chetanbharambe
chetanbharambe’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +RTBC
FileSize
333.91 KB
303.64 KB
3.04 MB

Verified 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.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

This now has tests

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

There are two places in LinkWidget that reference route:<button> but we're only updating one of them in this patch.

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
4.2 KB
1.52 KB

Update as per #14

Madhu kumar’s picture

Applied patch #8 cleanly and working as expected, able to add button in the menu section.
Screenshot for the reference.

Rinku Jacob 13’s picture

FileSize
97.22 KB
82.42 KB

Verified 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

Rinku Jacob 13’s picture

FileSize
691.5 KB
paulocs’s picture

Status: Needs review » Needs work

$this->drupalPostForm() is deprecated.
Replace
$this->drupalPostForm('/entity_test/add', $edit, 'Save');
to

$this->drupalGet('/entity_test/add');
$this->submitForm($edit, 'Save');
paulocs’s picture

Assigned: chetanbharambe » paulocs
paulocs’s picture

Assigned: paulocs » Unassigned
Status: Needs work » Needs review
FileSize
612 bytes
4.22 KB

Attaching a new patch.

kleiton_rodrigues’s picture

Assigned: Unassigned » kleiton_rodrigues
kleiton_rodrigues’s picture

The patch applies cleanly and works as expected.

3202166

paulocs’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Just 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.

xjm’s picture

Thanks 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!

xjm’s picture

It'd help if my test-only patch were not just the same as the actual patch.

The last submitted patch, 27: 3202166-25-FAIL.patch, failed testing. View results

  • xjm committed 53bc00e on 9.3.x
    Issue #3202166 by xjm, vakulrai, paulocs, Neslee Canil Pinto,...

  • xjm committed 69ba7f7 on 9.2.x
    Issue #3202166 by xjm, vakulrai, paulocs, Neslee Canil Pinto,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

OK, 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!

Status: Fixed » Closed (fixed)

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