Pagniation links with/without trailing slashes.#8591
Pagniation links with/without trailing slashes.#8591peterwilsoncc wants to merge 18 commits intoWordPress:trunkfrom
Conversation
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
from
212dd21 to
6c628cd
Compare
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
from
34cf831 to
c9bfda4
Compare
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
from
c9bfda4 to
b6a01bf
Compare
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
2 times, most recently
from
1f2dd39 to
6f57e5b
Compare
| array( 'foo=bar', 'foo=bar/' ), | ||
| array( 'foo=bar', 'foo=bar%2F' ), | ||
| array( 'foo=bar%2F', 'foo=bar' ), | ||
|
|
||
| array( 's=post', 's=post/' ), | ||
| array( 's=post', 's=post%2F' ), |
There was a problem hiding this comment.
How about a test case when there is no query string?
There was a problem hiding this comment.
How about a test case when there is no query string?
An empty query string gets dropped so the output is covered above.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@peterwilsoncc thanks for working on this ticket! |
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
from
e262810 to
e6b6553
Compare
| if ( $wp_rewrite->using_permalinks() && ! $wp_rewrite->use_trailing_slashes ) { | ||
| $pagenum_link = untrailingslashit( $url_parts[0] ); | ||
| } else { | ||
| $pagenum_link = trailingslashit( $url_parts[0] ); | ||
| } |
There was a problem hiding this comment.
My only outstanding question is whether or not it makes sense to use user_trailingslashit() here. For example, this has the equivalent effect, with the added benefit of having the value being passed through the user_trailingslashit filter:
| if ( $wp_rewrite->using_permalinks() && ! $wp_rewrite->use_trailing_slashes ) { | |
| $pagenum_link = untrailingslashit( $url_parts[0] ); | |
| } else { | |
| $pagenum_link = trailingslashit( $url_parts[0] ); | |
| } | |
| $pagenum_link = user_trailingslashit( $url_parts[0], 'paginate_links_base' ); |
The problem is user_trailingslashit() takes a second $type_of_url arg. A couple lines below we can see this function being used, and it is passing 'paged' as the value for this param. It's somewhat strange as well because below it is passing in page/%#% which is not a URL at all, but a path segment. I guess we could introduce a new value like "paginate_links_base", or else leave it blank.
There was a problem hiding this comment.
I'm not 100% clear why but user_trailingslashit() affects home_url() whereas the code here does not; so the home links end up being http://example.org rather than http://example.org/ -- which they should be regardless of the settings.
I recall digging in to this back when I started on this PR but can't remember what I found, it was a while ago.
| // URL base depends on permalink settings. | ||
| $format = $wp_rewrite->using_index_permalinks() && ! strpos( $pagenum_link, 'index.php' ) ? 'index.php/' : ''; | ||
| $format .= $wp_rewrite->using_permalinks() ? user_trailingslashit( $wp_rewrite->pagination_base . '/%#%', 'paged' ) : '?paged=%#%'; | ||
| if ( $wp_rewrite->using_permalinks() && ! $wp_rewrite->use_trailing_slashes ) { |
There was a problem hiding this comment.
The duplicated condition can be eliminated in favor of just looking at whether we need the slash:
| if ( $wp_rewrite->using_permalinks() && ! $wp_rewrite->use_trailing_slashes ) { | |
| if ( ! str_ends_with( $pagenum_link, '/' ) ) { |
There was a problem hiding this comment.
I think I'll keep it as is for clarity
| $format .= $wp_rewrite->using_permalinks() ? user_trailingslashit( $wp_rewrite->pagination_base . '/%#%', 'paged' ) : '?paged=%#%'; | ||
| if ( $wp_rewrite->using_permalinks() && ! $wp_rewrite->use_trailing_slashes ) { | ||
| $format = '/' . ltrim( $format, '/' ); | ||
| } |
There was a problem hiding this comment.
Oh, the $pagenum_link appending would need to be moved down here as well, of course.
| } | |
| } | |
| $pagenum_link .= '%_%'; |
| } else { | ||
| $pagenum_link = trailingslashit( $url_parts[0] ); | ||
| } | ||
| $pagenum_link .= '%_%'; |
There was a problem hiding this comment.
See https://github.com/WordPress/wordpress-develop/pull/8591/changes#r2934493034
| $pagenum_link .= '%_%'; |
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
@peterwilsoncc This is working well. Just a couple additional suggestions to surrounding the potential use of user_trailingslashit(). But since there was no use of this function before, it's not something that users would be missing since they didn't have it in the first place.
So I'm pre-approving.
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
from
26d9f89 to
f079de4
Compare
| $this->assertGreaterThan( 0, $found_links, 'There should be pagination links found.' ); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
@westonruter I added this test for sites with plain permalinks. To avoid a regex assertion, I used an array of expected links instead because I didn't want to miss the circumstance in which paged=X& got dropped from the output entirely.
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
from
f079de4 to
d2f01ff
Compare
Co-authored-by: Weston Ruter <[email protected]> Co-authored-by: Copilot <[email protected]>
Co-Authored-By: westonruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
peterwilsoncc
force-pushed
the
fix/61393-pagination-urls-take-two
branch
from
d2f01ff to
3f8302c
Compare
Second pass at https://core.trac.wordpress.org/ticket/61393
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.