WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#24606 closed defect (bug) (fixed)

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

Reported by: Nessworthy Owned by: SergeyBiryukov
Milestone: 4.0 Priority: normal
Severity: normal Version: 2.1
Component: General Keywords: has-patch has-unit-tests commit
Focuses: template Cc:

Description

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.

Attachments (6)

24606.diff (1.4 KB) - added by obenland 6 years ago.
24606.2.diff (2.9 KB) - added by obenland 5 years ago.
24606.3.diff (3.6 KB) - added by obenland 5 years ago.
24606.4.diff (1.5 KB) - added by obenland 5 years ago.
24606.5.diff (6.6 KB) - added by SergeyBiryukov 5 years ago.
24606.6.diff (7.9 KB) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (28)

@obenland
6 years ago

#1 @obenland
6 years ago

  • Component changed from General to Template
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.9
  • Version changed from 3.5.1 to 2.1

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.

#2 @nacin
6 years ago

  • Component changed from Template to General
  • Focuses template added

#3 @mikeschroder
5 years ago

  • Keywords needs-unit-tests added

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.

#4 @obenland
5 years ago

Working on UT for this…

@obenland
5 years ago

#5 @obenland
5 years ago

  • Keywords has-unit-test added; needs-unit-tests removed

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

This ticket was mentioned in IRC in #wordpress-dev by obenland. View the logs.


5 years ago

@obenland
5 years ago

#7 @obenland
5 years ago

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

#8 @nacin
5 years ago

  • Keywords has-unit-tests commit 4.0-early added; has-unit-test removed
  • Milestone changed from 3.9 to Future Release

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.

#9 @obenland
5 years ago

  • Milestone changed from Future Release to 4.0

#10 @SergeyBiryukov
5 years ago

#28334 was marked as a duplicate.

#11 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 28671:

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

props obenland, Nessworthy.
fixes #24606.

#12 @wonderboymusic
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


5 years ago

@obenland
5 years ago

#14 @obenland
5 years ago

  • Keywords 4.0-early removed

24606.4.diff to fix existing UTs.

#15 @SergeyBiryukov
5 years ago

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

#16 @SergeyBiryukov
5 years ago

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.

#17 follow-up: @SergeyBiryukov
5 years ago

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.

#18 @SergeyBiryukov
5 years ago

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

#19 @ocean90
5 years ago

  • Keywords commit removed

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

#20 in reply to: ↑ 17 @SergeyBiryukov
5 years ago

  • Keywords commit added

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.

#21 @wonderboymusic
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#22 @kovshenin
5 years ago

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.

Note: See TracTickets for help on using tickets.