Pagniation links with/without trailing slashes. by peterwilsoncc · Pull Request #8591 · WordPress/wordpress-develop

peterwilsoncc · 2025-03-26T00:39:48Z

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.

github-actions · 2025-03-26T00:53:36Z

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.

github-actions · 2025-03-26T23:40:45Z

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 peterwilsoncc, westonruter, audrasjb, juanmaguitar.

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

src/wp-includes/general-template.php

tests/phpunit/tests/general/paginateLinks.php

westonruter · 2026-01-13T01:42:47Z

tests/phpunit/tests/general/paginateLinks.php

+			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' ),


How about a test case when there is no query string?

How about a test case when there is no query string?

An empty query string gets dropped so the output is covered above.

tests/phpunit/tests/general/paginateLinks.php

Copilot

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.

src/wp-includes/general-template.php

tests/phpunit/tests/general/paginateLinks.php

src/wp-includes/general-template.php

juanmaguitar · 2026-03-11T16:46:10Z

@peterwilsoncc thanks for working on this ticket!
There's some pending feedback to address. Do you think we can move forward with this PR in time for WP 7.0 RC1?

westonruter · 2026-03-14T01:32:49Z

tests/phpunit/tests/general/paginateLinks.php

+	/**
+	 * Post IDs created for shared fixtures.
+	 *
+	 * @var int[]
+	 */
+	protected static $post_ids = array();
+
+	/**
+	 * Category ID created for shared fixtures.
+	 *
+	 * @var int
+	 */
+	protected static $category_id = 0;


I found these weren't used except only in wpSetUpBeforeClass(). So I replaced them with just regular variables in 1b1328f.

westonruter · 2026-03-14T01:37:58Z

tests/phpunit/tests/general/paginateLinks.php

+	/**
+	 * 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() {
+		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.' );
+	}


This test passes in both trunk and this branch.

westonruter · 2026-03-14T01:38:29Z

tests/phpunit/tests/general/paginateLinks.php

+	/**
+	 * 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() {
+		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.' );
+	}


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

westonruter · 2026-03-14T01:38:53Z

tests/phpunit/tests/general/paginateLinks.php

+	/**
+	 * 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 ) {
+		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.' );
+	}


This test passes in trunk and this branch.

westonruter · 2026-03-14T01:39:22Z

tests/phpunit/tests/general/paginateLinks.php

+	/**
+	 * 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 ) {
+		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.' );
+	}


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

westonruter · 2026-03-14T01:47:56Z

src/wp-includes/general-template.php

+	if ( $wp_rewrite->using_permalinks() && ! $wp_rewrite->use_trailing_slashes ) {
+		$pagenum_link = untrailingslashit( $url_parts[0] );
+	} else {
+		$pagenum_link = trailingslashit( $url_parts[0] );
+	}


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.

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.

westonruter · 2026-03-14T01:53:54Z

src/wp-includes/general-template.php

 	// 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 ) {


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, '/' ) ) {

I think I'll keep it as is for clarity

westonruter · 2026-03-14T02:00:59Z

src/wp-includes/general-template.php

 	$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, '/' );
+	}


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

Suggested change

}

}

$pagenum_link .= '%_%';

westonruter · 2026-03-14T02:01:08Z

src/wp-includes/general-template.php

+	} else {
+		$pagenum_link = trailingslashit( $url_parts[0] );
+	}
+	$pagenum_link .= '%_%';


See https://github.com/WordPress/wordpress-develop/pull/8591/changes#r2934493034

Suggested change

$pagenum_link .= '%_%';

westonruter

@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 · 2026-03-15T22:56:04Z

tests/phpunit/tests/general/paginateLinks.php

+		$this->assertGreaterThan( 0, $found_links, 'There should be pagination links found.' );
+	}
+
+	/**


@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.

Co-authored-by: Weston Ruter <westonruter@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-Authored-By: westonruter <westonruter@git.wordpress.org>

Co-authored-by: Weston Ruter <westonruter@gmail.com>

peterwilsoncc force-pushed the fix/61393-pagination-urls-take-two branch from 212dd21 to 6c628cd Compare March 26, 2025 22:43

peterwilsoncc marked this pull request as ready for review March 26, 2025 23:39

peterwilsoncc force-pushed the fix/61393-pagination-urls-take-two branch from 34cf831 to c9bfda4 Compare October 28, 2025 01:39

peterwilsoncc force-pushed the fix/61393-pagination-urls-take-two branch from c9bfda4 to b6a01bf Compare November 6, 2025 23:11

westonruter reviewed Nov 6, 2025

View reviewed changes

src/wp-includes/general-template.php Outdated Show resolved Hide resolved

ozgursar added a commit to ozgursar/wordpress-develop that referenced this pull request Jan 12, 2026

Applied patch WordPress#8591

5117f0b

peterwilsoncc force-pushed the fix/61393-pagination-urls-take-two branch 2 times, most recently from 1f2dd39 to 6f57e5b Compare January 12, 2026 23:29

westonruter requested a review from Copilot January 13, 2026 01:25