query_posts / WP_query parameter 'offset' should play nicely with 'paged'

[here]

Setting the offset parameter with WP_query ignores the paged parameter. Thus offset can only be used as if on page 1. This is unfortunate and seems worth fixing.

See prior #2558 "query_posts() should support offset"

[coffee2code]

Judging by the inline comments, ignoring the 'paged' parameter when 'offset' is set was intentional. It's been that way for 6 years seemingly without anyone complaining, and this ticket has sat without comment or patch for 7 months.

That said, I don't know why a query with an offset can't also be paged. Sure, you can figure it all out manually and set the 'offset' appropriately, but it seems like the kind of thing WP_Query should just handle (barring some justification for the mutual exclusion).

Attached is 18897.diff​ which implements the functionality. Should be no impact, assuming no one is using both 'offset' and 'paged' and expecting 'paged' to be ignored. I just added unit tests (see ​[UT708] ) that include a test marked as a known bug due to this ticket and which passes when the patch is applied.

[SergeyBiryukov]

Related: [3867]

[wonderboymusic]

Patch still applies cleanly, moving to 3.7 for discussion

[nacin]

Could benefit from some unit tests.

[coffee2code]

More than was already added in ​[UT708] as mentioned in my previous comment (2)? ;)

[wonderboymusic]

In 26012:

Respect paged when offset is used in WP_Query. Adds unit tests.

Props coffee2code.
Fixes #18897.

[bradyvercher]

As helpful as this is, it seems like a pretty major change at this point that's bound to cause all sorts of problems with code that took the previous behavior into account. I have a handful of projects in the wild that I know of that will break and just have to wait until I hear about issues with the others.

Considering this hasn't had much discussion, I'd like to open it back up for reconsideration. Is a breaking change here really worth the minor benefit it provides?

[here]

In my opinion, having a query_posts() function that is consistent with the expected WP_Query Parameters is worth it going forward.

The only "breaking" change would be code that uses both paged and offset, which is *currently* broken, due to this bug. Further, anyone working with query_posts today with be confounded by this unexpected and undocumented bug. As far as I know, this is the only example of WP_Query Parameters not working with query_posts(). This still seems worth fixing.

Since query_posts() silently ignores paged if offset is used -- I wonder how you would have code that would break. If you had previously coded something to work around this, you would have had to choose one or the other, not both. As such, most workarounds will continue to work after this patch. Can you give a more specific example of where you expect this patch to break something in your own code ?

Surely there are a number of folks who have used paged and offset together, and never noticed that paged was being ignored. This code might break -- but such code is *already* acting in a way inconsistent with expected function action (paged is ignored). Again, seems worth fixing for past and future.

​http://codex.wordpress.org/Class_Reference/WP_Query#Parameters

[bradyvercher]

The linked docs and code both had a comment about the behavior, so it's pretty well documented and has been consistent for 6 years. The Codex also has another article that details how to account for paged requests and how to fix pagination when using offset, which this patch doesn't consider, and is likely to break as well.

For a quick example of code that would break, just try the snippet there: ​http://codex.wordpress.org/Making_Custom_Queries_using_Offset_and_Pagination

Making this change also forces theme and plugin developers to implement functionality to account for both behaviors if they need to maintain backwards compatibility.

I agree it would have been nice if this were available earlier, but at this point, it has potential to cause a lot of issues. A new arg, something like paged_offset, might be pretty cool, though.

[wonderboymusic]

show me your code that will break, please

[here]

@wonderboymusic , the code linked by @bradyvercher in ​http://codex.wordpress.org/Making_Custom_Queries_using_Offset_and_Pagination#Offset_.26_Manual_Pagination would break if a user were using both paged and offset . After this patch, I believe that workaround code would cause the offset to be applied twice.

That article was added in March 2013.

The offered explanation of a "good reason" for the current behvior is : "There is a very good reason for this however... the offset argument is actually the value WordPress uses to handle pagination itself. If a developer sets a manual offset value, pagination will not function because that value will override WordPress's automatic adjustment of the offset for a given page."

As I don't feel well versed in the project's philosophy regarding this type of breaking change, I'll defer on whether this patch is appropriate for inclusion.

It is quite clear by the wiki article that the workaround is non-obvious and non-trivial for most users. A new arg seems like an unfortunate compromise , but may be the best possible solution .

[bradyvercher]

Like @here mentioned, that snippet in the Codex article will break. Or here's a basic example:

function ticket18897_pre_get_posts( $q ) {
	if ( is_admin() || ! $q->is_main_query() ) {
		return;
	}

	// Example settings.
	$offset = 2;
	$posts_per_page = 5;
	$page = $q->is_paged ? $q->get( 'paged' ) : 1;
	$q->set( 'posts_per_page', $posts_per_page );

	// Manually calculate the offset.
	$paged_offset = ( $page - 1 ) * $posts_per_page + $offset;

	// This value is $pgstart with old behavior.
	// This value is added to $pgstrt with the new behavior.
	$q->set( 'offset', $paged_offset );

	// When $page = 2
	// Old: $pgstart = 7
	// New: $pgstart = 12
}
add_action( 'pre_get_posts', 'ticket18897_pre_get_posts' );

Another common scenario that's likely to break is when displaying a different number of results on the first page than subsequent paged requests.

[wonderboymusic]

The new behavior is the correct behavior. 2nd page of results, offset by 5. Both values should be respected for WP_Query to properly act as an abstraction layer. Having to do pre_get_posts previously was a hack. Someone should update the Codex.

[bradyvercher]

I understand the point about the abstraction layer and offered up the paged_offset alternative, but more importantly, developers also depend on the API being stable. Making a breaking change like this undermines confidence in the API. It's not just the Codex that needs updating, but code in plugins, themes, tutorials, blog posts, support forums, and countless sites. Do a search on GitHub, Google, WordPress Answers, the WordPress.org support forums, or the plugin repository -- all of that code will break and the benefits of this change are minor.

Using the other example I mentioned above, say I want 5 posts on the first page of an archive and 10 on subsequent pages -- that's no longer possible without allowing offset to be negative (attached patch allows for that). In any case, these are just a couple of the uses I've come up with, who knows what other scenarios rely on the old behavior.

Offset is a special arg that breaks things like paginated links and if it's going to be used, then developers should understand the ramifications and they should be clearly documented.

[bradyvercher]

Allow negative offsets.

[SergeyBiryukov]

In 26479:

Restore @ticket reference. see #18897.

[SergeyBiryukov]

Replying to bradyvercher:

Using the other example I mentioned above, say I want 5 posts on the first page of an archive and 10 on subsequent pages -- that's no longer possible without allowing offset to be negative (attached patch allows for that).

That would indeed require a negative offset, but it wasn't possible before this change either, unless I'm missing something. This code works the same way for me in both 3.7.1 and current trunk (posts 6 to 10 are never displayed):

function change_posts_per_page_18897( $query ) {
	if ( is_admin() || ! $query->is_main_query() )
		return;

	if ( $query->is_paged() ) {
		$query->set( 'posts_per_page', 10 );
	} else {
		$query->set( 'posts_per_page', 5 );
	}
}
add_action( 'pre_get_posts', 'change_posts_per_page_18897' );

[bradyvercher]

You're not using offset in that code, which is why the behavior isn't different. Previously, it was possible by manually calculating the offset based on the current page. The second page of results with the snippet below would show results 5-15. Now it will show results 15-25.

function change_posts_per_page_18897( $query ) {
	if ( is_admin() || ! $query->is_main_query() )
		return;

	if ( $query->is_paged() ) {
		$query->set( 'posts_per_page', 10 );
		$query->set( 'offset', ( $query->get( 'paged' ) - 1 ) * 10 - 5 );
	} else {
		$query->set( 'posts_per_page', 5 );
	}
}
add_action( 'pre_get_posts', 'change_posts_per_page_18897' );

[SergeyBiryukov]

Replying to bradyvercher:

The second page of results with the snippet below would show results 5-15. Now it will show results 15-25.

Confirmed.

[nacin]

In 26525:

Revert [26012] for now, which had fixed using paged with offset in WP_Query but introduced side effects for those working around it.

see #18897.

[nacin]

I like this fix but question if we may be breaking quite a bit of hacky workarounds. I don't know if the potential for breakage actually outweighs the change, but it's clear that the original ticket/patch/commit failed to consider the possibility of breakage, so a revert is in order at this point in the cycle.

I'm glad we got this in early in the cycle (four weeks ago), as it gave us enough time to pull it back out if necessary.

Open to ideas.

[coffee2code]

Are we saying that because WP has broken behavior X and people have workarounds for X (to be expected), that we can't fix X because people hacked around it and now their hack may be duplicative?

The hacks mentioned above seem to be for the purpose of directly addressing the errant behavior of WP, which to me is a different ball of wax than if they are trying to make use of errant behavior in some unexpected way (still risky) or just casual usage not realizing there was any underlying errant behavior. And this isn't the use of advertised behavior of the interface/API.

Fixing errant behavior in core in your own code should come with the reasonable expectation that the behavior would (and even should) eventually get fixed and that your fix may no longer be needed (or may in fact cause issues at that point). There are ways developers could try to mitigate this sort of situation.

We don't really know how many (if many) are affected. Maybe this should soak longer in core? The negative side effect of the patched code in conjunction with hacked code is likely just posts being skipped on pagination under certain use cases, so sites shouldn't be fatally breaking if affected.

[SergeyBiryukov]

Reintroducing [26012] along with 18897.2.diff​ early in the cycle and updating Codex to reflect the new behaviour would make sense to me.

[MikeHansenMe]

see #30284

[coffee2code]

Update of 18897.diff with negative offset support and more unit tests. Does not take 18897-unittests.diff into account since the unit test being re-added in that patch hasn't been removed from core tests yet.

[coffee2code]

Updated 18897.diff​ with 18897.3.diff​ in order to add support for negative offsets as suggested by @bradyvercher.

The previously proposed 18897.2.diff​ was not a sufficient change to add support for negative offsets as it broke the basic functionality of 'offset' (test_query_offset() failed).

18897.3.diff also adds the following unit tests:

• test_query_negative_offset() : Test use of negative offset by itself (since no paging is involved, it should have no effect in this situation)
• test_query_negative_offset_and_paged(): Test use of negative offset in conjunction with paging.
• test_query_large_negative_offset_and_paged(): Test use of large enough negative offset that would try to reach before first possible post.

With negative offset support, it would be possible to have defined a front-page posts_per_page of, say, 5 (to show posts 1-5). Then on subsequent pages have posts_per_page be 10 with an offset of -5 (to yield posts 6-15 on page 2, 16-25 on page 3, etc).

That should satisfy @bradyvercher's functional request. We still have the notion brought up that we shouldn't fix a bug because people hacked around it. As I argued earlier, this is clearly a bug that developers knowingly worked around so they should be prepared for that bug to get fixed.

The original patch soaked in trunk for 4 weeks before getting reverted. Maybe the latest can get in on 4.2-early?

(Note: this .3.diff patch does not account for 18897-unittests.diff​ since the unit test re-added in that patch hasn’t been removed from core unit tests yet. Should that unit test get remove as per #30284 at some point, then both 18897.3.diff and 18897-unittests.diff will need to be applied unless a fresh patch that does so is added.]

[bradyvercher]

I disagree with the characterization that the existing behavior is clearly a bug. In the majority of APIs I've used with an offset parameter, it's been a zero-based implementation, so every time I used it in WP_Query, I considered that the intended behavior and worked with it, not around anything I thought was a bug. I'm sure most developers working with it were under the same impression, which is why it took 6 years for anyone to bring it up.

In any case, I haven't looked into how WP_Comment_Query and WP_User_Query handle it, but would those need to be updated as well if they exhibit the same behavior?

[jadpm]

Has this ticket lost track?

I would really love to see some consistency on this, to be honest. I will not enter into the discussion on whether the thing is broken or not, and whether we should work around it or not, but I feel it is somehow obviously wrong that we can pass a paged parameter to WP_Query different than 1 and get the posts corresponding to the first page.

Even the linkd Codex page states, after explaining a series of nice filters messing with the query, that "And now you're done. Pagination should now work correctly along with your own, custom offset.". So after that, pagination should work correctly with offset. In my book, it means that you need to apply a fix to make it work, so it is broken.

[wonderboymusic]

needs a fresh look