Opened 13 years ago
Last modified 6 years ago
#18897 reopened enhancement
query_posts / WP_query parameter 'offset' should play nicely with 'paged'
Reported by: | here | Owned by: | wonderboymusic |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.2.1 |
Component: | Query | Keywords: | has-patch needs-codex |
Focuses: | Cc: |
Description
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"
Attachments (4)
Change History (30)
#1
@
13 years ago
- Component changed from General to Query
- Keywords has-patch added
- Milestone changed from Awaiting Review to Future Release
- Version set to 3.2.1
#3
@
11 years ago
- Milestone changed from Future Release to 3.7
Patch still applies cleanly, moving to 3.7 for discussion
#4
@
11 years ago
- Keywords commit needs-unit-tests added
- Milestone changed from 3.7 to 3.8
- Type changed from defect (bug) to enhancement
Could benefit from some unit tests.
#6
@
11 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from new to closed
In 26012:
#7
@
11 years ago
- Cc brady@… added
- Resolution fixed deleted
- Status changed from closed to reopened
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?
#8
@
11 years ago
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
#9
@
11 years ago
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.
#11
@
11 years ago
@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 .
#12
@
11 years ago
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.
#13
@
11 years ago
- Keywords needs-codex added
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.
#14
follow-up:
↓ 16
@
11 years ago
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.
#16
in reply to:
↑ 14
@
11 years ago
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' );
#17
follow-up:
↓ 18
@
11 years ago
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' );
#18
in reply to:
↑ 17
@
11 years ago
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.
#20
@
11 years ago
- Milestone changed from 3.8 to Future Release
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.
#21
@
11 years ago
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.
#22
@
11 years ago
Reintroducing [26012] along with 18897.2.diff early in the cycle and updating Codex to reflect the new behaviour would make sense to me.
@
10 years ago
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.
#23
@
10 years ago
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.]
#24
@
10 years ago
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?
#25
@
10 years ago
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.
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.