WordPress.org

Make WordPress Core

#22873 closed defect (bug) (fixed)

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

Reported by: 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 16 months ago.
Patch to tell you you're _doing_it_wrong
22873.diff (1.8 KB) - added by aaroncampbell 16 months ago.
Uses _doing_it_wrong() and throws error in alpha, warning in beta, and notice otherwise

Download all attachments as: .zip

Change History (22)

comment:1 johnbillion16 months ago

How about a doing_it_wrong()?

comment:2 toscho16 months 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.

comment:3 wonderboymusic16 months 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.

comment:4 knutsp16 months 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.

comment:5 aaroncampbell16 months ago

+1 for wontfix

comment:6 ocean9016 months ago

+1 for wontfix

The user should see how bad plugins can be.

comment:7 georgestephanis16 months 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 16 months ago by georgestephanis (previous) (diff)

johnbillion16 months ago

Patch to tell you you're _doing_it_wrong

comment:8 johnbillion16 months ago

  • Keywords has-patch added

comment:9 nacin16 months ago

  • Keywords close removed

Please do not close this ticket, thanks.

aaroncampbell16 months ago

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

comment:10 follow-up: aaroncampbell16 months 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.

comment:11 follow-up: dd3216 months 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..

comment:12 in reply to: ↑ 11 wpmuguru16 months 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.

comment:13 in reply to: ↑ 10 ; follow-up: duck_16 months 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.

comment:14 in reply to: ↑ 13 ; follow-up: aaroncampbell16 months 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?

comment:15 in reply to: ↑ 14 DigiproveDevelopment16 months 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?

comment:16 follow-up: knutsp16 months 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.

comment:17 in reply to: ↑ 16 DigiproveDevelopment16 months 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.

comment:18 nacin16 months 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.

comment:19 nacin16 months ago

In 23216:

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

see #22873.

comment:20 nacin16 months 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.