Paginate links' "base" default value causes page 2 to display links incorrectly.

[Nessworthy]

Let's say if I called the following:

echo paginate_links( array(
    'current' => $current_page, // must be 2 to replicate
    'total' => $total_pages, // e.g. 5
) );

The following results in 'previous' and '1' page links becoming equal to the current URL. This means that if 'current' is being taken from the URL directly, the two links will always be equal to the second page.

Dissecting the source general-template.php:1922 , I can see that there is a check in the code where, if the current page is 2, it changes how the link is displayed. Here's the line:

$link = str_replace('%_%', 2 == $current ? '' : $format, $base);

So, using the default settings and setting the current page as two,
$link becomes an empty string.

This is because the default ('%_%') is being replaced by an empty string instead of $format .

Because link then doesn't contain the placeholder needed to show the intended page number, it remains empty when the next line is ran:

$link = str_replace('%#%', $current - 1, $link);

If no additional data is appended via. add args / add fragment, the string remains empty at the time of output, which browsers then default it to the current page URL.

[obenland]

Introduced in r4275. I was able to reproduce the bug with the example provided in the description.

The three instances where paginate_links() is used in core all define their own 'base' argument without '%_%', which is why they are not affected by this bug.

[kirasong]

This looks good!
It'd be great to have a unit test to commit alongside it, so that we don't run into this in the future.

[obenland]

Working on UT for this…

[obenland]

Added unit test. This is the first unit test I wrote for the WordPress project.
What can I do better?

[obenland]

Added UT for a false assertion and for the case of the current page being 1.

[nacin]

paginate_links() is a very hairy function, and it's "templating" of links is unlike really anything else in WP, so it's tough to grok. I'd like to do this earlier in a cycle just in case something goes wrong we didn't think about.

[SergeyBiryukov]

#28334 was marked as a duplicate.

[SergeyBiryukov]

In 28671:

Avoid an empty href attribute in paginate_links(). Add unit tests.

props obenland, Nessworthy.
fixes #24606.

[wonderboymusic]

[28671] is breaking unit tests and set the links equal to ?page=1 or /page/1/ which will force a canonical redirect.

[obenland]

24606.4.diff​ to fix existing UTs.

[SergeyBiryukov]

As noted above, ?page=1 or /page/1/ forces a canonical redirect. We should probably display the actual URL instead.

[SergeyBiryukov]

24606.5.diff​ reverts [28671] and uses the code from twentyfourteen_paging_nav()​ to set better defaults, to avoid empty href attributes (which [28671] attempted to fix) or canonical redirects.

[SergeyBiryukov]

The only difference I've noticed is that paginate_links() always used page query var as a default format value, while twentyfourteen_paging_nav() only checks for paged.

paged works for non-default permalinks, while page only seems to work for default ones.

Not sure if paginate_links() should always use paged for consistency, or we should check $wp_rewrite->using_permalinks(), like we do to set format.

Either way, I think paginate_links() should handle this too. Going to submit a new patch.

[SergeyBiryukov]

Looks like WP_TESTS_DOMAIN should be used instead of example.org in unit tests, see [28757].

[ocean90]

I talked to obenland at #wchh14 and we were (at least I was) a bit suprised to see the code of Twenty Fourteen in 24606.5.diff​ again. And yeah, the code shouldn't belong into a (default) theme, so +1 for moving the logic to paginate_links().

[SergeyBiryukov]

Replying to SergeyBiryukov:

Not sure if paginate_links() should always use paged for consistency, or we should check $wp_rewrite->using_permalinks(), like we do to set format.

get_pagenum_link()​ always uses paged, so 24606.6.diff​ should be good to go.

Core uses paginate_links() in WP_User_Search::do_paging()​, media_upload_library_form()​, wp_nav_menu_item_post_type_meta_box()​, and paginate_comments_links()​.

All these instances pass their own base, format, total, and current values, so this patch doesn't affect them. It just sets better defaults, so that themes don't have to calculate them on their own, like Twenty Fourteen does now.

[wonderboymusic]

In 28785:

Cleanup after [28671]:

• Set better defaults in paginate_links(), so that themes don't have to calculate them on their own, like Twenty Fourteen does now.
• Don't set page 1 to ?page=1 or /page/1/ - that will force a canonical redirect.
• Add and cleanup unit tests

Props obenland, SergeyBiryukov, wonderboymusic.
Fixes #24606.

[kovshenin]

In r28785 we changed the default behavior of paginate_links(). This has the potential to break plugins relying on previous defaults for the base and format arguments. My quick search through the plugins repo didn't reveal any such usage, but it's still something to keep in mind.