Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#35172 closed defect (bug) (wontfix)

WordPress 4.4 pagination not working - Fix for 34060 broke backward compatibility

Reported by: bobbingwide's profile bobbingwide Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Query Keywords: reporter-feedback close
Focuses: Cc:

Description

The fix for #34060 has altered the result set obtained for those routines that were passing an offset value of 0 (zero).

Whilst I agree that WP_Query should not ignore an offset of 0, I also feel that it should not ignore the value for 'paged' either.

In my plugin I had basically the same code as that in get_posts() - which @westonruter commented on; a call to WP_Query::query with the value for offset defaulting to 0.

Current situation

In 4.3.1 pagination only works when offset is 0 or null.
If you set offset to any other value then pagination no longer works.

In 4.4 pagination only works when offset is null.
If you set offset to 0 or any other value then pagination no longer works.

The question here is, what should the behaviour have been?

What is the expected result sets for different values of offset and paged
Particularly... should a 0 value for offset trump a non-zero value of paged?
I think not, hence this TRAC.

Determining the answer to this question will help us decide whether or not another fix should be developed to cater for all those plugins and themes that are still passing a value of 0 for offset.

Workaround

I appreciate that I can easily change my code to eliminate the default value for offset. I will be releasing a quickfix to my referencing #34060 and this new TRAC.

Change History (7)

#1 @boonebgorges
9 years ago

  • Keywords reporter-feedback close added

The central problem in #34060 was that the interaction between 'offset' and 'paged' was inconsistent: For offset=0, paged was taking precedence; for all other values of offset, offset was taking precedence. It seems like the prior design decision here (by "prior" I mean "whenever 'paged' and 'offset' were added to WP_Query) was that offset ought to take precedence, and the fact that it did not do so in the case of 0 was an oversight. [34697] and [35417] were meant to correct this.

Whilst I agree that WP_Query should not ignore an offset of 0, I also feel that it should not ignore the value for 'paged' either.

Should I take away from this comment that (a) you think that it should be the other way around, and paged should always take precedence over offset? Or perhaps (b) that offset and paged should somehow work together? What other way is there for WP_Query to recognize offset=0 while respecting paged at the same time?

Regarding "backward compatibility": Anytime we fix a bug, we are breaking backward compatibility with someone who was exploiting the bug as a feature. It sounds to me like this is a case where you, and possibly others, were exploiting the fact that offset=0 behaved abnormally. In cases like this, there's no perfect solution, but I think on balance it may be better to ask the few who are using the parameter in an odd way to update their usage, so that the API becomes more transparent to new developers. But perhaps I am missing some nuance of how paged and offset could be used together.

#2 @bobbingwide
9 years ago

Hi boone, thanks for your answers.

Should I take away from this comment

Yes, (a) I think it should be the other way round - paged takes precedence
and (b) It would be nice if WordPress could handle the combinations of offset and paged sensibly.

After raising this issue I found a hideous page on the codex that explains the hoops you need to jump through in order to get queries to work with both Offset and Pagination.

https://codex.wordpress.org/Making_Custom_Queries_using_Offset_and_Pagination

If the expected results from the combinations were more clearly documented then it could be more obvious how to use the parameter values.

There being so little information in #34060 it's difficult to know what problem was being addressed.

One point to consider is the fact that, in my experience, for an unpaged query, there didn't seem to be any difference between offset 0 and offset 1.

I would guess that in most cases, we weren't exploiting the bug intentionally, we'd just copied some code from get_posts() and blindly used the default values.
And that leads to the big surprise when backward compatibility is broken.

#3 @bobbingwide
9 years ago

One point to consider is the fact that, in my experience, for an unpaged query, there didn't seem to be any difference between offset 0 and offset 1.

It turns out that my experience varied depending on whether or not I'd specified a value for numberposts. When numberposts was missing then depending on shortcode being used I set numberposts to -1, which causes the offset value to effectively be ignored.

Version 0, edited 9 years ago by bobbingwide (next)

#4 @boonebgorges
9 years ago

@bobbingwide Thanks for the clarifications.

It's not unreasonable to want 'paged' and 'offset' to work together more elegantly. But this turns out to be harder than it seems. See #18897, [26012], [26525] for more details.

Reversing the order of preference so that 'paged' always wins will break backward compatibility for more extensively than what you've reported here. 'offset' has taken precedence since it was introduced in [3867] (!).

I'm not sure how to interpret your comment about 'numberposts'. get_posts() translates numberposts to posts_per_page=-1. If WP_Query sees posts_per_page=-1, it sets nopaging=true. If nopaging=true, then offset is ignored. This has always been the case, so I don't think it's a 4.4 regression.

For these reasons, I'd like to mark this ticket as wontfix. I acknowledge that the 0 behavior changes behavior, but I think it's a relatively easy fix for a very large improvement in the API. To follow future improvements in paged offsets, see #18897.

#5 @boonebgorges
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

#6 @bobbingwide
9 years ago

@boone, thanks for the #18897 link.

Don't take this personally but there seems to be some strange rules regarding backward compatibility and breaking sites.

If the site is broken since it was unintentionally using something which turned out to be a bug then that's fair game. You don't even need to tell the users in advance that the fix might break their sites.

However, if there's a tiny chance that a site is broken because you finally remove deprecated code that had been warning the user that it has been deprecated for 6 or more years, then that's not allowed.

See #33741

:rant off

#7 @boonebgorges
9 years ago

@bobbingwide WP's policies about backward compatibility are constantly evolving. Each case involves weighing the potential benefits vs the potential harm that may be caused by the change. In this case, my judgment was that the potential harm was fairly minor, relegated to what I would consider non-standard use (using an integer 0 to disable a parameter), while the potential gain was significant: the ability to specify a 0 offset, and the regularization of the API.

In cases where potential harm is significant, we do our best to let users and developers know. Browse through my recent posts on make/core to see some of my personal efforts in this area over the last couple of releases: https://make.wordpress.org/core/author/boonebgorges

Note: See TracTickets for help on using tickets.