When using contextual links to edit an item from a paged page, the page parameter is not passed in the destination parameter. Steps to reproduce:

- Navigate to a page with a pager (eg a list of news articles)
- Go to page 2 (news?page=1)
- Use contextual links to edit a news item
- The destination parameter is /node/xx/edit?destination=news instead of something like /node/xx/edit?destination=news?amp;page=1
- When saving the node, you end up on the first page, instead of the one you came from.

Issue fork drupal-2738547

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

BarisW created an issue. See original summary.

Wim Leers’s picture

Issue tags: +Needs tests

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
larowlan’s picture

Issue tags: +Novice, +Bug Smash Initiative

We have this code in initContextual in contextual.es6.js

// Set the destination parameter on each of the contextual links.
    const destination = `destination=${Drupal.encodePath(
      Drupal.url(drupalSettings.path.currentPath),
    )}`;

It should also consider window.location.search

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mehul.gada made their first commit to this issue’s fork.

mehul.gada’s picture

@larowlan, Thanks for the hint. I have added this code in contextual.es6.js and opened a merge request. I tested a few cases in the local environment and all of them worked as expected.

mehul.gada’s picture

Status: Active » Needs review
sahil.goyal’s picture

FileSize
129.67 KB
104.53 KB

Hi, Thanks for applying the patch, I have Reproduced the Problem on my local 9.4.x Environment by creating a page with pager on it. As seen on Example-page-SS.png
Steps which i followed to reproduce:

  • Navigate to the 2nd page with a pager (As link changed to (/artcile-view-page?page=1))
  • go to the edit on article, save the node.
  • Then automatically redirected to /artcile-view-page? instead of artcile-view-page?page=1

So, I have Applied the patch, And Now its working As expected, it redirected to the correct page from where i have went to edit the node.
Providing the after apply patch Screenshots. Thanx.

sahil.goyal’s picture

Status: Needs review » Reviewed & tested by the community

changing the status to RTBC and its working fine on 9.4.x

sahil.goyal’s picture

Status: Reviewed & tested by the community » Needs review

Making it now to Review as it can be checked for other versions

ameymudras’s picture

Status: Needs review » Reviewed & tested by the community

Tested on Drupal 9.4

- The issue summary is clear and testing steps have been provided
- Used the MR and it seems to resolve the issue (Not providing screenshots, already provided in #18)
- Code review doesn't show any other issues

Moving to RTBC

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Can we add tests for this?

sahil.goyal’s picture

Issue summary: View changes

#22 Could you please more eloborate what tests we need to add

larowlan’s picture

It would have to be a functional javascript test.

We already have \Drupal\Tests\contextual\FunctionalJavascript\ContextualLinksTest::testContextualLinksDestination

So if we change the drupalGet in that function to pass something like ['options' => ['query' => ['foo' => 'bar']]] as the second argument, we should be able to assert that $contextual_link_url_parsed['query'] includes &foo=bar

larowlan’s picture

Status: Needs review » Needs work
smustgrave’s picture

Added a test + reroll for 10.1

The last submitted patch, 26: 2738547-26-tests-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 26: 2738547-26.patch, failed testing. View results

larowlan’s picture

Nice, the test shows the existing bug, but also that the fix doesn't work 💪

smustgrave’s picture

I can never figure those out with the subdirectory. I wonder if my change broke something

smustgrave’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
1.8 KB

Instead of using user path using blocks path.

smustgrave’s picture

Version: 9.4.x-dev » 10.1.x-dev
smustgrave’s picture

Added a replace to get around the subdirectory issue.

DeepaliJ’s picture

- Tested and verified patch on Drupal 10.1.x-dev
- Patched applied cleanly.
- Able to reproduce the issue and seems working fine after applying the patch.
(Not providing screenshots, already provided in #18)

RTBC+1

gaurav-mathur’s picture

Applied patch #33 on drupal version 10.1.x-dev successfully.
Patched applied clearly working fine and it looks good to me.(screenshot already provided in #18, not providing screenshot)
RTBC+1
Thank you

larowlan’s picture

Status: Needs review » Needs work
+++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -110,11 +110,13 @@ public function testContextualLinksDestination() {
+    // Replace subdirectory to pass d.o test.
+    $expected = str_replace('/subdirectory', '', $contextual_link_url_parsed['query']);
+    $this->assertEquals("destination=$expected_destination_value%3Ffoo%3Dbar", $expected);

This is the wrong fix.
We need to construct a Url object and cast it to string, I think that will allow for the base path.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
2.01 KB

Like that?

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Yes, that looks good to me

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this

+++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinksTest.php
@@ -110,11 +111,11 @@ public function testContextualLinksDestination() {
-    $this->drupalGet('user');
+    $this->drupalGet('admin/structure/block', ['query' => ['foo' => 'bar']]);
     $this->assertSession()->waitForElement('css', '.contextual button');
-    $expected_destination_value = (string) $this->loggedInUser->toUrl()->toString();
+    $expected_destination_value = Url::fromRoute('block.admin_display')->toString();
     $contextual_link_url_parsed = parse_url($this->getSession()->getPage()->findLink('Configure block')->getAttribute('href'));
-    $this->assertEquals("destination=$expected_destination_value", $contextual_link_url_parsed['query']);
+    $this->assertEquals("destination=$expected_destination_value%3Ffoo%3Dbar", $contextual_link_url_parsed['query']);

Interesting. Why is it necessary to change the path from the user route to the block route?

I think it would be better to add to the existing test coverage, rather than changing the existing test coverage. The former test was testing that contextual link parameters were preserved with a redirect (from user to user/1 or whatever). This is testing that they are preserved with other query parameters. We need test coverage from both. In fact, we should have test coverage for each separately, and then both combined.

Thanks!

smustgrave’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
2.1 KB

Attempted to address #39

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
33.41 KB

I considered nit-picking that we could use the second argument to parse_url in the test, but realised it was the definition of yak shaving

a cartoon yak from Ren and Stimpy standing on it's back legs shaving it's face whilst looking in the mirror

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 2738547-40.patch, failed testing. View results

smustgrave’s picture

Status: Needs work » Reviewed & tested by the community

Random failure

xjm’s picture

Requeued the test job (we should always do that when something hits a known random, not just set it back from NW).

  • lauriii committed 4a0272e3 on 10.1.x
    Issue #2738547 by smustgrave, mehul.gada, sahil.goyal, larowlan, BarisW...

  • lauriii committed e72c868f on 10.0.x
    Issue #2738547 by smustgrave, mehul.gada, sahil.goyal, larowlan, BarisW...
lauriii’s picture

Version: 10.1.x-dev » 9.5.x-dev
FileSize
2.95 KB

Committed 4a0272e and pushed to 10.1.x and cherry-picked to 10.0.x. Thanks!

Sending the 9.5.x patch to the test bot.

Wim Leers’s picture

I remember this issue … 👴 Great to see it fixed!

  • lauriii committed 5ca9f5d8 on 9.5.x
    Issue #2738547 by smustgrave, mehul.gada, lauriii, sahil.goyal, larowlan...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5ca9f5d and pushed to 9.5.x. Thanks!

Status: Fixed » Closed (fixed)

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

Kristen Pol’s picture

Reviewing this issue. I'm not sure why DeepaliJ wasn't credited as no one had tested the patch beforehand but this issue is closed so I assume it won't be changed.

lauriii’s picture

I didn't credit @DeepaliJ because at that time, the patch is making one change to the contextual links which already has automated test coverage in the change being proposed. There also had been manual testing for the same change prior to the automated tests being added by @sahil.goyal, all that was changed since then was that automated tests were added.

Kristen Pol’s picture

Thank you for the explanation. I think that people who do QA wouldn’t realize it didn’t need testing. There was a new patch and no one explained it didn’t need testing. I wish we had a better process so people don’t waste time on things that aren’t needed.