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 | 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)
Change History (22)
#2
@
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
@
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
@
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.
#7
@
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)
@
11 years ago
Uses _doing_it_wrong() and throws error in alpha, warning in beta, and notice otherwise
#10
follow-up:
↓ 13
@
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:
↓ 12
@
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
@
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:
↓ 14
@
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:
↓ 15
@
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
@
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:
↓ 17
@
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
@
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.
How about a
doing_it_wrong()
?