Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/wp-includes/general-template.php
Original file line number Diff line number Diff line change
Expand Up @@ -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] );
}
Comment on lines +4679 to +4683
Copy link
Member

@westonruter westonruter Mar 14, 2026

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 the user_trailingslashit filter:

Suggested change
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.

Copy link
Contributor Author

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() 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.

$pagenum_link .= '%_%';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


// 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 ) {
Copy link
Member

Choose a reason for hiding this comment

The 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
if ( $wp_rewrite->using_permalinks() && ! $wp_rewrite->use_trailing_slashes ) {
if ( ! str_ends_with( $pagenum_link, '/' ) ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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, '/' );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the $pagenum_link appending would need to be moved down here as well, of course.

Suggested change
}
}
$pagenum_link .= '%_%';


$defaults = array(
'base' => $pagenum_link, // http://example.com/all_posts.php%_% : %_% is replaced by format (below).
Expand Down
182 changes: 181 additions & 1 deletion tests/phpunit/tests/general/paginateLinks.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes in both trunk and this branch.


/**
* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails in trunk but passes in this branch, as expected.


/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 paged=X& got dropped from the output entirely.

* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes in trunk and this branch.


/**
* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails in trunk but passes in this branch, as expected.


/**
* 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' ),
);
}
}
Loading