Opened 5 months ago
Last modified 4 months ago
#53226 accepted defect (bug)
Password reset screen: `wp_lang` GET parameter is lost when a wrong email address is provided
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Login and Registration | Keywords: | has-patch |
Focuses: | accessibility | Cc: |
Description
Step to reproduce:
- Install 2 or more language packages on the website, let's say
fr_FR
for exampleΒ π· but keepen_US
as the default locale - go to
/wp-admin?action=lostpassword&wp_lang=fr_FR
- enter a wrong email address
- you'll be redirected to
/wp-admin?action=lostpassword
instead of/wp-admin?action=lostpassword&wp_lang=fr_FR
We should keep the value of the wp_lang
parameter.
Please note that this is an accessibility issue, because language changes must be announced before they happen π
Please don't forget to give proper props to @byohann6 who spotted the issue in the French Slack team π
Attachments (4)
Change History (20)
This ticket was mentioned in βPR #1281 on βWordPress/wordpress-develop by βkapilpaul.
5 months ago
This PR contains a fix of wp_lang
GET parameter is lost when a wrong email address is provided on the password reset screen.
Trac ticket: https://core.trac.wordpress.org/ticket/53226
This ticket was mentioned in βSlack in #accessibility by ryokuhi. βView the logs.
5 months ago
#4
@
5 months ago
- Keywords needs-testing added
- Owner set to joedolson
- Status changed from new to accepted
#5
@
5 months ago
I've tested both https://core.trac.wordpress.org/attachment/ticket/53226/53226.diff and βhttps://github.com/WordPress/wordpress-develop/pull/1281, but they do not seem to fix the issue.
I am still redirected to wp-login.php?action=lostpassword.
#6
@
5 months ago
- Keywords needs-patch added; has-patch needs-testing removed
It seems like core doesn't provide any lostpassword links with the wp_lang
parameter by default?
Since we only want to add the wp_lang
parameter and not all possible parameters, 53226.2.diffβ should be sufficient in this case.
But I think this is actually a bigger issue since non of the current links in wp-login.php pass a possible wp_lang
parameter to the next screen. For example on the lostpassword screen we also have the login and a registration link. Those should probably also use the same locale?
And with #53321 we also want to make sure that the redirections in case of an expired/invalid key will also use the same locale.
Wondering if a cookie for the locale which is only used on wp-login.php could simplify this a bit.
#7
@
5 months ago
@ocean90
If we only want to support wp-lang
then what about other get params will come in future? everytime do we need to add one by one as per the need?
This ticket was mentioned in βSlack in #accessibility by joedolson. βView the logs.
4 months ago
#9
@
4 months ago
Trying to think through whether there's a significant benefit or concern to using a cookie. A cookie makes the language choice more hidden, so it's harder for a user to change to the default language if there's something wrong with the language set for them, but I'm not convinced that's a huge problem.
I could go either way; but the patch seems simpler to get in if we don't mess with setting a cookie, and just check that parameter for all three links.
#11
@
4 months ago
@joedolson
I have updated the patch with a simpler solution without using cookie. 53226.3.diff has the support of wp-lang
in login and registration link as well.
#12
@
4 months ago
The patch is still incomplete since it only changes the links for the lostpassword screen. As mentioned above, this is an issue which should be fixed for the whole login screen. #43700 is related and seems to be a better long-term fix.
#13
@
4 months ago
Seems like a decision needs to be made on this. This patch would solve the immediate bug - where an already-set language preference is lost during the process.
#43700 appears to have stalled out, and would be classed as an enhancement, so it's definitely not something that would happen in 5.8.
@audrasjb Have you taken a look at #43700? Does the existing patch for that solve this problem?
#14
@
4 months ago
Looking at the latest patch, introducing a new function for a specific parameter like that does not seem like the right approach to me. Once #43700 is implemented, any code added here might be redundant and would need to be removed or deprecated for backward compatibility.
I think I would prefer a smaller patch like 53226.2.diffβ here to solve the immediate issue, even if it's not a complete solution, and then we can iterate on it in #43700.
Created patch.