#47133 closed defect (bug) (wontfix)
Post's global variables not available on content_pagination filter
Reported by: | david.binda | Owned by: | |
---|---|---|---|
Milestone: | Priority: | high | |
Severity: | normal | Version: | 5.2 |
Component: | Posts, Post Types | Keywords: | 2nd-opinion |
Focuses: | Cc: |
Description
This is related to the same issue flagged in #47114 .
After r44941 the post's global variables are being defined after the content_pagination
filter fires due to the same reasoning as in #47114.
I personally haven't experienced any issues, but I was able to find a code on GitHub which would break due to the issue.
A textPager plugin, available on GitHub (last update was 3 years ago, tho), hooks `content_pagination` filter and on it's callback, it attempts to read the `global $page` variable, which has not been properly set yet (after r44941).
I'm not sure how often such a logic relying on global variables being set is used (the example I linked is the only one I've been able to find), so I'm not sure on the severity here.
Change History (5)
#3
follow-up:
↓ 5
@
6 years ago
- Keywords 2nd-opinion added
Thanks, @davidbinda.
I don't see an obvious fix for this. If we moved the 'content_pagination' filter into WP_Query::setup_postdata()
(as with 'the_post'
in [45285]), then the values of $numpages
, $more
, and $multipage
that are set in generate_postdata()
https://github.com/WordPress/WordPress/blob/e464f03549f0142a82a636beb2f4f9f4793ba760/wp-includes/class-wp-query.php#L4301 would be generated *before* the filter is applied. So you could end up with a $pages
global that doesn't match what was used to create these other globals.
Based on the late date and the following facts, I'd like to suggest that we do not make any further changes, and accept this minor compatibility break:
- The fact that this appears to be an extreme edge case
- The fact that it will be hard to come up with a fix that doesn't violate the spirit of [44941] (namely, by hoisting values into the global scope inside of the
generate_postmeta()
method) - The fact that the
'content_pagination'
filter receives the$post
variable as a parameter, which is, for reasons aside from this ticket, the more reliable way to determine the post context during'content_pagination'
callbacks (and, as such, is a "fix" or at least a workaround for anything that may break)
If it helps to mitigate, I (maybe with the assistance of @davidbinda ?) could write a dev-note.
#4
@
6 years ago
I know this maybe an unpopular view, but I don’t think we should fix this. Using global like that is unsafe at best. I think we should add filter to the end of generate post data method and push people to use that going forward.
Generate post data as a function, as part of the original patch, is a way of push people not to use those globals going forward.
#5
in reply to:
↑ 3
@
6 years ago
- Milestone 5.2 deleted
- Resolution set to wontfix
- Status changed from new to closed
Replying to boonebgorges:
I don't see an obvious fix for this.
I don't see one either. As noted, this filter is intended for (direct) manipulation of the splitting of post_content into separate pages and runs "in the middle" of setting the globals (i.e. it was not designed to make these globals usable). In that terms I'm also thinking this is a wontfix
.
If it helps to mitigate, I (maybe with the assistance of @davidbinda ?) could write a dev-note.
Yep, this is a great idea! A (short) dev-note can also include some info about the fixes in #42814, etc. and would be quite beneficial.
Replying to spacedmonkey:
I think we should add filter to the end of generate post data method and push people to use that going forward.
Yes, was thinking it would be good to have a filter there that would replace all needs to use the globals. Lets do that in 5.3 :)
Moving to 5.2 to evaluate if action is required before release tomorrow.
CC: @boonebgorges, @azaozz, @spacedmonkey.