Fix double appearance of the settings.php message in the installer

The text "All necessary changes to [directory] and [settings.php] have been made" means the user did something right and while it is important they change the file permissions the red implies something was incorrect. I think it's possible a user won't visit /admin for awhile though and that it does make sense they change the permissions of the settings.php file while they are still in "installation time".

What about having two messages, or would that be too much? One message informs the user of the installation success and the other is an error message informing them that the configuration file is not protected?

The text should be rewritten, to be action focused. Not - past, action, recommendation (as it is now).

I don't feel strongly about going to yellow, indeed not a very friendly error - but it does set the stage for Drupal in the future. Because then yellow would imply critical as well as red?

I can understand that some users might be thrown off by a message in red, but the color does convey the importance of the task.

Attached patched adds a regular, status message for "All necessary changes to %dir and %file have been made" and an error message for "In order to avoid security risks you should now remove write permissions to %file...". Please see screenshot.

What about this? I can't apply the patch here, to see how it looks.

"Remove write permissions to %file. In order to avoid security risks.", doesn't seem right as two sentences. How about, "Remove write permissions to %file in order to avoid security risks." ?

We are really dealing with two changes here. The first is taking the "All necessary changes to %dir and %file have been made" message and making it a normal status message. This, I am fully in agreement with. It's a positive action and should not be an "error".

The second aspect is whether or not the "Remove write permissions to %file in order to avoid security risks. If you are unsure how to do so, please consult the online handbook." should be an error or a warning. While I completely agree with coltrane in that red conveys the importance of the task, I do not really like the fact that you cannot install drupal without receiving an error, but given the importance of this message, I still feel a big red error message is correct.

With all that said, I think this is good to go.

I'm a bit torn now, the statement "you cannot install drupal without receiving an error" is valid and sound. It's the nature of what the message is, it's not an error in this case. Semantically a warning is more correct. Should red be used to elevate the severity of the task? Red is consistent with the status report page if the configuration file is not protected though.

So we all agree. Lets commit this.

So, the way Drupal 7 behaves today is actually different than the way it behaved a week ago, and different than Drupal 6. Previously, it only showed this message after the progress bar was finished installing (i.e., it showed it on the site configuration form). It did not show it at all on the "Drupal installation complete" page.

Currently in Drupal 7, it is repeating the message on both pages. The change is my fault - it was a side effect of the patch committed at #524728: Refactor install.php to allow Drupal to be installed from the command line. I had intended to eventually file it as part of a batch of followups after that giant issue is completely finished, but you people are fast :)

Anyway, I'm setting this back to "needs review" for two reasons:

1. We should think about going back to the way things were -- it might be much less intrusive if the message only shows once, and not on the final summary screen. Unfortunately, the installer currently has to do a couple tricky things with the form API in order to work correctly, so I don't know if this will be easy. But there's got to be some kind of solution.
2. If we want this new green status message, then I think we should really try to prevent repeating it. Repeating an error message twice might sort of be considered a feature, but I'm not sure they need to be told twice that all necessary changes to these particular parts of the filesystem have been made... do they really care? If that's the behavior that's being forced, let's call this a bug and fix it :)

Let's stop for a second and think: Which are the ideal screen(s) for these messages to be showing on, assuming we had complete freedom to put them anywhere?

karschp in favor of getting this stuff committed. Lets comeup with screeenshots and patches. I think we are on the right track, it just needs to happen.

OK, I spent some time thinking about this.

First, I think the patch in #16 is definitely an improvement and would support it going in: Getting rid of the green "here are the technical details of why your site is working OK" message is great :) Getting rid of the security warning is more controversial, but not that bad; as mentioned, we're still warning them about it later, and honestly, there are more likely ways for a Drupal site to be hacked into, and we don't warn people about those at all.

However, does this really solve the usability problem, or just defer it? They're still going to have their "OMG, I just installed Drupal and now I have a big red message!" moment; it will just happen a bit later (first time they visit the admin screen).

So here is an idea for how we might be able to get rid of that experience altogether:

1. First, and this is key, there are actually some reasons why showing them this message in the installer might make things easier on them. People see this message when Drupal can't automatically change settings.php permissions on their particular system, which means that in order to even get to this point in the installation, they've almost certainly already had to follow some instructions and figure out how to deal with this. In fact, they probably still have their terminal window or FTP program open, and they've already done 95% of the work; all we're doing now is asking them to undo the thing they just did 30 seconds ago, which shouldn't be that hard. If they wait until later to do it, it becomes harder.
2. If we were to keep this message in the installer, I don't think this should be framed as an error message or even a warning message at all; it is really more of a task. By far the most likely reason their site is now configured insecurely is that we forced them to make it that way earlier in the installation process, so yelling at them about it now is actually pretty rude :) Furthermore, making it an error or warning conflicts with other visual cues on the screen (e.g., the column on the left with the big row of green checkboxes that say everything is OK).
3. Even if the message were changed to green, it still totally gets in the way of everything else on the screen. In the attached screenshot (from Drupal 6), the point of the page is that they are supposed to be configuring their site information, but we also are showing them a totally random message that tells them to do something else. Regardless of what color it is, the message is misplaced, even in Drupal 6.
4. Conclusion: I think this message deserves its own screen in the installation process. The disadvantage of this is that it adds an extra click, but the advantage is that it shows them the task at hand and allows them to focus on it and move on when they're ready. So basically, the proposed new page would show as early as possible (I think immediately after the "set up database" step?) and contain some very simple information like the following:
   • An explanation of the file permissions they need to change.
   • Something like: "For security reasons, we recommend that you change them now. If you wait until after the installation, you'll receive a reminder about it in the administration section of your site."
   • Click to continue.

   Obviously, this page would not show at all for the people who don't need to see it; if Drupal can fix things things automatically, it would act the same as in @karschsp's patch above (i.e., show nothing).

Any thoughts on that idea? Pros or cons that I'm missing?

@David long write up. Conclusion is separate page for this error?

I think we should leave the error as a message, where it is now. The succes messages should be removed. But the error message, should not be pushed to the actual site. I want people to work with a completely green Drupal, starting off - its an important feeling to touch.

I would concur more, to an error message that is well written. I think that will solve this issue more, then a separate page or moving it to the actual site.

Its an error, lets not discount it - its important to change.

David_Rothstein, interesting post. I think I agree. Making it a separate page is an intriguing idea. I like that it because it no longer shows as a "big red error", but it maintains its importance. It really is a task for the person installing. I am envisioning two submit buttons "continue" (which would check to verify that write perms have been disabled, and present the user with an error if not) and a "skip" button (wordage can be debated) which would simply bypass the message and send them into their install. The error message would then be visible within the admin area as a reminder.

I dont really like #21 because there's something about seeing an error and warning free screen that says "drupal installation complete".

I really do like the idea of having it on a separate page, nice thinking David_Rothstein.

I moved away from this issue for a while because people were working on #418302: Copy default.settings.php to settings.php during install IFF webserver owns files (FTP on shared hosting), which looked like it was going to necessarily involve making some of the same broader UI changes I discussed above and in the process make this all moot :) Instead, though, the patch that got committed to Drupal 7 there was a more limited one, and the rest moved to #913796: Use authorize.inc to securely copy settings.php during the installer for Drupal 8.

So, we still have something to fix here. It's too late in the cycle to be adding pages to the installer at this point, so we'll have to go with one of the more limited fixes discussed above. Here is a patch that hopefully combines the best of all the above suggestions:

• Changes the error to a warning. Note that #938470: Shouldn't a successful install call drupal_set_message() with $type = 'warning' instead of 'error'? was filed recently and does that too (but nothing else); if it gets committed first, we can cross it off the list here :)
• Rewords the warning message it to be shorter and more actionable (although I think this might need a bit more thought - it feels like it's missing a bit of the "why this is happening" explanation that the previous wording has).
• Fixes the bug where the message displayed on two screens in a row; instead, it only displays on the configuration page (like Drupal 6).
• In the case where Drupal owns the files and can change the permissions itself (and does not need the user to do anything), removes the previous "the computer was able to write file XYZ for you so everything is OK" message altogether because it's totally unnecessary, as per #14 and other comments above.

Yes, I like that its more actionable - I wouldn't recommend adding much more on the "why" as it might be hard to capture in a few words.

That looks good. I think either "to" or "from" is grammatically OK (and "to" was already in the existing wording). But I agree "from" is easier to understand, and as long as we are changing this string, we might as well change that too. In that case, we should change the similar usage in INSTALL.txt also (attached patch).

Hopefully this one is ready to go :)

Rerolled because #938470: Shouldn't a successful install call drupal_set_message() with $type = 'warning' instead of 'error'? was just committed a few minutes ago :)

The current version of this patch changes strings. Tagging accordingly.

+++ includes/install.core.inc	15 Oct 2010 03:33:28 -0000
@@ -1460,11 +1460,10 @@ function install_configure_form($form, &
+  // Check that $_POST is empty so we only show this message when the form is
+  // first displayed, not on the next screen after it is submitted.
+  if (empty($_POST) && (!drupal_verify_install_file($settings_file, FILE_EXIST|FILE_READABLE|FILE_NOT_WRITABLE) || !drupal_verify_install_file($settings_dir, FILE_NOT_WRITABLE, 'dir'))) {

Can we extend the comments to explain 'why' we want this behavior? Thanks!

OK, I tried to give some context why the behavior is particularly bad in this case.

(Though really, this is just a general problem you hit anytime you try to set a message in a form builder function. Who would ever want the same message to display on two pages in a row?)

#34: install-error-messages-534556-34.patch queued for re-testing.

The change to install.core.inc needed a pretty simple reroll. The change to INSTALL.txt was also out-of-date, but no longer necessary since that file has been totally rewritten in the meantime.

Since this is just a re-roll of an RTBC patch, marking as RTBC. I looked at it, and the code was identical, afaik.

Hm. I'm not comfortable committing anything to install.core.inc this close to RC.

So let's get a re-roll without the "make it friendlier" part. That's polish anyway, and we're past that point now.

Looks like that should be easy enough to do.

#42: install-error-messages-534556-42.patch queued for re-testing.

(Those test failures look somewhat unlikely.)

Trivial change, and the patch still works as intended, so I'm setting this back to RTBC.

Committed to HEAD. Thanks!