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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Kevin Hankens’s picture

Status: Active » Needs review
FileSize
1.61 KB

Hmm, patch didn't get uploaded...

webchick’s picture

Issue tags: +Needs security review

This 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.

markabur’s picture

Hm, 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.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs security review

This is a bug fix, straight up. Erroneously calling something an error does not help security.

markabur’s picture

Status: Reviewed & tested by the community » Needs work

Don'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?

moshe weitzman’s picture

Status: Needs work » Reviewed & tested by the community

it 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.

markabur’s picture

It'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?

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Agreed 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.

David_Rothstein’s picture

Status: Needs work » Needs review

I 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:

  • The first one is not an error because it results from you doing exactly what the Drupal installer required you to do (make settings.php writable). It is just a message reminding you to do something else afterwards .
  • However, if you choose to ignore that message, well, at that point you have committed an error, so it then makes sense for your site to be in an error state.

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.

David_Rothstein’s picture

The 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.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as I agree with David's explanation about why status report can be remain an error.

markabur’s picture

Alright, makes sense to me. Way to resurrect that old issue; the thinking + refinements over there sound good too.

chx’s picture

#6 speaks sense IMO.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, let's do this then. :)

Committed to HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.