WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#16746 closed defect (bug) (fixed)

post paginating <!--nextpage--> does not work

Reported by: hakre Owned by: nacin
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:

Description

When <!--nextpage--> is placed at the very beginning of the post content (first page contains zero-length content) then pagination does not work at all.

Example post content for reproduction:

<!--nextpage-->
page2
<!--nextpage-->
page3

This will result in a single page, not in a three or two page setup.

It's related to strpos returning 0 which is casted into false when if'ed.

Related: #16745

Attachments (2)

16746.patch (512 bytes) - added by hakre 11 years ago.
Fix
16746.2.patch (942 bytes) - added by SergeyBiryukov 9 years ago.

Download all attachments as: .zip

Change History (17)

@hakre
11 years ago

Fix

#1 @nacin
11 years ago

  • Keywords has-patch commit added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to nacin
  • Status changed from new to accepted

#2 @nacin
10 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

Going to close this as wontfix. I can see someone using <!--nextpage--> at the start, and having it currently work, but then suddenly having an empty first page. If you want to have an empty first page, use a space or empty line or something.

#3 follow-up: @toscho
9 years ago

  • Cc info@… added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

May I ask to implement Hakre’s patch? The patch is really trivial, and I can see no side effects.

For a user on the other hand, the result is not intuitive. There is also no hint available for the user how she can fix it.

#4 @SergeyBiryukov
9 years ago

  • Component changed from General to Query
  • Milestone set to Awaiting Review

#5 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
9 years ago

Replying to toscho:

May I ask to implement Hakre’s patch? The patch is really trivial, and I can see no side effects.

One side effect is mentioned in comment:2. I agree that the current result is not intuitive though.

#6 in reply to: ↑ 5 @toscho
9 years ago

Replying to SergeyBiryukov:

One side effect is mentioned in comment:2. I agree that the current result is not intuitive though.

But that is not true: if <!--nextpage--> is at the first position, paging does not work. You get the content of all pages on one page, because the comparison …

if ( strpos( $content, '<!--nextpage-->' ) )

… is not strict enough. There is even a big red warning on the manual for that behavior:

This function may return Boolean FALSE, but may also return a non-Boolean value which evaluates to FALSE. Please read the section on Booleans for more information. Use the === operator for testing the return value of this function.

#7 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 3.6

#8 @nacin
9 years ago

  • Keywords needs-patch added; has-patch commit removed

I did understand the initial report here, and based on my original understanding of the surrounding code, I did not think this patch should be applied. Basically, if someone had <!--nextpage--> at the start of a post, and then later in the post, they'd currently have two pages. With this patch, they would have three — including an empty first page. This is not expected and is fairly lame. While a "page break" usually means to immediately start a new page, <!--nextpage--> just as much marks the start of a page, and if a user was using it to mark the start of page 1 (and it worked) then they should not be punished. I also assumed, knowing the age of this code, that this check existed since b2.

However, in this case, I was wrong. While this code is from b2, b2 used a preg_match that *did* obey <!--nextpage--> at the start of a string. It was changed to strpos() in [8754], for performance but it also changed the functionality.

Additionally, this is check is to decide whether there is pagination at all. If the post starts with <!--nextpage-->, strpos() will return 0, and no other <!--nextpage--> will be listened to, and the post will not be paginated. That is clearly incorrect behavior.

While failing to explicitly and strictly check the return value of strpos() is a code smell, it helps when bug reports point out why something smells, as well as any repercussions. In this case, I had felt that the implicit > 0 (needle in haystack, and needle not at start of haystack) was correct.

So — I think this would be best with pieces of both the current and old behaviors: Ignore <!--nextpage--> at the start of the string (so, no empty page), but don't let it prevent pagination.

#9 @nacin
9 years ago

In 1242/tests:

Move tests/query.php to tests/query/conditionals.php. New query test for setup_postdata() pagination with nextpage. see #16746.

#10 @SergeyBiryukov
9 years ago

  • Keywords has-patch added; needs-patch removed

#11 follow-up: @toscho
9 years ago

Just an idea: maybe we can remove leading <!--nextpage--> string including surrounding whitespace on save_posts?

#12 in reply to: ↑ 11 @SergeyBiryukov
9 years ago

Replying to toscho:

Just an idea: maybe we can remove leading <!--nextpage--> string including surrounding whitespace on save_posts?

We don't modify content on save_post for any other reason, so I guess we shouldn't do that in this case either.

#13 @nacin
8 years ago

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

In 24599:

Avoid getting tripped up on post content that starts with <!--nextpage-->. props SergeyBiryukov. fixes #16746.

#14 @nacin
8 years ago

In 1307/tests:

Update post content parsing tests for [24598] and add new tests for [24599]. see #16746.

#15 @nacin
8 years ago

In 1323/tests:

Simple test for wp_count_posts(). props johnpbloch. see #16746.

Note: See TracTickets for help on using tickets.