[Regression] login link has no destination=drupalSettings.path, so dumps you on the profile

LoginLogoutMenuLink is a class in user module, so setting that as the Component for this issue. Unless someone believes the right fix is somewhere else, in which case set the Component to that.

Yeah, we should definitely fix this. I would call it major, since it's a regression.

Funnily enough, I did not catch this in manual testing since the default front page is so sparse. :P (one of the UMN 2015 issues.) In a quick glance it didn't look much different than the similarly sparse user/1 page. :(

Do folks agree that the destination querystring param should be added via JS in order preserve our current two cache contexts (anon or authenticated)? How can I add an id on this link so that js can find and append a querystring?

If possible we should placeholder it in PHP, currently have no js for anon users and that's worth retaining.

We should use the same approach as for "active" links (adding .is-active to active links): JS-based solution for auth, PHP-based solution for anon.

(The assumption being: pages for anon users are cached in the Page Cache (or a reverse proxy) anyway. So it's fine to do the work in PHP for anon users. Of course, this should be overridable, in the same way that it is overridable for authenticated users.)

I'll take this on

This fully implements that, the only thing missing is test coverage. (Which can be inspired by ActiveLinkResponseFilterTest).

This introduces:

1. the data-current-path-destination attribute, which is used as a signal to give the link that it lives on a destination query argument
2. core/drupal.current-path-destination-link (much like core/drupal.active-link): a small bit of JS to add the destination query argument
3. a PHP implementation of this JS library for anonymous users: CurrentPathDestinationLinkResponseFilter (much like ActiveLinkResponseFilter)
4. an update to system_page_attachments(), to add the JS library for authenticated users, just like it does for active links
5. an updated contextual.js: it now reuses the logic of that JS library
6. finally: the "login" and "logout" menu links now both set this attribute (this is a small usability improvement to the log out link: it'll now keep you on the same page, instead of redirecting you to the front page)

Is it OK to do that for the logout link? It could end up with 403s that way.

Good point about logout.

The approach looks sound to me. I searched for "'destination' => " and just found a couple comment links which are candidates for this treatment. We currently placeholder those so no rush to convert those.

1. Maybe move the library attaching to the menu link(s) instead of system_page_attachments()? It looks weird to conditionally add it in system module, so disconnected from the consumer (a user menu link)
2. FYI, initContextual in contextual.js also has some destination= handling. dont think it is a dupe though.
3. In general, I forget exactly what contextual does at render layer and why its useful to piggy back on it here.
4. stale: "// For authenticated users, the 'is-active' class is set in JavaScript.

#9: good point. Now only doing it for the "Log in" link. Zero behavior change then.

#12:

Indeed, there are more places that could benefit from this, but let's start small :)

1. That's because this is generically useful, and is not at all specific to this menu link. Many things add a "destination" query arg. This could benefit all of those.
2. Indeed it does, and it actually is a dupe. The patch in #7 already removed that, to use this instead. See #7.6.
3. The server-side rendering of contextual links intentionally excludes the destination query argument. Because otherwise, we'd have to re-render it for every page. Because it is omitted, it's possible to cache it on the client side.
4. Fixed.

Fixed the test failures. All contextual link test coverage that needs minor updates.

The UserPasswordResetTest is still failing, there's some kind of endless loop problem there that I still have to figure out :)

Next: adding test coverage. The JS will need manual testing, of course. (And preferably the +1 of @nod_ or @droplet.)

Last fails fixed, test coverage added.

I'm not sure I understand why we need the JS based solution at all?

Also the logic of that check (add js for auth users only) seems flawed. We don't add JS for anonymous users *by default*. Almost any non-trivial site out there will however do exactly that. If anything, then it would have to be a check for whether we are adding JS or not?

Shouldn't a response filter work for anything, e.g. also an ajax response that adds a link with a destination?

Also the logic of that check (add js for auth users only) seems flawed. We don't add JS for anonymous users *by default*. Almost any non-trivial site out there will however do exactly that. If anything, then it would have to be a check for whether we are adding JS or not?

We use the same logic for "active" links and that has worked out well.

Any site that serves JS to anon can easily override this behavior.

Shouldn't a response filter work for anything, e.g. also an ajax response that adds a link with a destination?

To answer the generic question: How could it possibly work for "anything". It can only work for specific types of responses.

To answer the specific question: If an AJAX response is involved, then JS is involved, and it's implemented as a behavior, so all new HTML added via AJAX responses will also get the destination query string.

This feels a little excessive to me, too.

Can't we just use the normal placeholder pattern for the current URL we use elsewhere as the minimum solution?

Having one unified placeholder for the current url has several advantages even for ESI - especially with Varnish 4.1.

---

Even if we decide to use a JS based solution for placeholdering current url links, should we then not just use a placeholder strategy to do so to unify?

e.g. replace the [current url] placeholder with #attached library or in case of anonymous replace with the current url directly.

FWIW, we have an identical need at #2579907: Re-introduce the destination parameter in the Devel menu items. If the login link were the only one, I think standard placeholder would be fine. But I think this is a more common need than one might think. We could end up with a dozen such placeholders.

Can't we just use the normal placeholder pattern for the current URL we use elsewhere as the minimum solution?

Where else do we use this?

Even if we decide to use a JS based solution for placeholdering current url links, should we then not just use a placeholder strategy to do so to unify?

I'm assuming you mean Renderer placeholders here (i.e. #create_placeholder and #attached[placeholders]).

Hrm… so basically use the same approach as \Drupal\Core\Access\RouteProcessorCsrf::processOutbound(). That could work too. Except… how are you going to add those placeholders?

We have a #attached[placeholders-based solution for CSRF tokens because the route declares it needs a CSRF token, and we have outbound route processors, therefore we can do this.

But the destination query string can appear on any URL. Neither outbound route processors not outbound path processors help with that.

But even then we still have a use case for the JS: contextual links. Contextual links already use this pattern. It makes absolutely no sense to use #attached[placeholders] there, because the contextual links are cached across page loads, on the client-side. The only thing they are lacking, is the "destination" query string.

For that, we'll always need a JS solution.

So, I think to arrive at the best solution possible, we need to thoroughly analyze the pros and cons of the two possible solutions.

We could perhaps add a default placeholder via system_page_attachments_alter().

I think all placeholder processing logic already checks if a placeholder exists at least once on the page, so adding it unconditionally should not hurt?

Just would need a generator or something like this.

Yes, we need the JS and class anyway, but the question is can we make the processing in the response subscriber simpler.

the question is can we make the processing in the response subscriber simpler.

If you want to use the Render system's "placeholder" subsystem, then you need to ensure there's a unique string to be found somewhere in the response. i.e. discovery mechanism is string parsing.

Whereas a JS solution cannot use string parsing. It must rely on the DOM.

That's why the server-side part of the JS solution looks like it does: it mimics the JS logic. The reason it is relatively complex: because PHP's DOMDocument sucks/is very slow, and therefore implementing this tiny subset of logic of DOM parsing to find the right place in the DOM makes it much faster. See #1979468-238: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.

I think #25 is worth exploring.

you need to ensure there's a unique string to be found somewhere in the response. i.e. discovery mechanism is string parsing.

Can we do that with a system similar to CacheContext services, but a little different, e.g., CachePlaceholder. But the idea is that via services.yml, you can make an association between an identifier (e.g., 'path.current') and an object that can return a value for that. Then could a CachePlaceholdersManager generate unique placeholders based on the identifier, such as:

function getPlaceholderString($placeholder_id) {
  return Crypt::hmacBase64($placeholder_id, Settings::getHashSalt());
}

Then LoginLogoutMenuLink::getOptions() could do:

$options['query']['destination'] = $cache_placeholder_manager->getPlaceholderString('path.current');

And then at placeholder replacement time (whether that's a function added in system_page_attachments_alter() per #25 or something else) we iterate all CachePlaceholder IDs and replace all occurrences of the corresponding placeholder string with the service's getValue() method.

Or if there's a way to optimize it so that prior to writing to dynamic_page_cache we do the iteration and store the IDs that were found so that on a dynamic_page_cache hit we only need to iterate the ones used by that page, even better.

#27: Yes, that is exactly what I thought. A CachePlaceholder manager like thing.

It would be great to have that anyway and it would also be great to easily register 'global lazybuilders' so to speak.

As get-me-the-current-url is very very useful.

e.g. lets take a [current-url] token in a block, which makes the block cacheable only per url, but with the transparent service it gets very simple to use a placeholder in between and just at rendering time replace it.

#27 + #28: But we don't want to do all that server-side stuff for dynamically generated responses (i.e. for auth users). So we need a JS solution for that. So, let me repeat #26:

Whereas a JS solution cannot use string parsing. It must rely on the DOM.

I.e. how does that system allow the JS to still work?

I don't see the value in reinventing the wheel here and potentially coming up with new APIs during RC. We can reuse a pre-existing pattern, that has been working fine for almost two years now (it landed on January 23, 2014).

Finally, the JS-based solution is also a great fit for https://github.com/rails/turbolinks/, which we could (and should?!) add in addition to BigPipe, for maximum speed-up. It allows us to even start from a server-side rendered destination query string and replace it when we transition to a different page using Turbolinks. Without some marker on the link, we would not be able to find it.

But we don't want to do all that server-side stuff for dynamically generated responses (i.e. for auth users). So we need a JS solution for that.

Why? I think #27 would actually be quite fast to do server-side after a dynamic_page_cache hit for an auth user? Or am I wrong about that?

Regardless of implementation, IMO it would be great to get this committed during RC, so adding the tag so core committers can discuss it.

Why? I think #27 would actually be quite fast to do server-side after a dynamic_page_cache hit for an auth user? Or am I wrong about that?

Yes, that is wrong IMO.

By the same reasoning, we should do this for "active" links too.

The JS solution allows us to do the work after the page has begun rendering in the browser — it allows us to do a computation basically in a JIT fashion.

The PHP solution requires us to do the work before sending the relevant HTML. In HEAD, that means literally blocking the response on those placeholders being processed. In a BigPipe world, that means that every placeholder that is rendered also needs to go through this "cache placeholder response filter".
Let's not forget it can't be implemented as a regular placeholder (#attached[placeholders]), as I said in #24: […] how are you going to add those placeholders? […] Neither outbound route processors not outbound path processors [allow you to attach such "cache placeholders"]. So then you're back to a response filter-style approach (like this patch does).

In general: if there's simple replacement work like this happening, that may affect many things on the page, and it's not something critical… then it's better to do it in JS, because it'll result in better perceived performance, because the server isn't slowed down by it.

I'm not convinced that we do, but if we really want JS for auth and no-JS for anon, then can we do this:

LoginLogoutMenuLink::getOptions() does:

$options['attributes']['data-current-path-destination'] = TRUE;

regardless of user.

system_page_attachments() stays the same as #19: adds the library only for authenticated.

We add an outbound path processor that only for anonymous does:

if (!empty($options['attributes']['data-current-path-destination'])) {
  $options['query']['destination'] = $this->currentPath->getPath();
}

I'm not convinced that we do, but if we really want JS for auth

That was a xpost with #32, not a reply to it.

#33: yes, that's a way to still allow the JS to work, i.e. that would address my remark in #29: Without some marker on the link, we would not be able to find it.

… except that $options as passed to \Drupal\Core\PathProcessor\OutboundPathProcessorInterface::processOutbound() apparently does not have either the final value or changes to it may be lost. IIRC @pwolanin ran into that a week or so ago.

Can somebody explain to me why we are seemingly diverging so much, even though we can literally apply a solid, battle-hardened pattern that we've had for almost two years? It seems to me like all this should be follow-up material, we should fix the regression with as few changes (and thus as few new patterns) as possible.

Can somebody explain to me why we are seemingly diverging so much, even though we can literally apply a solid, battle-hardened pattern that we've had for almost two years? It seems to me like all this should be follow-up material, we should fix the regression with ... as few new patterns ... as possible.

Yeah, that's fair. Per #26, although #19's CurrentPathDestinationLinkResponseFilter seems pretty ugly, it just copies the pattern used by HEAD's existing ActiveLinkResponseFilter, which has been in HEAD in more or less its current form for 6 months, and the ugly part existed in HEAD in SystemController for over a year before that. Based on that, I guess it does make sense to apply the same pattern here and punt ideas for improvement to follow-ups.

For the Needs manual testing part, pinging @nod_ and @droplet.

This has now been silent for 25 days. This is a huge usability regression and will impact many, many end users of Drupal.

IMO we totally should get this done before release.

Does this need some committer direction to move forward? I am also a bit worried, we might launch with this and over a implementation discussion that can easily be refined after release.

Uhm, we are waiting for manual testing from nod_ and/or droplet as far as I can see.

I stated in IRC that I am fine with copying the same pattern for now.

Assigning to nod_ for feedback.

Login and logout work for
1) /contact
2) /node/1
3) homepage
4) /search (without "keys" parameter)
and
5) Logout for /user/1
6) Login from /user/register?destination=/node/1%23comment-form

but login don't work like expected for:

1) /search (with "keys" parameter)
1) Enter something on search field
2) Click Search (result page /search/node?keys=something)
3) Login
Actual result:/search/node (missing keys parameter)
Expected result:/search/node?keys=something

2) Pages with access denied
1) Go to /node/add
2) Login
Actual result:/system/403
Expected result:/node/add
3) Unexisting page
1) Go to /mmmmmm (or other unexisting page)
2) Login
Actual result:/system/404
Expected result: homepage (or other accessible page for user)

Funny, nobody thought of the query string before you!

I'll fix that, but this still needs a JavaScript review, so setting back to "needs review" for that.

403/404 should be fixed also

+++ b/core/modules/contextual/js/contextual.js
@@ -54,12 +54,13 @@
+    window.setTimeout(function () {
+      Drupal.attachBehaviors($contextual.get(0));
     });

missing 2nd required argument (delay)

First, a straight rebase against HEAD (no conflicts encountered). Should be green again.

#41 + #44: regarding 403/404 getting the wrong responses: that is caused by this being 4xx responses being generated incorrectly in HEAD. If you apply this patch and #2595695: 4xx handling using subrequests: no longer able to vary by URL, you'll see that it does work correctly.

#44: RE: setTimeout(): good catch, that should have a delay of zero.

@Wim otoh there could be implementation based on HTTP[referer] header that most browsers sending

Now also fixed #41's query string problems.

#49: well, that's for a distant future, where HTTP/1 doesn't matter anymore. :)

@xjm and I agree that this is an RC target.

Minor change in the comment block

+   *   The current path.
+   * @param \Symfony\Component\HttpFoundation\RequestStack
+   *   The request stack.
+   */
+  public function __construct(AccountInterface $current_user, CurrentPathStack $current_path, RequestStack $request_stack) {

@param \Symfony\Component\HttpFoundation\RequestStack
should be replaced by
@param \Symfony\Component\HttpFoundation\RequestStack $request_stack

#52: Good nitpick, thanks. Can be fixed on commit.

Removed the nested attachBehavior call and called directly the attach function, contextual depend on the script so we're sure it'll be there.

Apart from that the JS is ok with me.

Alright!

Who dares RTBC? :)

RTBC - looks great to me!

We're in the final home stretch of preparing 8.0.0, and we won't be committing the majority of "rc target" issues any more, but still leaving the tag on them to triage them for patch-release or minor-release post 8.0.0 release. Meanwhile, switching to "8.0.0 target" for issues we'd still love to get in before 8.0.0.

• Fixed the #52 nitpick.
• Provided the test-only patch #58 asked.

This does not work for me.

1. Install standard
2. Log out
3. Go to /admin
4. Get 403 but the link on the log in is http://DOMAIN/user/login?destination=system/403

I have render cache disabled.

#60: that is merely another known bug you are seeing, see #46.

I'm not sure what the point of the JS version is tbh - if a user is logged in they are not going to have a log in link. Also the current path in drupalSettings for /admin is completely wrong - it is system/403 :(

Here's a fix and test for anon.

So I think that the currentPath being system/403 or system/404 in drupalSettings feels like a pretty serious bug.

Okay, let's answer this again, point-by-point:

#64:

I'm not sure what the point of the JS version is tbh - if a user is logged in they are not going to have a log in link.

AFAICT there are plenty of use cases where we want to have a ?destination=<current location> query string for authenticated users. In Drupal core, there's only contextual links. In contrib, there will surely be more. Anything where the authenticated user does something and should end up back in the place they started can use this.

#64 + #65:

Also the current path in drupalSettings for /admin is completely wrong - it is system/403 :(

This is what you already said in #60, to which I already replied in #62.

Drupal 8 HEAD handles 403/404 responses incorrectly. This is a known bug. Apply #2595695: 4xx handling using subrequests: no longer able to vary by URL and it will work as expected. That patch has already been deemed okay to be committed in a patch release of Drupal 8.0.x.

#65's work-around is wrong; the proper fix is to fix how 403/404 responses are generated, which is what #2595695: 4xx handling using subrequests: no longer able to vary by URL already fixes. The patch there is ready.

I think it's fine if this gets committed during the 8.0.x cycle. It doesn't have to happen by 8.0.0. Even though it results in a frustrating UX. If we don't feel 100% about this patch, we shouldn't commit it now, a day before 8.0.0.

So applying #2595695: 4xx handling using subrequests: no longer able to vary by URL doesn't fix the currentPath in drupalSettings when the a user is logged in but does not have permissions to access /admin...

<script type="application/json" data-drupal-selector="drupal-settings-json">{"path":{"baseUrl":"\/","scriptPath":null,"pathPrefix":"","currentPath":"system\/403","currentPathIsAdmin":false,"isFront":false,"currentLanguage":"en","currentQuery":{"_exception_statuscode":403,"destination":"\/admin"}},"pluralDelimiter":"\u0003","ajaxTrustedUrl":{"\/search\/node":true},"user":{"uid":"2","permissionsHash":"ed011ac77bd12c096da236a9762d75bbe23d5a5ef09539388da1de53794138ca"}}</script>

I would argue the currentPath and currentPathIsAdmin are both wrong here.

Also I removed the "8.0.0 target because I think we need to fix https://www.drupal.org/node/2595695 - therefore postponing and marking as a minor version target.

8.1.x has been opened by now. The patch in #65 still applies.

Keeping postponed because #2595695: 4xx handling using subrequests: no longer able to vary by URL needs to be fixed first.

#2595695: 4xx handling using subrequests: no longer able to vary by URL landed.

Reuploading @alexpott's patch of #65, straight reroll.

Still the same 2 failures after ~7 weeks, good.

I can't believe not more people are complaining this is annoying?! I've done the work to fix it way back then, but it seems it's not actually a problem? It definitely annoys me though. But we went two minor releases without a single comment, which I think is grounds enough to demote from major to normal, and probably even to minor.

It bugs me a lot.

It is very annoying.

Though I think if the installer didn't log you in directly, it would be even more widely reported.

I think 'normal' is a better compromise between major and minor.

People are now creating new contrib modules to fix this, and they're not finding this issue.

#2824418: [D8] Login Return Page

Ok, straight reroll from #73.

hahaha I forgot the patch. xD.

I fixed the more straightforward tests.

Let's see what the bot says.

I think this is beyond me, I tried today to fix the failing tests and BigPipe always fails in my local (even without this patch) and the fail related with outside_in return:

0 passes, 0 fails, 0 exceptions

I will leave it for now, in case someone else wants to give it a try.

Regards.

This issue was hard to find. I was fairly certain it was a regression and also expected lots of people to be complaining about it, and was specifically looking for this issue, and somehow didn't find it. Gnuget made a whole module to work around it.

I think folks had settled on normal priority, so you'll need to make a case for raising it to major.

The BigPipe fail is trivial: in HEAD, it doesn't expect the core/drupal.current-path-destination-link library to be listed, but this patch adds that. So BigPipe's expectation needs to be updated.

The StandardTest fail looks like it may have a small error in its expectation in the new test assertion.

Thanks for the feedback.

New patch.

Is there still no drupal 8 functionality that returns a user to the page they clicked the login button from? I've been searching for how to do it, came upon gnuget's blog post, then his unapproved module to here. This seems like something that drupal should be able to do right?

Yes, this is a regression, this is something that Drupal 7 does correct and D8 doesn't.

The last patch just needs a bit of work and should be ready to be committed, due to my experience working with JS I wasn't able to figure it out the problem.

I hope to try again soon.

I'm glad you are working on it gnuget. I'll keep an eye on this thread with hope and anticipation.

Rerolled. Wasn't as straightforward as I hoped. Patch is somewhat smaller now. Some tests have changed over time, and so some code just becomes obsolet.

This seems to be the case for :

BigPipePlaceholderTestCases
MenuTest
UserLoginTest
DisplayTest
NodeRevisionsTest

Lets see what the bot says.

Next round. Locally the ContextualDynamicContextTest runs just fine. Needs some more investigation. Hope this one fixes the StandardTest fail.

Don't know for what the base_uri stuff in StandardTest is for. Removing it, fixes the fail - at least locally. So lets ask the boot again.

I don't think removing it is the right solution, it looks like it's testing something real. Can you figure out what href the assertion is expecting and what it's actually getting?

I tested the patch in #106 and it did not play well with the cas module enabled

My use case: I have cas "forced login" turned on for all "/user/*" links on D8 client sites so that clicking on the login menu link actually redirects to /caslogin which in turn redirects to my SSO server then back to the client site.

Without the patch:
* I am redirected to my cas server for login and then dumped back to /user as this issue description notes

With the patch:
* I am redirected right back to my current page and never go to the cas server

Here's an attempt at a straight re-roll of 106 against 8.8.x -- not trying to fix any of the test issues mentioned in 107.

And here's one that applies cleanly to 8.6.x.

I tested #110.
Using Drupal 8.7.x + Umami.

I checked it on a multilingual website. The login link works great on every page, it goes to the login screen and then redirects to the page I started from.

@azinck you might want to upload #110 again as it is the rtbc patch and you want the rtbc retest to pick that patch and not the 8.6.x one.

@shaal it sounds like #107 and #108 need to be addressed.

Re-uploading #110 to make the 8.8.x patch the current one.

Also realised that #107/ #108 are yet to addressed.

FWIW I'm pretty sure #108 is a bug in the CAS module and how it's handling its redirects.

@azinck that might be the case but then that means we need to somehow either fix that module - it's quite widely used - or at least prep a workaround so sites can rely on the old behaviour.

Before I take a bigger look, lets move the changes to the correct place. Thanks.

1. +++ b/core/misc/current-path-destination-link.js
   @@ -0,0 +1,32 @@
   +/**
   + * @file
   + * Attaches behaviors for Drupal's current path destination link query string.
   + */
   +
   

   This should be in an .es6 file and then built.

2. +++ b/core/modules/contextual/js/contextual.js
   @@ -55,12 +55,9 @@
   +    // Call Drupal.behaviors.currentPathDestinationLink.attach directly to
   +    // set all destination parameters.
   +    Drupal.behaviors.currentPathDestinationLink.attach($contextual.get(0));
   

   These changes should be in the .es6 file and then run the build process.

Regarding #108 - I am the maintainer of the CAS module. This patch just surfaces a problem in the CAS module. It's not a problem with this patch. I'll fix the issue in #3029612: CAS automatic login should respect existing destination parameter

I can confirm that the issues with CAS are resolved.
We are seeing that the patch in #110 works on all pages except the front page. On the front page the destination parameter is still unset.

So we are NW for #107

So close, yet so far.

I tested the patch, it seems to be working fine to add destination to login link. 2 comments though:
- If I am on the homepage, I got "destination=" added - which then brings me to the user profile page after login (not back to homepage)
- If I am navgating from login page to register or lost password & back to login page, I am losing my destination parameter. It would be great if the destination is saved until I log-in or cancel my registration / login process. What do you think?

I've just run into a bug caused by this patch. I'm trying to serve up HTML files as private files in Drupal and this patch attempts to call setContent on that response, which is not permitted since it's a BinaryFileResponse object so an exception is thrown:

"The content cannot be set on a BinaryFileResponse instance"

I'm honestly not sure of the best approach here...should we just check whether our response is an instance of the class or has the class as one of its parents? That would block anything that might extend that class but I suppose it's possible that some subclass of BinaryFileResponse *might* be able to handle a setContent call. Is there any other info we can key on to determine if calling setContent would be appropriate here? Or should we just catch the exception?

Let's try this. This includes a small reroll change for 8.9x plus adds a check to ensure we don't try to modify BinaryFileResponse responses.

Leaving as "needs work" due to the other issues flagged in this thread.

Re #126 can we catch the LogicException that BinaryFileResponse throws?

Re-roll of #127 for 8.9 (only some small changes in the tests)

Just tiggering the testbot see where we're at.

Last patch failed to apply , re-rolling patch , kindly review.

Patch does not seem to apply to 9.1.3, for me, sorry.
I'll just add my voice to the outcry, that this has been broken for >5 years now, is unbelievable to me. Plus this issue node is more obscure than it ought to be, took me a while to come across it.
Thanks all for your work on fixing it.

patching file misc/current-path-destination-link.js

patching file modules/block/tests/src/Functional/Views/DisplayBlockTest.php

patching file modules/contextual/contextual.libraries.yml

patching file modules/contextual/js/contextual.js

patching file modules/contextual/src/Element/ContextualLinks.php

patching file modules/contextual/tests/src/Functional/ContextualDynamicContextTest.php

patching file modules/system/system.module

patching file modules/system/tests/src/Functional/Menu/MenuAccessTest.php

Hunk #1 FAILED at 61.

1 out of 1 hunk FAILED -- saving rejects to file modules/system/tests/src/Functional/Menu/MenuAccessTest.php.rej

patching file modules/user/src/Plugin/Menu/LoginLogoutMenuLink.php

patching file profiles/standard/tests/src/Functional/StandardTest.php

Hunk #1 succeeded at 268 (offset 1 line).

patching file tests/Drupal/Tests/Core/EventSubscriber/CurrentPathDestinationLinkResponseFilterTest.php

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-08-31/2582797-132_1.patch

1. +++ b/core/lib/Drupal/Core/EventSubscriber/CurrentPathDestinationLinkResponseFilter.php
   --- /dev/null
   +++ b/core/misc/current-path-destination-link.js
   

   Ther should be a .es6 version of this that we transpile to js.

2. +++ b/core/misc/current-path-destination-link.js
   @@ -0,0 +1,32 @@
   +        destinationValue += '?' + $.param(drupalSettings.path.currentQuery);
   

   I think we can use encodeURIComponent() here instead and then we don't need to depend on jQuery - which is good because that is slowly being removed from Drupal.

Re-rolled for 9.2.x. And addressed pointer in #135.

Worked on test case. Kindly review the patch.

Was #107 ever addressed? I don't see any follow up for it in the comments.

I manually tested on simplytest.me

• I was still directed to /user/1 when I log in from the homepage if there's nothing else in the url.
• I was still directed to /user/1 when I go to Log in to Create new acct/Reset pw, and then back to Log in.
• Otherwise, I was directed back to the page I started on.
• Log out always took me back to homepage.

On simplytest.me

1. Launch sandbox with drupal core 9.2.x and add patch.
2. Log in from home page - go to /user/1
3. log out from /user/1 - go to homepage
4. log in from /search/node?keys= (click search) - stay on /search/node?keys=
5. Log out
6. Log in from /search/node?keys=test (search for "test") - stay on /search/node?keys=test
7. still logged in, go to My account (/user/1)
8. Log out
9. Log in from Page not found (e.g., /test) - stay on /test
10. Still logged in, go to homepage
11. Log out
12. Log in from homepage - go to /user/1
13. Create a node of type Page - /node/add/page
14. Log out from /node/1
15. Log in from /node/1 - stay on node/1
16. Log out
17. Log in from Access denied (e.g., /node/add) - back to /node/add
18. Log out
19. go to Log in (/user/login?destination=), click on Create new account (/user/register), click on Log in. Log in - go to /user/1
20. Log out
21. go to Log in (/user/login?destination=), click on Reset your password (/user/password), click on Log in. Log in - go to /user/1
22. Log out
23. Search for text /search/node?keys=test, click Log in, click Create new account, click Log in. Log in - back to /user/1
24. Log out
25. Log in from /node (which is the default front page) - back to /node
26. change default front page (/admin/config/system/site-information) to /node/1, go to /node/1
27. Log out
28. Add /node/1 to url, Log in - back to /node/1
29. Log out
30. Log in from /contact - back to /contact

Re-rolled for 9.2.5 (a test got slightly changed)

Re-rolled for 9.2.6

Re-rolled for 9.3.0

I think that #107, #119 and #135 are addressed.

But we are NW for a decision on #128 and for test coverage of the issue raised in #126.

This is my first reroll of this coding issue from #144 above.
Also fixed a white space coding issue from testbot.

#146 doesn't work with Drupal core 9.4, re-rolling a new patch file.

I compared the patches in #146 and #149, and the re-roll looks good to me.

There are a lot of test failures, but I think they are all because of deprecated code. (I did not check them all.) For example:

Remaining self deprecation notices (4)

4x: Drupal\Core\Http\RequestStack::getMasterRequest() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use getMainRequest() instead. See https://www.drupal.org/node/3253744
2x in BlockAdminThemeTest::testAdminTheme from Drupal\Tests\block\Functional
2x in BlockAdminThemeTest::testSevenAdminTheme from Drupal\Tests\block\Functional

That should be easy to fix. But see the change record: it might be more than a 1-line change. If you can run one of the failing tests locally, it will be a good idea to check before asking the testbot to try again.

Replace the getMasterRequest deprecated function with getMainRequest, uploading a new patch file.

Much better! Only 1 test failure, not 1,305.

I think that remaining failure is real, although I get something different from the testbot. In fact, the testbot's report is confusing:

There was 1 failure:

1) Drupal\Tests\block\Functional\Views\DisplayBlockTest::testBlockContextualLinks
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Configure blockRemove blockEdit view'
+'Configure blockRemove blockEdit view'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:90
/var/www/html/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php:406
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

I get something that makes more sense:

There was 1 failure:

1) Drupal\Tests\block\Functional\Views\DisplayBlockTest::testBlockContextualLinks
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<ul class="contextual-links"><li><a href="/admin/structure/block/manage/x0jxmkla" data-current-path-destination>Configure block</a></li><li><a href="/admin/structure/block/manage/nv5qbxlh/delete">Remove block</a></li><li><a href="/admin/structure/views/view/test_view_block/edit/block_1" data-current-path-destination>Edit view</a></li></ul>'
+'<ul class="contextual-links"><li><a href="/admin/structure/block/manage/x0jxmkla" data-current-path-destination>Configure block</a></li><li><a href="/admin/structure/block/manage/x0jxmkla/delete" data-current-path-destination>Remove block</a></li><li><a href="/admin/structure/views/view/test_view_block/edit/block_1" data-current-path-destination>Edit view</a></li></ul>'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:119
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:98
/var/www/html/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php:406
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

That looks related to this issue, and I think this is a case where we need to update the test.

When I run the test with PHP 8.1, I also get this:

There were 3 errors:

1) Drupal\Tests\block\Functional\Views\DisplayBlockTest::testBlockCategory
Exception: Deprecated function: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated
Drupal\Core\EventSubscriber\CurrentPathDestinationLinkResponseFilter->onResponse()() (Line: 71)
...

I think that should be fixed, too. It comes from these lines:

    if ((stripos($response->headers->get('Content-Type'), 'text/html') === FALSE)
      || is_a($response, '\Symfony\Component\HttpFoundation\BinaryFileResponse')) {
      return;
    }

The patches in #149 and #152 are oddly formed: to show an empty line of context (neither added nor removed) they use an empty line. Every patch I have ever looked at until now uses a line with a single space. To my surprise, both git apply and patch -p1 applied the patches without complaining. But the interdiff utility failed.

I am attaching

• A version of the patch from #152 with spaces added to those empty lines.
• A diff comparing the patches from #146 and #149 (with spaces added) (reroll)
• An interdiff comparing the patches from #149 and #152.

@benjifisher do you have an opinion on #126 / #128 ?

Rerolling patch in #153 to address PHP 8.1 issue with stripos() along with deprecation of FilterResponseEvent. Does not address the test issue. Both @benjifisher's and this patch address the BinaryFileResponse issue as suggested in the #127 patch.

I'm including an interdiff which has some changes due to rolling the newer patch against the latest 9.4.x + the changes above.

#153 suggests the remaining fail needs to be fixed by updating the test (the expectation I assume).

Re-rolled #155 for 9.5.9 (not fixing the test issue)