Editing menus user-experience has regressed

As discussed, I suggested to simply explicitly set query to ['destination' => $current_path] and not bother with the service as we I think explicitly only want the fallback behavior of the redirect destination service.

1. Starting by confirming undesirable behavior. Confirmed. Basically, I went to /admin/structure/menu/manage/main?destination=/admin/structure/menu and then to /admin/structure/menu/manage/main/add?destination=/admin/structure/menu, and after adding my link, it returned me to /admin/structure/menu. Also agreed this is jarring.
2. Applying patch: $ curl -l https://www.drupal.org/files/issues/2018-04-19/2957953-9.patch | git apply -v
3. Applied clean.
4. drush cr
5. Redoing my test from step 1: mmm nope, same behavior. Either I'm testing in the wrong place, or this patch isn't working. Or, maybe there's some caching here that I'm not properly clearing.
6. Taking a look at this statement from the description "The destination is removed from the links on admin/structure/menu": ok, this is even weirder behavior. Testing this scenario (adding a link using the dropdown) leaves me on the menu item afterwards (/admin/structure/menu/item/3/edit).

So.. I don't think this is fixed, and I think I also uncovered a similar undesirable behavior, in a different location, in my step 1.

Not sure I'm testing this right, so leaving issue status as-is.

Yes, what @runeasgar said :)

Also, the test-only patch did not actually fail.

The reason is that you picked an empty menu in your test and your test doesn't have the local actions (not tasks) block, the only "add link" is the empty table message. And that empty table message already does exactly what I suggested we should do also in the local actions plugin: It *specifically* links back the destination to the menu edit form.

And because that's not enough fun, we also have cacheablity metadata bugs in \Drupal\menu_ui\Plugin\Menu\LocalAction\MenuLinkAdd, because without clearing the render cache right above the test, we see a render cached local actions block that does have the "correct" destination because it's from a previous request.

So this actually does fail, but it's pretty fragile, but it's just a test-only patch. Working proper fixes...

This test does not fail on its own due to the caching issue, but I don't think we should keep a cache clear to work around a bug that no longer exists with this implementation. I think it does sufficiently prove that things are working?

Without the now unnecessary use.

Hm. Missed that part.

If you go to admin/content?destination=/front then editing a node and saving will redirect you back to /front.

Maybe instead of dropping the query argument, we should prevent it from being added to the edit links for menus then? This page after all is very simlar to taxonomy term pages, the main difference is that the term overview there is not attached to the edit form but a separate page, so it doesn't get the default destination argument.

I still think it makes sense to keep the other improvements to have consistency between the the two add links.

I also noticed there is a third add link on the menu operations, without any destination, which means you end up on the edit form after saving, which honestly doesn't really make sense to me. But not sure where it should go instead and I think we're doing enough already here.

Would love to get this moving along again?

Sure! Although ... I kinda went specific with the tests, let me know what you think?

Okay ... that was unexpected ... failed due to the base path? I'll revisit this later.

Although ... might just be a quick fix... Removing the toString on the failing line, it shouldn't have made any difference ~ but there's an existing test below doing the same thing without it that I didn't spot previously.

The fail in #25 is probably because the testbot runs from a subdirectory. In any case, the test in #29 is not working.

It also passes without the code to fix it, so the test looks like it is not specific enough.
It looks like the patch is correct though.

Ugh, meant change the status in my previous comment.

Duplicated by https://www.drupal.org/project/drupal/issues/3019834 (which is now merged with the same fix).

Changed status back to open, the "duplicate" tag is incorrect. The referenced issue 3019834 was published in v8.7, but does NOT contain this fix, the bug still exists in v8.7.6

@hudri This hasn't regressed, I've just quickly checked it now. Please can you describe in detail the steps you're taking?

E.g.

1. Started on /admin/structure/menu
2. Clicked to edit the "Management" menu, URL contains a destination; /admin/structure/menu/manage/admin?destination=/admin/structure/menu
3. Clicked to edit a menu item, noting the URL now has no destination; /admin/structure/menu/link/entity.context.collection/edit (the lack of a destination is the "fix")
4. Saved the menu item, redirected back to the "Management" menu as expected

https://www.drupal.org/files/issues/2018-12-23/entity-url-methods-301983...

diff --git a/core/modules/menu_ui/src/MenuListBuilder.php b/core/modules/menu_ui/src/MenuListBuilder.php
index b694c8b937..c6efe07222 100644
--- a/core/modules/menu_ui/src/MenuListBuilder.php
+++ b/core/modules/menu_ui/src/MenuListBuilder.php
@@ -48,7 +48,7 @@ public function getDefaultOperations(EntityInterface $entity) {
       $operations['add'] = [
         'title' => t('Add link'),
         'weight' => 20,
-        'url' => $entity->urlInfo('add-link-form'),
+        'url' => $entity->toUrl('add-link-form'),
       ];
     }

Maybe for your circumstances, something else is causing it? Else, maybe I've just forgotten exactly what this covered!

Hmmmm you know what?... You're right, it's just that it's the "Add link" which is broken and not the edit functionality. I swear this was working previously and oddly if you change the test to actually click the link rather than go directly to the page (i.e. so there's no "destination") the tests actually still pass...

/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php

  /**
   * Tests menu functionality.
   */
  public function doMenuTests() {
    $menu_name = $this->menu->id();

    // Test the 'Add link' local action.
    $this->drupalGet('admin/structure/menu');
    $this->xpath('//ul[@class="dropbutton"]/li/a[contains(@href,"' . $this->menu->id() . '")]')[0]->click();
    $destination = 'admin/structure/menu';
    $this->assertUrl(Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name], ['query' => ['destination' => $destination]]));

    $this->clickLink(t('Add link'));
    // Ensure query destination is set.
    $destination = Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name])->toString();
    $this->assertUrl(Url::fromRoute('entity.menu.add_link_form', ['menu' => $menu_name], ['query' => ['destination' => $destination]]));

Not entirely sure I understand, as /core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php suggest that carrying over the "destination" is intentional?

@Pobster, I noticed there is an additional, hidden (caching?) quirks with the destination parameter: The backlink / destination parameter is correct on the very first visit after creating a new menu, but is incorrect after a cache clear:

1. Start on /admin/structure/menu
2. Click on "Add menu", create and save a new menu called "foo"
3. You'll be redirected to /admin/structure/menu/manage/foo (without any destination parameter). Now every link regaring menu items in there (the local action "Add" button, the inline "Add" link if the menu is empty, and the "edit" button on existing items) is correctly using the destination parameter ?destination=/admin/structure/menu/manage/foo
4. Clear all caches, e.g. do a drush cr
5. Visit /admin/structure/menu and click the edit button of the "foo" menu
6. The URL shows /admin/structure/menu/manage/foo?destination=/admin/structure/menu. Now the destination parameter is no longer correct on the "Add" and "Edit" menu item buttons, instead of the expected ?destination=/admin/structure/menu/manage/foo there now is an incorrect ?destination=/admin/structure/menu.
7. Note though that the "Add" link (the inline link inside the drag table, only visible if the menu is empty) always has the correct ?destination=/admin/structure/menu/manage/foo, even after a cache clear

Just making sure I am on the same page; just tried this with 8.7.9 and the issue remains. Destination set when editing a menu item is: admin/structure/menu - which is wrong.

Looks all list builders affected by caching of destination links...

Alright, so after applying the patch to 8.8.1 (cleanly), it appears the functionality is still off. Below are my test cases:

Creating a menu link from the Add link dropdown
1. From /admin/structure/menu, I click the Main Menu dropdown and click Add Link.
2. I am directed to the menu entity form, populate the fields, and submit the form.
3. The form saves and I am redirected to the (same) newly created menu entity form.

Actual result: After creating a menu link entity, I'm on the menu link entity form.
Expected result: After creating a menu link entity, I am redirected back to the Main Menu link listing.

Editing an existing menu link
1. From /admin/structure/menu, I click the edit link for the Main Menu and then click edit on an existing menu link.
2. I edit the existing menu entity and submit the form.
3. I am redirected back to /admin/structure/menu.

Actual result: After editing a menu link entity, I'm redirected back to the menu listing.
Expected result: After editing a menu link entity, I am redirected back to the Main Menu link listing.

Notes
I can see that the ?destination=/en/admin/structure/menu destination parameter persists when clicking the edit link for a menu from /admin/structure/menu. That explains why editing menu links sends you back to the menu listing. For menus, I don't really see a use-case for being redirected back to the list of menus in any scenario. In my mind, it seems extremely rare to finish editing a menu and then want to edit other menus.

Anyway, just wanted to add that testing feedback!

@ryan.gibson: Creating a menu link from the Add link dropdown and Editing an existing menu link now works

Five additional destination tests: Add Link and Edit Menu on /admin/structure/menu and Add Link, Edit and Delete on menu link list.

Verified and tested by applying patch#55.It looks good to me.Can be moved to RTBC.

Steps to test -
1. Go to the admin site.
2. Go to admin/structure/menu
3. Click on any edit menu
4. Click on add link
5. Fill out the add link form and press save
6. Verify , it should re-directs to the link listing page instead of menu listing page.

Before Patch -

After Patch -

@alexpott I tested the patch #55.I have already posted my comment.

@alexpott #57 works as expected. My opinion is that the destination should be set to the menu page, because of two reasons:

• I would like to see the result of the operation, especially because in most cases you have to controll the position in the menu (except you have all weight values of all other menu items in mind)
• More often you add many items to one menu, not one item after another to many menus

The patch changes this. Please review.

#62 works great for me

#62 needs work. This is now unused and can be removed (assuming the changes are good);

+  /**
+   * {@inheritdoc}
+   */
+  protected function ensureDestination(Url $url) {
+    // We don't want to add the destination URL here, as it means we get
+    // redirected back to the list-builder after adding/deleting menu links from
+    // a menu.
+    if ($url->getRouteName() === 'entity.menu.edit_form') {
+      return $url;
+    }
+    return parent::ensureDestination($url);
+  }

removed

function ensureDestination(Url $url)

, please review

Nobody wants to push this problem? Does nobody use the menu UI and is annoyed by the extra clicks on every menu entry?

#65 implements the solution and tests from @alexpott , but sets the destination to the menu page /admin/structure/menu/manage/{menuid} to ensure that you can control your results in the hierarchy and the menu order. Most users will add many items to one menu, not one item after another to many menus.

I agree with #57 it definitely makes sense to me to be taken to the place where you're managing the menu's links after adding a new one.

The interdiff between #57 and #65 looks good to me and confirmed that #65 only removed the ensureDestination function.

This is good for RTBC in my opinion! I believe @pameeela is doing some manual testing so will wait for that before changing status :)

Adding bugsmash tag.

So I went and did some manual testing in order to update the IS as currently the User interface changes section is incorrect.

There is still one way in which this patch doesn't fix the issue.

Steps to reproduce:
1) Navigate to admin/structure/menu
2) Click Edit menu on one of the menus
3) Notice you now have a destination parameter to /admin/structure/menu
4) The Add Link button now has the destination parameter too
5) This is now cached so navigating directly to /admin/structure/menu/manage/FOO will also have the Add link button with the incorrect destination
6) Clearing cache and visiting /admin/structure/menu/manage/FOO directly will fix it.

I'll look at adding a failing test case for this.

Also needs work for comment #40

+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
@@ -281,21 +296,46 @@ public function deleteCustomMenu() {
+    // Test the 'Add link' local action.

So I was confused how the asserts following this were passing with my comments above but I may have found out why.

When we click Add link here there are no other menu links in the page so it's highly likely it's clicking the "Add link" link inside the empty text in the table. From my manual testing, the link inside the empty text has the correct destination however I cannot reproduce this in a test run. BUT I can reproduce it via simplytestme. I'm wondering now if there's something weird with caching on drupalci because I've had similar issues getting other tests to fail caching bugs e.g https://www.drupal.org/project/workbench_access/issues/3172841#comment-1...

Should we not move this issue to the 9.1.x branch?

I think be redirected to the /admin/structure/menu after add a new link is really a problem.
I try to add the same approach from #65 to the 9.1.x branch but I'm still been redirected to the /admin/structure/menu page.
I didn't create a patch to the 9.1.x branch because I would like to know if we have to work on this issue on the 8.9.x or 9.1.x branch.

Patch #12 from https://www.drupal.org/project/drupal/issues/2975407 does solve all issues for us.

Patches #57 and #65 here do not solve the edit-links, as stated above.

maosmurf thanks your right.
This patch worked for me Drupal 9.2.0-dev

@batkor, can you please upload the patch from that issue? I have closed #2305353: Make redirect after saving a menu link into another parent menu, go to the parent menu admin form as a duplicate to avoid any further duplication of effort.

I am also transferring credit from that issue for when this gets fixed.

The patch from issue #2975407: After adding, deleting or editing a menu item, I am redirected to the Menus Management page rather than returned to the Menu Edit page

Last worked patch for this issues
And i think this my patch not best practical.
+1 vote to use patch #79

We need now write tests for patch #79.

add more test coverage of deleting and editing a menu-link via the UI so we can be sure that everything behaves as we'd like.

I'm not sure how write tests for patch #79. If someone has any tip, please tell me.

There's test in #2975407-12: After adding, deleting or editing a menu item, I am redirected to the Menus Management page rather than returned to the Menu Edit page

The tests in #2975407: After adding, deleting or editing a menu item, I am redirected to the Menus Management page rather than returned to the Menu Edit page don't cover what we want. The test is already in patch #79 but it only ensures that it will not be redirected to '/admin/structure/menu'.
I think we need to write test to ensure that the page is redirected to the correct page.

Adding test cases for the patch mentioned in #79

Tests passed locally. Triggering test again.

+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
@@ -100,7 +104,7 @@ protected function setUp(): void {
-  public function testMenu() {
+  public function testMenuMain() {

nit - why not testMainMenu()?
this test class already has testSystemMenuRename()

Worked on comment #94

+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
@@ -193,6 +197,16 @@ public function addCustomMenuCRUD() {
+    $this->drupalPostForm(NULL, [], t('Delete'));

@@ -281,27 +296,66 @@ public function deleteCustomMenu() {
+    $this->clickLink(t('Add link'));
+    $this->drupalPostForm(NULL, ['link[0][uri]' => '/', 'title[0][value]' => $this->randomString()], t('Save'));

IMO none of these strings should be wrapped in t(), I'm not sure if this is a standard or not but even the test changes in this patch have a mix of both.

@acbramley

+++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
@@ -281,27 +296,66 @@ public function deleteCustomMenu() {
+    // Test the 'Add link' local action.
     $this->clickLink(t('Add link'));
     $link_title = $this->randomString();
     $this->drupalPostForm(NULL, ['link[0][uri]' => '/', 'title[0][value]' => $link_title], t('Save'));
     $this->assertSession()->addressEquals(Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name]));
+

This is good as we have local test separately.

Spoke with @Sam152 about this and he agreed that patches shouldn't be adding new calls to t() in tests. There were also 2 calls to the deprecated drupalPostForm so I've fixed those up too.

@acbramley is right. We should not use t() in tests because we are not testing the translation.
I'm moving to needs work because patch #102 still uses drupalPostForm.
See:

+$this->drupalPostForm(NULL, ['link[0][uri]' => '/', 'title[0][value]' => $link_title], 'Save');
and
+ $this->drupalPostForm(NULL, ['link[0][uri]' => '/', 'title[0][value]' => $link_title], 'Save');

Worked on Comment #103

@kishor_kolekar you should remove NULL, from $this->submitForm(NULL, ['link[0][uri]' => '/', 'title[0][value]' => $link_title], 'Save'); in both cases.

I tested patch #107 and it looks good to me.
The test looks good too. Moving to RTBC.

Patch for drupal 9.1.x

Applied patch #113 on 9.2.x and it works fine.Added screen recordings.

Tested on simplytest.me following steps to reproduce 1-5 in the issue description. Works as expected: You end up back at the menu with its list of links.

Also applied patch with composer on existing site with core 9.1.3. Same results.

Thanks! This fixes something pretty annoying.

Hello,

Tested on 9.1.4, working great, thanks for the patch.

Added Merge Request to update patch in #113 for 9.2-dev
https://git.drupalcode.org/project/drupal/-/merge_requests/592

Diff here:https://git.drupalcode.org/project/drupal/-/merge_requests/592/diffs
Patch here: https://git.drupalcode.org/project/drupal/-/merge_requests/592/diffs.patch

Tugboat QA site listed on merge request.

The patch 2957953-9.2.x-113.patch no longer applicable on 9.1.8

Updating the patch!

Verified and tested patch #121.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: admin/structure/menu
# Click on any edit menu
# Click on add link
# Fill out the add link form and press save
# User should be redirected to back at the menu with its list of links but actually, the user is redirected to back at the list of menus.

Expected Results:
# User should be redirected to back at the menu with its list of links.

Actual Results:
# User is redirected to back at the list of menus.

Looks good to me.
Can be a move to RTBC.

The patch in #121 didn't seem to apply so here is another reroll for 9.1.x.

Bug fixes go against the latest version, so this needs a working patch against 9.3.x.

I read the Issue Summary and it is out of date. The remaining steps show that a test is needed but there is one in the patch. As a reviewer, that makes me wonder if the test needs more work?

I had a brief look at the patch and I want to recommend changes to a comment but I have to go now.

Given the last summary update was performed in #2957953-44: Editing menus user-experience has regressed, I'm updating the issue summary with where we're at.

Removing outdated issue tags to reflect the issue summary update and to remove an out-of-date conference tag.

As a friendly reminder, here's where our remaining tasks sit as of this comment (as also reflected in the summary):

Remaining tasks

• This issue needs a working patch against `9.3.x` (as requested in #2957953-124: Editing menus user-experience has regressed).
• Any additional patch (test comments) feedback (as mentioned in #2957953-124: Editing menus user-experience has regressed).

Let's squash this bug!

Here i have re-rolled the patch against 9.3.x

Fixing the test cases and removing deprecated method.
The patch was not applicable on the latest 9.3.x head, I had to re-roll it.

Re-roll of last working 9.2.x patch (from #118 / #119) that should apply to latest 9.2.x branch / 9.2.x release version.

Sorry, needs a reroll.

Re-rolled the patch in #130 against the latest 9.3.x, thanks!

Re-rolled the patch in #131 against the latest 9.2.x.

applied patch #121 working fine
after patch the destination will be menu list for ref sharing

Patch #133 looks good!

It properly redirects when deleting a menu link and when editing a menu link.
The code looks clean as well and the tests seems to cover the bug.

Cheers, Paulo.

+

1. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -195,6 +199,16 @@ public function addCustomMenuCRUD() {
   +    // Click the "Add link" operation in the Tools row.
   

   This is the delete link, not the add link.

   Whilst we're here, can we add a comment saying that we're doing this via the UI so that we can test destination handling, otherwise someone might think this duplicates the existing delete test case in ::deleteCustomMenu

2. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -279,11 +294,35 @@ public function deleteCustomMenu() {
   +    $link_title = $this->randomString();
   +    $this->submitForm(['link[0][uri]' => '/', 'title[0][value]' => $link_title], 'Save');
   ...
   +    $link_title = $this->randomString();
   +    $this->submitForm(['link[0][uri]' => '/', 'title[0][value]' => $link_title], 'Save');
   

   Can we also assert that the new link exists was indeed crated, e.g the link title appears on the page.

3. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -300,6 +339,20 @@ public function doMenuTests() {
   +    // Clear the cache to ensure that recent caches aren't preventing us from
   +    // seeing a broken add link.
   +    $this->resetAll();
   

   I see talk above about their being a caching bug in the local actions block - is that what this is for?

   If so, is there an existing issue for that?

   If not, can we create one?

   If so, can we postpone this issue on fixing that first?

4. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -300,6 +339,20 @@ public function doMenuTests() {
   +    $this->clickLink('Add link');
   +    $this->submitForm(['link[0][uri]' => '/', 'title[0][value]' => $this->randomString()], 'Save');
   

   Same here re testing that the new link was indeed created/exists on the page

Patch #139 looks good!

It properly redirects when deleting a menu link and when editing a menu link.
The code looks clean as well and the tests seems to cover the bug.

Saving issue credit

Committed 8a18762 and pushed to 9.3.x. Thanks!

As this is fixing a major bug and there is little risk of regression, backported to 9.2.x

Thanks everyone for the mammoth effort here