Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
After installing D7 a lot over the past few months, it occurred to me that printing a successful installation message as an error might be semantically incorrect. It seems to me that calling drupal_set_message() with $type = 'warning' would be more appropriate here. Seeing a red "x" after installation seems like something went wrong, despite there being no actual "error" messages.
I'm sorry if this was covered in D7UX, or if there were issues elsewhere around this (I couldn't find anything on d.o).
Comment | File | Size | Author |
---|---|---|---|
#7 | settings-warning-message.png | 36.23 KB | markabur |
#7 | settings-error-message.png | 30.43 KB | markabur |
#1 | install_core_settings_warning.patch | 1.61 KB | Kevin Hankens |
Comments
Comment #1
Kevin Hankens CreditAttribution: Kevin Hankens commentedHmm, patch didn't get uploaded...
Comment #2
webchickThis would be nice, since it would stop Drush from spitting out SCARY RED ERRORS when doing a drush si.
I'm not sure if the security team would get behind reducing the severity of this error though. You can get your site pwned if this file remains writable.
Comment #3
markabur CreditAttribution: markabur commentedHm, it's not like, "Whoa, an error happened." -- but it is like "Stop now and do this thing." To me the yellow level is more of a suggestion, like the recommended speed for an offramp. This message is telling you something that you really really ought to do immediately, so the error level of message seems more appropriate to me.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedThis is a bug fix, straight up. Erroneously calling something an error does not help security.
Comment #5
markabur CreditAttribution: markabur commentedDon't forget that the end user knows nothing about what we call it internally, all they see is the message box. Semantics has nothing to do with it. The real question is whether or not its (visual) severity level affects the user's behavior (is it feasible that seeing the stop sign icon might help get permissions set properly more often?)
What's the reasoning for using the "warning" level of message, as this is clearly not a warning?
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedit is precisely a warning. The user is *warned* that her site is not yet completely secure. The user has not performed an error, nor has any error occurred. The system is in its expected state.
Comment #7
markabur CreditAttribution: markabur commentedIt's a success message plus a warning, I guess. If we're going to call it a warning then shouldn't we change the status report as well? It is currently a REQUIREMENT_ERROR if the configuration file is not protected. Seems odd to have the message escalate in severity from one screen to another.
On the available updates screen we use "error" for security-related messages and "warning" for normal ones. Does the decision here affect that?
Comment #8
webchickAgreed that both places should be consistent if we're going to change the severity. I would still like to see more people on the sec. team chime in here, too.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedI think this is a good change. Don't print an error when the user didn't do anything wrong. (And security-wise, on most servers at least we're talking about a security hardening type of thing here, not an actual security hole that can be exploited by itself.)
I don't think I agree with the idea that the severity needs to be the same in both places. The context is totally different:
Hm, I also think there is an older issue in the 'install system' queue that is similar to this one (though it was a somewhat more broad issue, so it's fine to keep them separate). I'll see if I can dig that one up sometime tomorrow; I think there was lots of relevant discussion there about warning vs error.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedThe other issue I was thinking about is #534556: Fix double appearance of the settings.php message in the installer.
Actually, looking at how that issue started, this one really is a direct duplicate of it. However, the other one broadened a bit as it went on, so it's fine to keep them separate.
I just rerolled the patch there now. But in the meantime, I personally think the patch here is good to go as it is.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedBack to RTBC as I agree with David's explanation about why status report can be remain an error.
Comment #12
markabur CreditAttribution: markabur commentedAlright, makes sense to me. Way to resurrect that old issue; the thinking + refinements over there sound good too.
Comment #13
chx CreditAttribution: chx commented#6 speaks sense IMO.
Comment #14
webchickOk, let's do this then. :)
Committed to HEAD.