3rd attempt at fixing 42957. Replaces PRs 1095, 1896 by daveagp · Pull Request #6834 · WordPress/wordpress-develop

daveagp · 2024-06-16T05:26:06Z

On my website we get about one email a month from a person who can't create an account. This turns out to be caused by https://core.trac.wordpress.org/ticket/42957

you're using gmail
you try to create an account name ending in a period, e.g. Dave P.
the email is sent containing https://blah...&login=Dave%20P.
gmail generally assumes links ending in periods have the periods added by humans and thus ignores them
so clicking on it, you arrive at https://blah...&login=Dave%20P and the user's told the link is invalid

Fixes this by encoding the trailing period as an entity.

This is my first submitted pull request to WP (technically second but it's a fix to the first), thanks in advance for telling me if any more steps are needed.

Trac ticket: https://core.trac.wordpress.org/ticket/42957

github-actions · 2024-06-16T05:26:21Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props daveagp, dmsnell, mukesh27.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-06-16T05:48:49Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

src/wp-includes/functions.php

tests/phpunit/tests/functions/encodeQueryArgEndingPeriod.php

src/wp-includes/functions.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

dmsnell · 2024-06-25T19:12:17Z

src/wp-includes/functions.php

+		return '';
+	}
+
+	$url = rawurlencode( $url );


note that we don't want to call rawurlencode() on a full URL. it's only meant to encode the parts after the domain, and we want to avoid double-encoding it, since if we call this on an already-escaped URL, we'll end up escaping the escapes and corrupting it.

Fair enough, though I am going to update my pull request to match your other very elegant solution

Step 1/4 to implement cleaner solution due to dmnell https://core.trac.wordpress.org/ticket/42957#comment:37

Step 2/4

First edit with nice solution

See https://core.trac.wordpress.org/ticket/42957#comment:37

daveagp · 2024-06-30T05:03:01Z

I've reimplemented using the nice solution from https://core.trac.wordpress.org/ticket/42957#comment:37. Thanks @dmsnell for the review and thoughtful suggestion!

dmsnell

I've reimplemented…Thanks @dmsnell for the review and thoughtful suggestion!

Happy to help. Any problem we don't have to solve is usually better in my opinion than any solution to that problem 😉

…period. When WordPress sends out a password-reset or new-user email, it generates a link for someone to follow in order to take them to the reset page. If the user login name ends in a period, however, that generated URL will end in a period and many email clients will confuse it with a sentence-ending period instead of being part of the query arguments. In this patch, the generated URL's query argument are rearranged so that the link will never end in a period. Alternative ideas were explored to create a new function to escape URL-ending periods, but this patch resolves the reported problem without raising any further architectural questions. Developed in #6834 Discussed in https://core.trac.wordpress.org/ticket/42957 Props audrasjb, costdev, daveagp, dmsnell, hellofromTonya, markparnell, mukesh27, nhrrob, obrienlabs, paulcline. Fixes #42957. git-svn-id: https://develop.svn.wordpress.org/trunk@58674 602fd350-edb4-49c9-b593-d223f7449a82

dmsnell · 2024-07-04T22:01:02Z

Merged in [58674]
82deed3

…period. When WordPress sends out a password-reset or new-user email, it generates a link for someone to follow in order to take them to the reset page. If the user login name ends in a period, however, that generated URL will end in a period and many email clients will confuse it with a sentence-ending period instead of being part of the query arguments. In this patch, the generated URL's query argument are rearranged so that the link will never end in a period. Alternative ideas were explored to create a new function to escape URL-ending periods, but this patch resolves the reported problem without raising any further architectural questions. Developed in WordPress/wordpress-develop#6834 Discussed in https://core.trac.wordpress.org/ticket/42957 Props audrasjb, costdev, daveagp, dmsnell, hellofromTonya, markparnell, mukesh27, nhrrob, obrienlabs, paulcline. Fixes #42957. Built from https://develop.svn.wordpress.org/trunk@58674 git-svn-id: http://core.svn.wordpress.org/trunk@58076 1a063a9b-81f0-0310-95a4-ce76da25c4cd

…period. When WordPress sends out a password-reset or new-user email, it generates a link for someone to follow in order to take them to the reset page. If the user login name ends in a period, however, that generated URL will end in a period and many email clients will confuse it with a sentence-ending period instead of being part of the query arguments. In this patch, the generated URL's query argument are rearranged so that the link will never end in a period. Alternative ideas were explored to create a new function to escape URL-ending periods, but this patch resolves the reported problem without raising any further architectural questions. Developed in WordPress/wordpress-develop#6834 Discussed in https://core.trac.wordpress.org/ticket/42957 Props audrasjb, costdev, daveagp, dmsnell, hellofromTonya, markparnell, mukesh27, nhrrob, obrienlabs, paulcline. Fixes #42957. Built from https://develop.svn.wordpress.org/trunk@58674 git-svn-id: https://core.svn.wordpress.org/trunk@58076 1a063a9b-81f0-0310-95a4-ce76da25c4cd

…period. When WordPress sends out a password-reset or new-user email, it generates a link for someone to follow in order to take them to the reset page. If the user login name ends in a period, however, that generated URL will end in a period and many email clients will confuse it with a sentence-ending period instead of being part of the query arguments. In this patch, the generated URL's query argument are rearranged so that the link will never end in a period. Alternative ideas were explored to create a new function to escape URL-ending periods, but this patch resolves the reported problem without raising any further architectural questions. Developed in WordPress#6834 Discussed in https://core.trac.wordpress.org/ticket/42957 Props audrasjb, costdev, daveagp, dmsnell, hellofromTonya, markparnell, mukesh27, nhrrob, obrienlabs, paulcline. Fixes #42957. git-svn-id: https://develop.svn.wordpress.org/trunk@58674 602fd350-edb4-49c9-b593-d223f7449a82

daveagp added 4 commits June 16, 2024 00:48

copy of https://github.com/WordPress/wordpress-develop/pull/1896/files

e46b6ac

suggested rename

a8c9ad2

missed a rename

a496d87

fix renames. unit tests pass

2ae5bab

add missing trailing newline caught by php coding standards

05eccf4

mukeshpanchal27 reviewed Jun 16, 2024

View reviewed changes

src/wp-includes/functions.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Jun 16, 2024

View reviewed changes

tests/phpunit/tests/functions/encodeQueryArgEndingPeriod.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Jun 16, 2024

View reviewed changes

src/wp-includes/functions.php Outdated Show resolved Hide resolved

daveagp and others added 3 commits June 16, 2024 09:57

Update tests/phpunit/tests/functions/encodeQueryArgEndingPeriod.php

f6921b0

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Update src/wp-includes/functions.php

e1d8d56

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Add @access private, following example of other functions

055c7ab

dmsnell reviewed Jun 25, 2024

View reviewed changes

daveagp added 6 commits June 29, 2024 21:47

Delete src/wp-includes/functions.php

8e11d51

Step 1/4 to implement cleaner solution due to dmnell https://core.trac.wordpress.org/ticket/42957#comment:37

Delete tests/phpunit/tests/functions/encodeQueryArgEndingPeriod.php

b6b91a9

Step 2/4

Update pluggable.php

350ceb1

First edit with nice solution

Update user.php - final elegant dmsnell fix

4251cbb

See https://core.trac.wordpress.org/ticket/42957#comment:37

revert file

c7d8003

Revert changes in functions.php

b8faf66

daveagp added 2 commits June 29, 2024 22:05

Update pluggable.php - less fancy quotes

eb70e67

Update user.php - less fancy quotes

38c0296

dmsnell approved these changes Jul 1, 2024

View reviewed changes

daveagp and others added 4 commits July 4, 2024 11:58

Update pluggable.php - add comment

fc17842

Update user.php - add comment

eb3fb9c

Merge branch 'trunk' into 42957b

e65a51b

Expand comment warning not to move the query arg.

59aed17

dmsnell closed this Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3rd attempt at fixing 42957. Replaces PRs 1095, 1896#6834

3rd attempt at fixing 42957. Replaces PRs 1095, 1896#6834
daveagp wants to merge 20 commits intoWordPress:trunkfrom
daveagp:42957b

daveagp commented Jun 16, 2024

Uh oh!

github-actions bot commented Jun 16, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jun 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmsnell Jun 25, 2024

Uh oh!

daveagp Jun 30, 2024

Uh oh!

daveagp commented Jun 30, 2024

Uh oh!

dmsnell left a comment

Uh oh!

dmsnell commented Jul 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

daveagp commented Jun 16, 2024

Uh oh!

github-actions bot commented Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 16, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmsnell Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

daveagp Jun 30, 2024

Choose a reason for hiding this comment

Uh oh!

daveagp commented Jun 30, 2024

Uh oh!

dmsnell left a comment

Choose a reason for hiding this comment

Uh oh!

dmsnell commented Jul 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jun 16, 2024 •

edited

Loading