- ๐ฌ๐งUnited Kingdom catch
@neclimdul re caching see
RouteProcessorCsrf::processOutbound()
which handles lazy building - so as long as the URL is built using the routing system, that should be fine.Obviously it won't work if a logout link is sent in an email or something, but that shouldn't happen for logout links really (and if it did, you'd get the form fallback).
- ๐บ๐ธUnited States xjm
Belatedly saving additional credits for folks who helped sort the release note.
- ๐บ๐ธUnited States xjm
Okay great, thanks! Let's mark this issue back to fixed, then.
It already has the "Needs followup" tag, which looks like it was left on by accident when @catch filed a different followup in #206. However, it looks like we have two additional potential followups to file, for #230 and the last paragraph in #23. I'll levae it to the respective contributors to file those followups since they will be best able to articulate the issues. Thanks again!
- ๐บ๐ธUnited States neclimdul Houston, TX
@xjm with the exception of my concern the link generated from the code in the changelog might store csrf links in caches, yes that addresses it.
- ๐บ๐ธUnited States xjm
@acbramley, sorry, I just realized this was actually my fault. I thought I had specifically stated where the comment had come from but apparently it never made it from my brain to the issue comment. :) My bad!
- ๐บ๐ธUnited States xjm
While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?
This is probably worth opening a followup issue. Discussion on this issue should only be about what needs to go in the release note so that it can be marked back to fixed. @neclimdul, does the current release note snippet in the IS give sufficient info for a site owner to address what you encountered? Thanks!
- ๐บ๐ธUnited States neclimdul Houston, TX
I see how my comment was misunderstood. It was technically in the sites themes but we have several form designs so a new form has to be assigned to a specific look or it just ends up "unthemed". On our site, unthemed was quite ugly.
More my point was more that any form isn't desirable and a fair few years of building Drupal sites has made me lazy about dropping a menu link in a template.
While we're discussing it, I haven't don't much testing but I think I echo some concerns from 14 years ago about caching. I haven't tested yet but is the CSRF token going to end up cached in templates using the code in the change log? Should core provide a lazy builder?
@Prashant.c That issue hasn't been finished yet, so the service doesn't exist.
- ๐ฎ๐ณIndia Prashant.c Dharamshala
I have a few points here:
- Can we update the procedural
user_logout()
in
core/modules/user/src/Form/UserLogoutConfirm.php
with the service from https://www.drupal.org/project/drupal/issues/2012976 ๐ Deprecate user_logout() and user_login_finalize() and replace with a service Needs review ?
Cancel
URL is always<front>
, Should not this be conditional based on the destination parameter?
- Can we update the procedural
- ๐ฆ๐บAustralia pameeela
Presumably it will have whatever styles apply to other forms though, and if you have users logging in I'd expect your login form to have some styles. I see the snippet is already updated to remove the part about the site's theme. I do think the rest can go in a follow up but just wondering about why a theme would have no styles or why this would look particularly ugly when it's just a heading and two buttons.
- ๐ฆ๐บAustralia acbramley
again, I linked the issue where the comment was posted above.
Sorry, I saw you link the issue but didn't (and still don't) see that was where the comments were coming from. Should've checked it I guess.
- ๐ฆ๐บAustralia mstrelan
I think the issue is that not all front end themes, particularly custom themes, will render the confirmation form nicely. Previously a link to
/user/logout
would log the user out immediately, whereas now it shows a confirmation form. So custom themes should follow the steps in the change record โ to avoid this. - ๐ฆ๐บAustralia pameeela
Link to the comment ๐ 10.3.0 release notes Active
Not much information to go on.
- ๐บ๐ธUnited States xjm
I asked @neclimdul to comment here so that he can speak for himself.
Meanwhile, fixed the link text accessibility issue.
- ๐บ๐ธUnited States xjm
@acbramley, again, I linked the issue where the comment was posted above.
- ๐ฆ๐บAustralia acbramley
Re #218 where are these comments coming from? The form is definitely themed as a frontend page (i.e the site's themed). Reworded the release note to remove references to this and provide better guidance. I was trying to keep the note brief as more information is available in the CR (which I linked to).
- ๐ฆ๐บAustralia pameeela
"a pretty ugly unthemed confirmation form"
It looked OK to me in testing but that was using Olivero (see screenshot in IS). I'm not sure why it would not appear in the site's theme? I think a follow up would be good with some additional details and what the proposed fixes would be to address the concern. But it would definitely be good to clarify for the release note as I think the current text is not very clear.
- ๐บ๐ธUnited States xjm
Thanks!
One improvement that we could probably add is explaining who is affected, and how. For example, one audience that is affected is site users, who will encounter a behavior change. I added a bit about that to the release note.
@neclimdul also described it as "a pretty ugly unthemed confirmation form", so I added a bit about that to the release note too. I wonder if there is something we could do to mitigate the theming issue? Separate possible followup, I guess.
Could someone check my modified release note above and ensure it's accurate? Meanwhile, I'm adding it to the 10.3.0 draft. Thanks!
- ๐บ๐ธUnited States xjm
Instructions for writing release notes: https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... โ
- ๐บ๐ธUnited States xjm
Per @neclimdul, this is potentially disruptive and merits a release note. Thanks!