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.
Comment | File | Size | Author |
---|---|---|---|
#47 | 2738547-40-d95.patch | 2.95 KB | lauriii |
#41 | yak-shaving-day-2546142921.jpeg | 33.41 KB | larowlan |
#40 | 2738547-40.patch | 2.1 KB | smustgrave |
#40 | interdiff-37-40.txt | 1.35 KB | smustgrave |
Issue fork drupal-2738547
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
Wim LeersComment #12
larowlanWe have this code in initContextual in contextual.es6.js
It should also consider window.location.search
Comment #16
mehul.gada@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.
Comment #17
mehul.gadaComment #18
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi, 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:
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.
Comment #19
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedchanging the status to RTBC and its working fine on 9.4.x
Comment #20
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedMaking it now to Review as it can be checked for other versions
Comment #21
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedTested 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
Comment #22
larowlanCan we add tests for this?
Comment #23
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented#22 Could you please more eloborate what tests we need to add
Comment #24
larowlanIt 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=barComment #25
larowlanComment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded a test + reroll for 10.1
Comment #29
larowlanNice, the test shows the existing bug, but also that the fix doesn't work 💪
Comment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedI can never figure those out with the subdirectory. I wonder if my change broke something
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedInstead of using user path using blocks path.
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #33
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded a replace to get around the subdirectory issue.
Comment #34
DeepaliJ CreditAttribution: DeepaliJ at Salsa Digital commented- 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
Comment #35
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedApplied 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
Comment #36
larowlanThis 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.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedLike that?
Comment #38
larowlanYes, that looks good to me
Comment #39
xjmThanks for working on this
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
touser/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!
Comment #40
smustgrave CreditAttribution: smustgrave at Mobomo commentedAttempted to address #39
Comment #41
larowlanI considered nit-picking that we could use the second argument to
parse_url
in the test, but realised it was the definition of yak shavingComment #43
smustgrave CreditAttribution: smustgrave at Mobomo commentedRandom failure
Comment #44
xjmRequeued the test job (we should always do that when something hits a known random, not just set it back from NW).
Comment #47
lauriiiCommitted 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.
Comment #48
Wim LeersI remember this issue … 👴 Great to see it fixed!
Comment #50
lauriiiCommitted 5ca9f5d and pushed to 9.5.x. Thanks!
Comment #52
Kristen PolReviewing 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.
Comment #53
lauriiiI 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.
Comment #54
Kristen PolThank 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.