- π¨π¦Canada robbdavis
Just had a client stumble upon this and the custom maintenance page and text made no sense in the context of failed login. They asked for a separate message / theme for failed login.
A separate template option would be great.
As a hack, I used a path-name class in the the body and hid the maintenance stuff and displayed blocked login stuff based on whether the path was `path-login`.
- Status changed to Needs review
7 months ago 4:29pm 19 June 2024 - π¨π¦Canada b_sharpe
Done. I did not create templates for themes like Claro/Olivero here as I believe those are opinionated and should be their own issues. I also didn't create tests here as no messaging has changed.
What might be needed is a change record, as pages previously utilizing the maintenance page for this will now get a different template. Marking for review for now.
- π¨π¦Canada b_sharpe
It appears there were tests around this already. Updated and added stable9 template as was expected as well.
- Status changed to Needs work
7 months ago 9:16pm 19 June 2024 - πΊπΈUnited States smustgrave
Neat. Will definitely need a CR for the template addition.
- Status changed to Needs review
7 months ago 1:39pm 2 July 2024 - π§π·Brazil carolpettirossi Campinas - SP
I'd like to contribute with the Change Record. I've started it here: https://www.drupal.org/node/3458540 β
Can someone please review it?
- π¨π¦Canada b_sharpe
Looks great! Thanks @carolpettirossi. Leaving for someone else to RTBC as I was original committer.
- Status changed to RTBC
7 months ago 6:38pm 5 July 2024 - πΊπ¦Ukraine kolesnikoff
I reviewed the code, looks good, no issues found.
Also tested the branch locally:
1. Created user "bruteforce".2. Tried to log in using incorrect password. Received standard error message.
3. After 5 failed attempts received new error message
4. Created new theme "Test" based on Olivero. Copied the template failed-login-page.html.twig and changed it.
5. Cleared cache, tried to log in again. Received error page that used new template.
6. Test passed.
- Status changed to Needs work
6 months ago 12:40pm 17 July 2024 - π¬π§United Kingdom catch
The new template has identical markup to the default maintenance page template. If the intention is to allow themes to theme them differently, then we ought to be able to use template suggestions for this instead of an entirely new theme hook.
See #1804614: [meta] Consolidate similar twig templates and properly use theme suggestions in core β .
This would also provide a smoother upgrade path for themes that provide a custom maintenance template but want it to be the same for the failed login page.
- Merge request !89303306107: Allow separate template suggestions for flood pages. β (Closed) created by b_sharpe
- π¨π¦Canada b_sharpe
b_sharpe β changed the visibility of the branch 3306107-create-a-failed to hidden.
- π¨π¦Canada b_sharpe
b_sharpe β changed the visibility of the branch 11.x to hidden.
- Status changed to Needs review
6 months ago 11:31pm 25 July 2024 - π¨π¦Canada b_sharpe
@catch good call, I've moved it to just an extension of the maintenance page which will add the theme suggestion.
- Status changed to Needs work
6 months ago 4:22pm 1 August 2024 - πΊπΈUnited States smustgrave
Can IS be updated with latest solution. Not 100% sure if a change record is still needed? If so existing one will need updating.
- Status changed to Needs review
6 months ago 6:07pm 1 August 2024 - π¨π¦Canada b_sharpe
IS updated. I don't think a change record is needed anymore as there is no longer any changes to core implementations here, just an additional theme suggestion for those who want it.
- Status changed to RTBC
6 months ago 3:04pm 2 August 2024 - πΊπΈUnited States smustgrave
Going to agree, this seems like a much simpler solution.
- Status changed to Needs work
6 months ago 11:46pm 2 August 2024 - π¬π§United Kingdom catch
Why
flood
and notlogin
for the theme suggestion? Otherwise looks good though. - Status changed to Needs review
6 months ago 12:50am 3 August 2024 - π¨π¦Canada b_sharpe
@catch because I realized the template is used for two different cases, both ip flood and login failure flood, seemed to be the common name
- Status changed to RTBC
6 months ago 6:36pm 7 August 2024 - πΊπΈUnited States smustgrave
@catch maybe the issue summary could be updated some? Or not sure if needed
- π³πΏNew Zealand quietone
Now this is a bit interesting about the change record. There is one here but #19 says it is not needed. How will the committer (or me who follows up making sure change records are published) are to know to not publish the change record.
I think that needs to be made clear in the issue summary. Also, changing the CR as well would help. There is no set practice for this and I have seen different approaches in the change records themselves. Some people have changed the title, other have added a new first sentence is added like 'do not publish', and others delete the contents as well.
To avoid setting this back to NW I have updated the CR body to indicate that it should not be published.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
I love the way this issue evolved and the end result is really simple and elegant. Thanks to everyone involved!
Also +1 for @quietone's concerns regarding change records, it would be nice if we could flag them for removal if throughout the lifetime of an issue it becomes apparent they are no longer needed. I couldn't find an issue for this in the issue queue β , but as I was searching through it I found your comment here (on the same day as your last comment in this issue): β¨ Publish change records when issues are fixed Postponed So I guess that issue can be used for that discussion.
Anyways, big +1 from me.
- Status changed to Fixed
4 months ago 4:26pm 9 September 2024 - π¬π§United Kingdom catch
I went ahead and deleted the change record here.
Committed/pushed to 11.x and cherry-picked to 10.4.x, thanks!
Automatically closed - issue fixed for 2 weeks with no activity.