Problem/Motivation

Follow-up to #2503755: Switch from user login block to login menu link and search block in standard profile

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

Proposed resolution

Add destination.

User interface changes

None.

API changes

None. API addition: links can set the data-current-path-destination attribute to automatically get the destination query argument

Data model changes

None.

Why this is an RC target

This is severe UX regression introduced by #2503755: Switch from user login block to login menu link and search block in standard profile

Files: 

Comments

YesCT created an issue. See original summary.

effulgentsia’s picture

Component: Bartik theme » user.module

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.

webchick’s picture

Title: login link has no destination=[current.path], so dumps you on the profile » [Regression] login link has no destination=[current.path], so dumps you on the profile
Priority: Normal » Major
Issue tags: +regression

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. :(

moshe weitzman’s picture

Title: [Regression] login link has no destination=[current.path], so dumps you on the profile » [Regression] login link has no destination=drupalSettings.path, so dumps you on the profile
Assigned: Unassigned » moshe weitzman

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?

catch’s picture

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

Wim Leers’s picture

Assigned: moshe weitzman » Wim Leers

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

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
Issue tags: +JavaScript
FileSize
12.35 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,139 pass(es), 16 fail(s), and 7 exception(s). View

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)
Wim Leers’s picture

Issue summary: View changes
catch’s picture

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

Status: Needs review » Needs work

The last submitted patch, 7: login_destination-2582797-7.patch, failed testing.

moshe weitzman’s picture

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.
Wim Leers’s picture

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

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

Wim Leers’s picture

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

The last submitted patch, 13: login_destination-2582797-13.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: login_destination-2582797-14.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
7.25 KB
18.14 KB

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

Status: Needs review » Needs work

The last submitted patch, 17: login_destination-2582797-17.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
23.87 KB
6.55 KB

Last fails fixed, test coverage added.

Berdir’s picture

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?

Wim Leers’s picture

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.

Fabianx’s picture

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.

moshe weitzman’s picture

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.

Wim Leers’s picture

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.

Fabianx’s picture

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.

Wim Leers’s picture

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.

effulgentsia’s picture

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.

Fabianx’s picture

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

Wim Leers’s picture

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

effulgentsia’s picture

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?

webchick’s picture

Issue tags: +rc target triage

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

Wim Leers’s picture

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.

effulgentsia’s picture

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();
}
effulgentsia’s picture

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.

Wim Leers’s picture

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

effulgentsia’s picture

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.

Wim Leers’s picture

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

Wim Leers’s picture

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.

Bojhan’s picture

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.

Fabianx’s picture

Assigned: Unassigned » nod_

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.

timisoreana’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing
FileSize
29.14 KB
38.4 KB
27.06 KB
33.67 KB
28.79 KB
35.46 KB

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)

Wim Leers’s picture

Status: Needs work » Needs review

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.

Status: Needs review » Needs work

The last submitted patch, 19: login_destination-2582797-19.patch, failed testing.

andypost’s picture

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)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
23.87 KB

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

Wim Leers’s picture

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

Wim Leers’s picture

Wim Leers’s picture

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

andypost’s picture

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

Wim Leers’s picture

Now also fixed #41's query string problems.

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

alexpott’s picture

Issue summary: View changes
Issue tags: -rc target triage +rc target

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

heykarthikwithu’s picture

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

Wim Leers’s picture

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

nod_’s picture

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.

Wim Leers’s picture

Assigned: nod_ » Unassigned

Alright!

Who dares RTBC? :)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - looks great to me!

effulgentsia’s picture

Issue tags: -rc target +8.0.0 target

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.

YesCT’s picture

Nice. I see the test for the query added in #50

Might be good to have a patch and tests-only patch so reviewers can see the fails and make sure they are failing in the way we expect.

Wim Leers’s picture

  • Fixed the #52 nitpick.
  • Provided the test-only patch #58 asked.
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

The last submitted patch, 59: login_destination-2582797-59-test-only-FAIL.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: login_destination-2582797-59-test-only-FAIL.patch, failed testing.

alexpott’s picture

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 :(

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
25.63 KB

Here's a fix and test for anon.

alexpott’s picture

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

Status: Needs review » Needs work

The last submitted patch, 65: 2582797-65.patch, failed testing.

Wim Leers’s picture

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.

alexpott’s picture

Issue tags: -8.0.0 target

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.

alexpott’s picture

Status: Needs work » Postponed
Issue tags: +minor version target

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.

Wim Leers’s picture

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

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.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs review » Needs work

The last submitted patch, 73: login_destination-2582797-73.patch, failed testing.

Wim Leers’s picture

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

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.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.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

Wim Leers’s picture

Priority: Major » Minor

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.

moshe weitzman’s picture

It bugs me a lot.

tim.plunkett’s picture

Priority: Minor » Normal

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.

Wim Leers’s picture

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

#2824418: [D8] Login Return Page

gnuget’s picture

Status: Needs work » Needs review

Ok, straight reroll from #73.

gnuget’s picture

hahaha I forgot the patch. xD.

Status: Needs review » Needs work

The last submitted patch, 83: login_destination-2582797-82.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
28.91 KB
3.28 KB

I fixed the more straightforward tests.

Let's see what the bot says.

Status: Needs review » Needs work

The last submitted patch, 85: login_destination-2582797-85.patch, failed testing.

Fidelix’s picture

gnuget’s picture

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.

mlncn’s picture

Priority: Normal » Major

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.

cilefen’s picture

Priority: Major » Normal

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

Wim Leers’s picture

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.

gnuget’s picture

Status: Needs work » Needs review
FileSize
29.84 KB
953 bytes

Thanks for the feedback.

New patch.

Status: Needs review » Needs work

The last submitted patch, 92: login_destination-2582797-92.patch, failed testing.