Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#55104 closed defect (bug) (fixed)

is_main_query() fatal error on null

Reported by: nhadsall's profile nhadsall Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: major Version:
Component: Query Keywords: good-first-bug has-patch 2nd-opinion
Focuses: Cc:

Description

In some cases, get_the_title( $posd_id ) and then plugins hook into the_title calling is_main_query(). In these cases, $wp_query is null. This causes a fatal error. It would be safer to check if $wp_query is set before.

Change History (17)

#1 @costdev
2 years ago

  • Keywords 2nd-opinion added
  • Version trunk deleted

Removing trunk as this was not introduced during the 6.0 cycle.

Adding 2nd-opinion to gather thoughts on this.

#2 @johnbillion
2 years ago

  • Keywords needs-patch good-first-bug added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release

It makes sense to add a guard condition for $wp_query to is_main_query(). All the other is_*() conditionals (is_single(), is_home(), etc) that inspect $wp_query have this guard condition and trigger a call to _doing_it_wrong() when $wp_query isn't set.

This ticket was mentioned in PR #2711 on WordPress/wordpress-develop by vdankbaar.


2 years ago
#3

  • Keywords has-patch added; needs-patch removed

Added guard condition on $wp_query to is_main_query

Trac ticket: https://core.trac.wordpress.org/ticket/55104

This ticket was mentioned in PR #2714 on WordPress/wordpress-develop by vdankbaar.


2 years ago
#4

Fixed inconsistent guard conditions in query.php in line with; https://core.trac.wordpress.org/ticket/55104

Trac ticket: https://core.trac.wordpress.org/ticket/55722

#5 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.1

SergeyBiryukov commented on PR #2711:


2 years ago
#7

Thanks for the PR!

I have updated the $version argument of the _doing_it_wrong() call to 6.1.0, which indicates the next major WordPress version, and added a unit test. Looks ready to merge now 🙂

#8 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 53395:

Query: Check if $wp_query is set in is_main_query().

This avoids a PHP fatal error and triggers a _doing_it_wrong() notice if is_main_query() is called too early, bringing consistency with all the other is_*() conditionals: is_single(), is_home(), etc.

Follow-up to [16947], [17068], [17083], [18699], [37985].

Props vdankbaar, nhadsall, johnbillion, costdev, thijsoo, teunvgisteren, timkersten655, SergeyBiryukov.
Fixes #55104.

#10 @nacin
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This failing hard with a fatal error seems like a benefit, and is something I originally anticipated. It's not something we could have introduced after the fact for many of the 2.x is_* functions, but because is_main_query was introduced much later (3.3), we could get away with doing so. You'd only get a fatal if you call this too early in WordPress load. Given this function has existed in this manner for ~11 years without incident, I'd consider reversion.

#11 @SergeyBiryukov
2 years ago

  • Keywords 2nd-opinion added

Thanks! I see the point, but the issue from the ticket description seems valid and a _doing_it_wrong() message seems like a better developer experience than a fatal error here. Curious to see what others think.

#12 @SergeyBiryukov
2 years ago

In 53396:

Tests: Expand the test for conditional tags returning early if $wp_query is not set.

When called too early, conditional query tags should throw a _doing_it_wrong() notice and return false. This commit verifies that behavior not only for is_main_query(), but for all the other conditional tags too.

Follow-up to [16947], [17068], [17083], [18699], [37985], [53395].

See #55104.

#13 @peterwilsoncc
2 years ago

In [53396] I think test_conditional_tags_trigger_doing_it_wrong_and_return_false_if_wp_query_is_not_set could benefit from a data provider rather than a foreach loop.

This failing hard with a fatal error seems like a benefit, and is something I originally anticipated.

@nacin Your thinking at the time is not documented on #18677, do you recall what it was? This is a somewhat optimistic question as it was 11 years ago.

#14 @SergeyBiryukov
2 years ago

In 53400:

Tests: Use a data provider in the test for conditional tags returning early if $wp_query is not set.

Follow-up to [53395], [53396].

Props peterwilsoncc.
See #55104.

SergeyBiryukov commented on PR #2714:


2 years ago
#15

Hi there, thanks for the PR!

For reference, here is the list of functions affected by this change:

  • have_posts()
  • in_the_loop()
  • rewind_posts()
  • the_post()
  • have_comments()
  • the_comment()

I had some notes when looking at the initial commit:

  • These functions are not exactly conditional tags like the other is_*() functions: is_single(), is_home(), etc. So the same _doing_it_wrong() messages referencing conditional tags are not quite accurate here.
  • Not all of these functions have a return value, e.g. rewind_posts(), the_post(), or the_comment(). Introducing a return value seems a bit out of scope for this change and could use a separate discussion.

If the goal is to avoid a fatal error if $wp_query is not set, I would like to suggest doing just that and returning early. That might not necessarily be consistent with the other is_*() functions, but would be in line with the current return values of these particular functions. A _doing_it_wrong() message could then be considered later as a separate enhancement.

With that in mind, I have made some adjustments to the PR and added a unit test. Any thoughts are welcome 🙂

#17 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

I believe this can be closed with the changes in [53395], [53396], and [53400].

Note: See TracTickets for help on using tickets.