-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Pagniation links with/without trailing slashes. #8591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
54b0e97
9f48cd9
5ec38b0
cd0891a
15119ab
d858766
b0ec9ea
9b9be2c
e4a8406
81346b1
15361e3
4c582f2
f5c8b20
963e4f6
e43d7e9
5785971
3f8302c
c9a244b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -4669,12 +4669,26 @@ function paginate_links( $args = '' ) { | |||||||
| $total = $wp_query->max_num_pages ?? 1; | ||||||||
| $current = get_query_var( 'paged' ) ? (int) get_query_var( 'paged' ) : 1; | ||||||||
|
|
||||||||
| // Append the format placeholder to the base URL. | ||||||||
| $pagenum_link = trailingslashit( $url_parts[0] ) . '%_%'; | ||||||||
| /* | ||||||||
| * Ensures sites not using trailing slashes get links in the form | ||||||||
| * `/page/2` rather than `/page/2/`. On these sites, linking to the | ||||||||
| * URL with a trailing slash will result in a 301 redirect from the | ||||||||
| * incorrect URL to the correctly formatted one. This presents an | ||||||||
| * unnecessary performance hit. | ||||||||
| */ | ||||||||
| 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 .= '%_%'; | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://github.com/WordPress/wordpress-develop/pull/8591/changes#r2934493034
Suggested change
|
||||||||
|
|
||||||||
| // 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 ) { | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The duplicated condition can be eliminated in favor of just looking at whether we need the slash:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll keep it as is for clarity |
||||||||
| $format = '/' . ltrim( $format, '/' ); | ||||||||
| } | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, the
Suggested change
|
||||||||
|
|
||||||||
| $defaults = array( | ||||||||
| 'base' => $pagenum_link, // http://example.com/all_posts.php%_% : %_% is replaced by format (below). | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,28 @@ | |
| */ | ||
| class Tests_General_PaginateLinks extends WP_UnitTestCase { | ||
|
|
||
| private $i18n_count = 0; | ||
| private int $i18n_count = 0; | ||
|
|
||
| /** | ||
| * Set up shared fixtures. | ||
| * | ||
| * @param WP_UnitTest_Factory $factory Factory instance. | ||
| */ | ||
| public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ): void { | ||
| $category_id = $factory->term->create( | ||
| array( | ||
| 'taxonomy' => 'category', | ||
| 'name' => 'Categorized', | ||
| ) | ||
| ); | ||
| self::assertIsInt( $category_id ); | ||
|
|
||
| $post_ids = $factory->post->create_many( 10 ); | ||
| foreach ( $post_ids as $post_id ) { | ||
| self::assertIsInt( $post_id ); | ||
| self::assertIsArray( wp_set_post_categories( $post_id, array( $category_id ) ) ); | ||
| } | ||
| } | ||
|
|
||
| public function set_up() { | ||
| parent::set_up(); | ||
|
|
@@ -383,4 +404,163 @@ public function test_custom_base_query_arg_should_be_stripped_from_current_url_b | |
| $page_2_url = home_url() . '?foo=2'; | ||
| $this->assertContains( "<a class=\"page-numbers\" href=\"$page_2_url\">2</a>", $links ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures pagination links include trailing slashes when the permalink structure includes them. | ||
| * | ||
| * @ticket 61393 | ||
| */ | ||
| public function test_permalinks_with_trailing_slash_produce_links_with_trailing_slashes(): void { | ||
| update_option( 'posts_per_page', 2 ); | ||
| $this->set_permalink_structure( '/%postname%/' ); | ||
|
|
||
| $this->go_to( '/category/categorized/page/2/' ); | ||
|
|
||
| // `current` needs to be passed as it's not picked up from the query vars set by `go_to()` above. | ||
| $links = paginate_links( array( 'current' => 2 ) ); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $links ); | ||
| $found_links = 0; | ||
| while ( $processor->next_tag( 'A' ) ) { | ||
| ++$found_links; | ||
| $href = (string) $processor->get_attribute( 'href' ); | ||
| $this->assertStringEndsWith( '/', $href, "Pagination links should end with a trailing slash, found: $href" ); | ||
| } | ||
| $this->assertGreaterThan( 0, $found_links, 'There should be pagination links found.' ); | ||
| } | ||
|
Comment on lines
+408
to
+430
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test passes in both |
||
|
|
||
| /** | ||
| * Ensures pagination links do not include trailing slashes when the permalink structure doesn't include them. | ||
| * | ||
| * @ticket 61393 | ||
| */ | ||
| public function test_permalinks_without_trailing_slash_produce_links_without_trailing_slashes(): void { | ||
| update_option( 'posts_per_page', 2 ); | ||
| $this->set_permalink_structure( '/%postname%' ); | ||
|
|
||
| $this->go_to( '/category/categorized/page/2' ); | ||
|
|
||
| // `current` needs to be passed as it's not picked up from the query vars set by `go_to()` above. | ||
| $links = paginate_links( array( 'current' => 2 ) ); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $links ); | ||
| $found_links = 0; | ||
| while ( $processor->next_tag( 'A' ) ) { | ||
| ++$found_links; | ||
| $href = (string) $processor->get_attribute( 'href' ); | ||
| $this->assertStringEndsNotWith( '/', $href, "Pagination links should not end with a trailing slash, found: $href" ); | ||
| } | ||
| $this->assertGreaterThan( 0, $found_links, 'There should be pagination links found.' ); | ||
| } | ||
|
Comment on lines
+432
to
+454
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test fails in |
||
|
|
||
| /** | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 |
||
| * Ensures pagination links do not include trailing slashes when the permalink structure is plain. | ||
| * | ||
| * @ticket 61393 | ||
| */ | ||
| public function test_plain_permalinks_are_not_modified_with_trailing_slash(): void { | ||
| update_option( 'posts_per_page', 2 ); | ||
| $this->set_permalink_structure( '' ); | ||
|
|
||
| $term = get_category_by_slug( 'categorized' ); | ||
| $this->assertInstanceOf( WP_Term::class, $term ); | ||
| $category_id = $term->term_id; | ||
| $this->go_to( "/?cat={$category_id}&paged=2" ); | ||
|
|
||
| // `current` needs to be passed as it's not picked up from the query vars set by `go_to()` above. | ||
| $links = paginate_links( array( 'current' => 2 ) ); | ||
|
|
||
| $expected_links = array( | ||
| home_url( "?cat={$category_id}" ), // Previous | ||
| home_url( "?cat={$category_id}" ), // Page 1 | ||
| home_url( "?paged=3&cat={$category_id}" ), // Page 3 | ||
| home_url( "?paged=4&cat={$category_id}" ), // Page 4 | ||
| home_url( "?paged=5&cat={$category_id}" ), // Page 5 | ||
| home_url( "?paged=3&cat={$category_id}" ), // Next | ||
| ); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $links ); | ||
| $found_links = 0; | ||
| while ( $processor->next_tag( 'A' ) ) { | ||
| $expected_link = $expected_links[ $found_links ] ?? ''; | ||
| ++$found_links; | ||
| $href = (string) $processor->get_attribute( 'href' ); | ||
| $this->assertSame( $expected_link, $href, "Pagination links should include the category query string, found: $href" ); | ||
| } | ||
| $this->assertSame( count( $expected_links ), $found_links, 'There should be this number of pagination links found.' ); | ||
| } | ||
|
|
||
| /** | ||
| * Ensures the pagination links do not modify query strings (permalinks with trailing slash). | ||
| * | ||
| * @ticket 61393 | ||
| * @ticket 63123 | ||
| * | ||
| * @dataProvider data_query_strings | ||
| * | ||
| * @param string $query_string Query string. | ||
| */ | ||
| public function test_permalinks_with_trailing_slash_do_not_modify_query_strings( string $query_string ): void { | ||
| update_option( 'posts_per_page', 2 ); | ||
| $this->set_permalink_structure( '/%postname%/' ); | ||
|
|
||
| $this->go_to( "/page/2/?{$query_string}" ); | ||
|
|
||
| // `current` needs to be passed as it's not picked up from the query vars set by `go_to()` above. | ||
| $links = paginate_links( array( 'current' => 2 ) ); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $links ); | ||
| $found_links = 0; | ||
| while ( $processor->next_tag( 'A' ) ) { | ||
| ++$found_links; | ||
| $href = (string) $processor->get_attribute( 'href' ); | ||
| $this->assertStringEndsWith( "/?{$query_string}", $href, "Pagination links should not modify the query string, found: $href" ); | ||
| } | ||
| $this->assertGreaterThan( 0, $found_links, 'There should be pagination links found.' ); | ||
| } | ||
|
Comment on lines
+493
to
+520
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test passes in |
||
|
|
||
| /** | ||
| * Ensures the pagination links do not modify query strings (permalinks without trailing slash). | ||
| * | ||
| * @ticket 61393 | ||
| * @ticket 63123 | ||
| * | ||
| * @dataProvider data_query_strings | ||
| * | ||
| * @param string $query_string Query string. | ||
| */ | ||
| public function test_permalinks_without_trailing_slash_do_not_modify_query_strings( string $query_string ): void { | ||
| update_option( 'posts_per_page', 2 ); | ||
| $this->set_permalink_structure( '/%postname%' ); | ||
|
|
||
| $this->go_to( "/page/2?{$query_string}" ); | ||
|
|
||
| // `current` needs to be passed as it's not picked up from the query vars set by `go_to()` above. | ||
| $links = paginate_links( array( 'current' => 2 ) ); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $links ); | ||
| $found_links = 0; | ||
| while ( $processor->next_tag( 'A' ) ) { | ||
| ++$found_links; | ||
| $href = (string) $processor->get_attribute( 'href' ); | ||
| $this->assertStringEndsWith( "?{$query_string}", $href, "Pagination links should not modify the query string, found: $href" ); | ||
| $this->assertStringEndsNotWith( "/?{$query_string}", $href, "Pagination links should not be slashed before the query string, found: $href" ); | ||
| } | ||
| $this->assertGreaterThan( 0, $found_links, 'There should be pagination links found.' ); | ||
| } | ||
|
Comment on lines
+522
to
+550
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test fails in |
||
|
|
||
| /** | ||
| * Data provider. | ||
| * | ||
| * @see self::test_permalinks_without_trailing_slash_do_not_modify_query_strings() | ||
| * @see self::test_permalinks_with_trailing_slash_do_not_modify_query_strings() | ||
| * | ||
| * @return array<string, array{ 0: string }> Data provider. | ||
| */ | ||
| public function data_query_strings(): array { | ||
| return array( | ||
| 'single query var' => array( 'foo=bar' ), | ||
| 'multi query vars' => array( 'foo=bar&pen=pencil' ), | ||
| ); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theuser_trailingslashitfilter:The problem is
user_trailingslashit()takes a second$type_of_urlarg. 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 inpage/%#%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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% clear why but
user_trailingslashit()affectshome_url()whereas the code here does not; so the home links end up beinghttp://example.orgrather thanhttp://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.