Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#53226 closed defect (bug) (fixed)

Password reset screen: `wp_lang` GET parameter is lost when a wrong email address is provided

Reported by: audrasjb's profile audrasjb Owned by: joedolson's profile 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 4 years ago.
Created patch.
53226.2.diff (1021 bytes) - added by ocean90 4 years ago.
53226.3.diff (1.9 KB) - added by kapilpaul 4 years ago.
Updated patch with support of wp-lang in login and registration link.
53226.4.diff (12.5 KB) - added by kapilpaul 4 years ago.
Updated patch. add support for wp_lang in whole login screen

Download all attachments as: .zip

Change History (24)

@kapilpaul
4 years ago

Created patch.

#1 @kapilpaul
4 years ago

  • Keywords has-patch added; needs-patch removed

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 @joedolson
4 years ago

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

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

@ocean90
4 years ago

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

@kapilpaul
4 years ago

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

#10 @kapilpaul
4 years ago

  • Keywords has-patch added; needs-patch removed

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

@kapilpaul
4 years ago

Updated patch. add support for wp_lang in whole login screen

#13 @joedolson
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 @SergeyBiryukov
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 @audrasjb
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 @hellofromTonya
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 @sabernhardt
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:

  1. Verify site language: English (US), with French and others installed.
  2. Visit /wp-login.php?action=lostpassword&wp_lang=fr_FR in a browser where I'm not already logged in (Edge 95 / Windows 10).
  3. Enter fake@name.com as the email address.
  4. The error still redirected to /wp-login.php?action=lostpassword, but the content stayed in French.

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

Last edited 3 years ago by joedolson (previous) (diff)

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.

Note: See TracTickets for help on using tickets.