Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#47133 closed defect (bug) (wontfix)

Post's global variables not available on content_pagination filter

Reported by: davidbinda's profile 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)

#1 @desrosj
6 years ago

  • Milestone changed from Awaiting Review to 5.2
  • Priority changed from normal to high

Moving to 5.2 to evaluate if action is required before release tomorrow.

CC: @boonebgorges, @azaozz, @spacedmonkey.

#2 @desrosj
6 years ago

  • Component changed from General to Posts, Post Types

#3 follow-up: @boonebgorges
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:

  1. The fact that this appears to be an extreme edge case
  2. 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)
  3. 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 @spacedmonkey
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 @azaozz
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 :)

Last edited 6 years ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.