WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#14729 closed defect (bug) (fixed)

Fatal errors after [15531] when conditional tags are used too early

Reported by: zamoose Owned by: nacin
Milestone: 3.1 Priority: normal
Severity: blocker Version:
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Upon activating BuddyPress, the entire site dies with the following error:

Fatal error: Call to a member function is_page() on a non-object in [file path]/wp-includes/query.php on line 371

Prior to a recent SVN rev (I'm not sure which), BuddyPress worked on 3.1 alpha.

Attachments (1)

14729.diff (7.0 KB) - added by nacin 3 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 follow-up: nacin4 years ago

  • Keywords dev-feedback added; buddypress removed
  • Milestone changed from Awaiting Review to 3.1
  • Summary changed from Latest trunk breaks BuddyPress to Fatal errors after [15531] when conditional tags are used too early

The changes introduced in [15531] will generate fatal errors in plugins that have been incorrectly using the $wp_query object before it is properly initialized.

I like the idea of leaving in fatal errors as then plugin developers know something is wrong -- we did so for register_taxonomy() with rewrite = true before init, although it wasn't a conscious decision to do so -- but because this change affects conditional tags, it'll likely break a good number of plugins and maybe themes.

I have an interesting idea for this -- we leave this as is until late in the dev cycle. Hopefully that fixes up a good number of plugins including BuddyPress. And then we add sanity checks before release. Perhaps crazy, but at least it'll fix up a lot of code along the way.

comment:2 follow-up: zamoose4 years ago

I've created a corresponding ticket for BP over here: http://trac.buddypress.org/ticket/2609

We'll see if that gets any traction.

As a side note: is there a way to turn on annotation in Trac's source view so that I can see when files were committed/whom they were committed by?

comment:3 in reply to: ↑ 2 ; follow-up: nacin4 years ago

Replying to zamoose:

As a side note: is there a way to turn on annotation in Trac's source view so that I can see when files were committed/whom they were committed by?

Look for the annotate link in the top right corner.

comment:4 johnjamesjacoby4 years ago

Thanks for making a ticket over on bp.org. Will try to sneak a fix into the next bug-fix release.

comment:5 in reply to: ↑ 3 zamoose4 years ago

Replying to nacin:

Replying to zamoose:

As a side note: is there a way to turn on annotation in Trac's source view so that I can see when files were committed/whom they were committed by?

Look for the annotate link in the top right corner.

Thanks. Dunno why I missed that the first time.

comment:6 in reply to: ↑ 1 demetris4 years ago

Replying to nacin:

I have an interesting idea for this -- we leave this as is until late in the dev cycle. Hopefully that fixes up a good number of plugins including BuddyPress. And then we add sanity checks before release. Perhaps crazy, but at least it'll fix up a lot of code along the way.

Another idea would be to make a post on wpdevel, from now, tagged 31compat and going something like:

Please check your plugins/themes with WP_DEBUG on WordPress 3.0. If you get “Trying to get property of non-object” notices in WP 3.0, that means that your plugin/theme will cause FATAL ERRORS in WP 3.1.

Why this happens: Blah blah.

How to fix: Blah blah.

comment:9 zamoose4 years ago

Anyone mind schooling me, as someone not all that familiar with the intricacies of query.php, why r15531 caused this behavior to go from "warning" to "Fatal error"? I'm parsing through the diff and I think it's due to the change in the types that the various is_*() functions return. Is that correct?

comment:10 follow-up: nacin4 years ago

If $wp_query is not set, then accessing a nonexistent property ($wp_query->is_page) triggers a notice (maybe a warning, don't remember). But accessing a nonexistent method ($wp_query->is_page()) is a fatal error.

comment:11 in reply to: ↑ 10 zamoose4 years ago

Replying to nacin:

If $wp_query is not set, then accessing a nonexistent property ($wp_query->is_page) triggers a notice (maybe a warning, don't remember). But accessing a nonexistent method ($wp_query->is_page()) is a fatal error.

Ahh! Precisely the open lead I needed connected! Many thanks.

comment:12 follow-up: markjaquith3 years ago

I upgraded my site to trunk and got WSOD on the front, and an is_feed fatal error on the backend that gave no clue as to which plugin might be causing it. We should check is_object( $wp_query ) before calling a method on it, rather than issue a fatal error.

comment:13 in reply to: ↑ 12 ; follow-up: nacin3 years ago

  • Severity changed from critical to blocker

Replying to markjaquith:

I upgraded my site to trunk and got WSOD on the front, and an is_feed fatal error on the backend that gave no clue as to which plugin might be causing it. We should check is_object( $wp_query ) before calling a method on it, rather than issue a fatal error.

http://core.trac.wordpress.org/ticket/14729#comment:1

Suggesting we leave this for deep into RC, with the hope that many plugins will be able to correct themselves during the beta/RC phases. Marking for blocker so we don't forget the important part :-)

comment:14 westi3 years ago

  • Cc westi added

comment:16 azizur3 years ago

  • Cc azizur added

comment:17 in reply to: ↑ 13 avbentem3 years ago

Replying to nacin:

Suggesting we leave this for deep into RC, with the hope that many plugins will be able to correct themselves during the beta/RC phases.

Now that the beta is out, you might want to reconsider that? I cannot install BuddyPress, which kind of implies I simply cannot test the beta. This is NOT a problem for me, but it might apply to others too and hence you might get less test results than expected?

comment:18 avbentem3 years ago

  • Cc avbentem added

comment:19 nacin3 years ago

Tempted to finally introduce a _deprecated_message() for this use case and for some of the more 'innovative' uses of _deprecated_argument().

Or maybe doing_it_wrong( $message ) :-)

comment:20 westi3 years ago

I think that a generic _doing_it_wrong() type call is best for these cases where we detect a plugin misbehaving.

comment:21 filosofo3 years ago

Since we get PHP 5 in 3.2, we could throw custom exceptions for these kinds of things.

comment:22 nacin3 years ago

(In [16947]) You are doing it wrong in query.php. see #15824, see #14729.

nacin3 years ago

comment:23 nacin3 years ago

  • Keywords has-patch added

Patch here for when we're ready to do this.

comment:24 demetris3 years ago

Re: 14729.diff

Maybe we could also include a note/reminder to remove the isset() checks at some point in the future; say, when we start developing v3.3.

comment:25 scribu3 years ago

That would only make sense if by v3.3 it would somehow be less likely that developers would make that mistake, which I doubt.

comment:26 nacin3 years ago

Don't think we should do that. This is going to break a number of themes and plugins. Unfortunately an E_NOTICE is not enough to believe that we can remove anything, whether it be this or a nooped deprecated function. I agree with scribu.

As a language construct, isset() is crazy fast. That's not something to be concerned with.

comment:27 nacin3 years ago

  • Owner set to nacin
  • Status changed from new to accepted

comment:28 nacin3 years ago

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

(In [17068]) Don't fatal error when conditional tags are called before init. fixes #14729.

comment:29 nacin3 years ago

(In [17083]) Clarify the _doing_it_wrong message for conditioanl tags. init prevents the fatal error, but the query still isn't run. Just drop the mention of a hook and we end up with better reasoning why this doesn't work to boot. see #14729.

Note: See TracTickets for help on using tickets.