#53226 closed defect (bug) (fixed)
Password reset screen: `wp_lang` GET parameter is lost when a wrong email address is provided
Reported by: | audrasjb | Owned by: | joedolson |
---|---|---|---|
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 (24)
This ticket was mentioned in PR #1281 on WordPress/wordpress-develop by kapilpaul.
4 years ago
#2
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.
4 years ago
#4
@
4 years ago
- Keywords needs-testing added
- Owner set to joedolson
- Status changed from new to accepted
#5
@
4 years 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
@
4 years 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
@
4 years 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 years ago
#9
@
4 years 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 years 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 years 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 years 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 years 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.
#15
@
4 years ago
On my side, 53226.2.diff
partially solves the problem. I think it's better to use it as a base to iterate on the issue 👍
#16
@
4 years ago
- Milestone changed from 5.8 to 5.9
Today is 5.8 Beta 1. Ran out of time for this one. Moving to 5.9.
#17
@
3 years ago
This may be fixed after adding the language switcher in #43700. The query string does not keep wp_lang
, though the language did not change for me.
Steps I followed:
- Verify site language: English (US), with French and others installed.
- Visit
/wp-login.php?action=lostpassword&wp_lang=fr_FR
in a browser where I'm not already logged in (Edge 95 / Windows 10). - Enter
fake@name.com
as the email address. - The error still redirected to
/wp-login.php?action=lostpassword
, but the content stayed in French.
#18
@
3 years ago
The fix to #43700 sets the wp_lang query string as a cookie, so I think it's fair to say that this is closed by that fix, as well. I'll repeat the test to verify.
#19
@
3 years ago
- Resolution set to fixed
- Status changed from accepted to closed
Repeated test, confirming this. I'm going to close this as fixed in 52058. Please re-open if any additional issues are spotted.
hellofromtonya commented on PR #1281:
3 years ago
#20
Thank you for this PR and your contribution! A different implementation was committed in https://core.trac.wordpress.org/changeset/52058 and the ticket closed.
Created patch.