Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#22873 closed defect (bug) (fixed)

Consider moving to a notice for $wpdb->prepare in 3.5.1

Reported by: nacin's profile nacin Owned by:
Milestone: 3.5.1 Priority: low
Severity: minor Version: 3.5
Component: Warnings/Notices Keywords: has-patch
Focuses: Cc:

Description

This is either a 3.5.1 fix, or a wontfix.

See #22262 and http://make.wordpress.org/core/2012/12/12/php-warning-missing-argument-2-for-wpdb-prepare/. Then see my comment here:

We probably could have had it generate a notice in 3.5 and a warning in 3.6, but I was incredibly torn by the idea of shipping a notice that most developers wouldn't even see (let alone give themselves a chance to ignore it) when this is, at its heart, a potential security issue. Issuing a warning seemed like the most responsible thing to do, despite the (relatively minor) pain it'll cause.

A side-note, we've gotten very good (I say this facetiously) about accidentally breaking plugins that were doing something wrong in a major release, only to fix the issue in the next minor release after all of the plugins have updated for it. Happened with JavaScript enqueueing hooks in both 3.2 and 3.4. This was indeed a deliberate change, but there's nothing preventing us from moving to a notice in 3.5.1, then back to a warning in 3.6 again (hopefully giving developers some cover to make changes).

Attachments (2)

22873.patch (772 bytes) - added by johnbillion 11 years ago.
Patch to tell you you're _doing_it_wrong
22873.diff (1.8 KB) - added by aaroncampbell 11 years ago.
Uses _doing_it_wrong() and throws error in alpha, warning in beta, and notice otherwise

Download all attachments as: .zip

Change History (22)

#1 @johnbillion
11 years ago

How about a doing_it_wrong()?

#2 @toscho
11 years ago

  • Cc info@… added

The big advantage of the warning is users are forced to make a decision. I think it is better to run the site without an insecure plugin or theme for a while than keeping the security issue hidden. doing_it_wrong() and notices are not visible for the average user, they do not help. And there are probably even more issues in plugins and themes with such code.

There was enough time for developers to test their code. Let’s keep it as it is now, this might be a useful lesson.

#3 @wonderboymusic
11 years ago

I vote for wontfix - if you aren't developing with your Apache error_log in plain view, this is probably not the only warning you are generating. If you are using $wpdb directly against Barry's wishes and aren't also looking at your logs, you deserve the warnings you are getting.

#4 @knutsp
11 years ago

  • Keywords close added

A PHP Notice is just a hint to make the code better an more robust. A PHP Warning is a - warning. Something might be wrong or dangerous. It tells the developer: You are not knowing what you are doing here! And that is what this is all about.

Suggest wontfix.

#5 @aaroncampbell
11 years ago

+1 for wontfix

#6 @ocean90
11 years ago

+1 for wontfix

The user should see how bad plugins can be.

#7 @georgestephanis
11 years ago

+1 for doing_it_wrong()

The point's been made, and the end is nigh, but let's give the users a reprieve in the mean time.

(with, of course, bringing back the warning in 3.6)

Last edited 11 years ago by georgestephanis (previous) (diff)

@johnbillion
11 years ago

Patch to tell you you're _doing_it_wrong

#8 @johnbillion
11 years ago

  • Keywords has-patch added

#9 @nacin
11 years ago

  • Keywords close removed

Please do not close this ticket, thanks.

@aaroncampbell
11 years ago

Uses _doing_it_wrong() and throws error in alpha, warning in beta, and notice otherwise

#10 follow-up: @aaroncampbell
11 years ago

22873.diff is based off what we talked about in dev chat. Basically it switches to using _doing_it_wrong(), but modifies _doing_it_wrong() to throw an error when on alpha, a warning when on beta, and a notice otherwise.

#11 follow-up: @dd32
11 years ago

+1 for 22873.diff from me, I like throwing different levels of warnings based on the environment of the user.

I have to say however, I was surprised to see that we display Warnings by default when WP_DEBUG is not enabled, I was under the impression that we didn't for some reason..

#12 in reply to: ↑ 11 @wpmuguru
11 years ago

Replying to dd32:

+1 for 22873.diff from me, I like throwing different levels of warnings based on the environment of the user.

I have to say however, I was surprised to see that we display Warnings by default when WP_DEBUG is not enabled, I was under the impression that we didn't for some reason..

Some hosts are turning warnings on in the php.ini in which case in most instances they are always on.

#13 in reply to: ↑ 10 ; follow-up: @duck_
11 years ago

Replying to aaroncampbell:

22873.diff is based off what we talked about in dev chat. Basically it switches to using _doing_it_wrong(), but modifies _doing_it_wrong() to throw an error when on alpha, a warning when on beta, and a notice otherwise.

I think we want to be warning during RC too. Also, we discussed including wp-includes/version.php instead of relying on the global which can be overridden.

#14 in reply to: ↑ 13 ; follow-up: @aaroncampbell
11 years ago

Replying to duck_:

I think we want to be warning during RC too. Also, we discussed including wp-includes/version.php instead of relying on the global which can be overridden.

Honestly it seems like RC is supposed to be a time when we encourage users (not just developers) to test, saying that it's safe to run. As much as I'd love to throw warnings to devs even in RC (I'm still not opposed to just leaving it how it is), I'm not sure that we should.

As for version, is it common enough for people to change the version during the dev cycle that we need to explicitly include the file?

#15 in reply to: ↑ 14 @DigiproveDevelopment
11 years ago

Replying to aaroncampbell:

Honestly it seems like RC is supposed to be a time when we encourage users (not just developers) to test, saying that it's safe to run. As much as I'd love to throw warnings to devs even in RC (I'm still not opposed to just leaving it how it is), I'm not sure that we should.

You are spot-on with this - I suspect the vast majority of users are not developers and hitting them with warning errors that might break their site but certainly look ugly is going to undermine their trust in Wordpress.

I like 22873.diff​ but if it is decided to leave the warning in may I suggest that the "Headers already sent" consequence of warning messages is also addressed so that sites don't break?

#16 follow-up: @knutsp
11 years ago

  • Cc knut@… added

If warnings (or errors or notices) are displayed on a production site, then the server setup is wrong and possibly vulnerable. If you have some plugins the chance of getting some warnings is quite high, but end users seldom sees them, so these extra warnings doesn't change anything. It's not about "hitting" users with warnings, as the warnings will go to the error_log or displayed in the debug bar (plugin), where any developer should look.

I develop plugins and child themes, and this new warning have already caught me doing things wrong (thanks, core team!). Don't forget that this particular warning is added because the reason for it may imply possible vulnerabilities in plugins or themes.

I'm still and firmly for wontfix (always a warning), at least in all development versions.

#17 in reply to: ↑ 16 @DigiproveDevelopment
11 years ago

Replying to knutsp:

If warnings (or errors or notices) are displayed on a production site, then the server setup is wrong and possibly vulnerable. If you have some plugins the chance of getting some warnings is quite high, but end users seldom sees them, so these extra warnings doesn't change anything. It's not about "hitting" users with warnings, as the warnings will go to the error_log or displayed in the debug bar (plugin), where any developer should look.

It's a matter of opinion whether production sites should not be displaying warnings. However as plugin authors we are not in control of this, and the fact is there are very many sites that do display warnings.

#18 @nacin
11 years ago

In 23215:

In 3.5.1, have $wpdb->prepare() issue a notice for an insufficient number of arguments, instead of a warning. see #22873.

#19 @nacin
11 years ago

In 23216:

Remove accidental sprintf(), which also requires at least two arguments. :-)

see #22873.

#20 @nacin
11 years ago

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

This now produces a notice in 3.5.1.

For 3.6, see #23062.

Note: See TracTickets for help on using tickets.