WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 7 weeks ago

#53226 accepted defect (bug)

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 keep en_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)

53226.diff​ (930 bytes) - added by kapilpaul 2 months ago.
Created patch.
53226.2.diff​ (1021 bytes) - added by ocean90 8 weeks ago.
53226.3.diff​ (1.9 KB) - added by kapilpaul 7 weeks ago.
Updated patch with support of wp-lang in login and registration link.
53226.4.diff​ (12.5 KB) - added by kapilpaul 7 weeks ago.
Updated patch. add support for wp_lang in whole login screen

Download all attachments as: .zip

Change History (20)

@kapilpaul
2 months ago

Created patch.

#1 @kapilpaul
2 months ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in ​PR #1281 on ​WordPress/wordpress-develop by ​kapilpaul.


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


2 months ago

#4 @joedolson
2 months ago

  • Keywords needs-testing added
  • Owner set to joedolson
  • Status changed from new to accepted

#5 @justinahinon
8 weeks 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.

@ocean90
8 weeks ago

#6 @ocean90
8 weeks 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 @kapilpaul
8 weeks 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.


7 weeks ago

#9 @joedolson
7 weeks 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.

@kapilpaul
7 weeks ago

Updated patch with support of wp-lang in login and registration link.

#10 @kapilpaul
7 weeks ago

  • Keywords has-patch added; needs-patch removed

#11 @kapilpaul
7 weeks 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 @ocean90
7 weeks 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.

@kapilpaul
7 weeks ago

Updated patch. add support for wp_lang in whole login screen

#13 @joedolson
7 weeks 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 @SergeyBiryukov
7 weeks 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 @audrasjb
7 weeks 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 @hellofromTonya
7 weeks 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.

Note: See TracTickets for help on using tickets.