Problem/Motivation

On install.php or update.php the requirement checklist does not show warnings if there are no errors. There are important warnings (like the PHP memory limit warning) that should be visible.

Proposed resolution

Show warning if it is a PHP memory limit warning

Remaining tasks

Consider adding a "reload page" link to the "verify requirements" page when there are only warnings to allow one to attempt to fix the problem and re-test (see #15).

User interface changes

In #951644-15: Requirement warnings (e.g. for PHP memory limit) are not shown on install or update unless there is a requirement error also, during interactive installation, the message text when requirements are not met would change from:
Check the error messages and proceed with the installation.
to:
Check the messages and proceed with the installation.

The same message during update would similarly change from:
Check the error messages and try again.
to:
Check the messages and try again.

#951644-20: Requirement warnings (e.g. for PHP memory limit) are not shown on install or update unless there is a requirement error also has a D7 patch that does not include this string change.

API changes

None.

Original report by @David_Rothstein

This seems to be a bug that was introduced in Drupal 7 due to the use of the beautiful 'status report'-style list for the requirement checking that occurs in install.php and update.php.

In Drupal 6 and earlier, requirement errors and warnings were communicated with drupal_set_message(), with a type of 'error' or 'warning' as appropriate.

But in Drupal 7, we only show the requirement checklist at all if there is at least one error. So if you have warnings but not errors, you will never see them, even if (like the PHP memory limit warning) they are important enough that you probably should see them.

Not sure how easy this will be to fix. It seems like the ideal behavior is that requirement warnings should still result in the list being shown, but unlike with errors, there would need to be a way to bypass the list and continue with the installation or update even though you haven't changed anything on the server.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

avidd’s picture

Priority: Normal » Major

I think this bug should be upgraded to major because it was proposed as a fix to http://drupal.org/node/281405 which was marked "critical". The PHP memory_limit warning is not being displayed, and that can cause installation to fail silently. (it cost me many hours of sleep and almost made me give up on drupal7)

I found several recent forum posts from people experiencing this problem where install.php would halt and hang at a blank page after before the "configuring site" step. I'm using Nginx with fastcgi. My memory_limit for in php.ini (php5-fpm) was set to 32M since my vhost has limited resources. Drupal7's install.php was not only awayre of the 32M available, it was hilighted in green. After talking to a friend who runs an ISP I changed the memory_limit to 128 and set it to spawn only one instance of php5-fpm. That installed successfully.

While I was having this problem I found many recent forum posts from people experiencing the same thing. None of them suggested raising the limit above 32M. A warning here would help a lot of people.

David_Rothstein’s picture

This issue was not proposed as a fix to #281405: Drupal 7.x can't be installed with memory_limit=32M. It was only linked to from there.

If what you're seeing is this:

My memory_limit for in php.ini (php5-fpm) was set to 32M since my vhost has limited resources. Drupal7's install.php was not only awayre of the 32M available, it was hilighted in green.

then it is unrelated to this issue. This issue is only about what happens when you're trying to install Drupal with less than the recommended memory limit (which currently is 32M).

If you have evidence that 32M is not enough for an installation of Drupal core, please follow up at #1008362: 32M is sometimes not enough memory to install Drupal 7 rather than here. Thanks!

Stevel’s picture

I believe the rationale behind this is to make installation as easy as possible. When there are no show-stoppers (e.g. REQUIREMENT_ERRORs) for installing, don't bother the user with it. Non-critical "things to look at" are brought to attention once the installation has completed.

In other words, if the installation can continue, don't show warnings but just continue.

catch’s picture

Right, I think memory limit is the exception here. We can't really set a hard requirement because installs might complete fine on systems where it's lower than recommended - due to APC, a slim PHP build etc, in which case we'd be crying wolf. And whatever requirement we set, there's a chance the install won't complete because someone's PHP install might be really bloated.

So I think just having it as a warning rather than an error makes sense, but it's a good point that it doesn't show up at all if everything else is OK.

One option that would probably avoid breaking string freeze or massive refactoring, would be to revert to the Drupal 6 method of drupal_set_message() right on the initial install screen, only in the case of the memory requirement - this means you'll get nagged twice if you see the requirements report and the message, but that's not necessarily such a bad thing.

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
catch’s picture

Status: Active » Needs review
FileSize
1.21 KB

Likely this is not slick enough, but in the interests of getting this out of the active queue here's a patch.

aschiwi’s picture

The patch in #6 works for me. I had my memory_limit at 16M for the test.

I wonder would it be better to set the message as a warning?

drupal_set_message($description, 'warning');

Attaching a patch for this tiny suggestion because I finally wanna try out patching with git :)

sun’s picture

Status: Needs review » Needs work
+++ b/modules/system/system.install
@@ -202,7 +202,13 @@ function system_requirements($phase) {
+      // The memory limit is not a hard requirement, however it may prevent
+      // installation depending on the install profile. For this reason ensure
+      // that this message is shown during the installation process by both
+      // adding it to the requirements array, and also setting
+      // a message.

I'm missing the key information here for why the additional drupal_set_message() is required.

18 days to next Drupal core point release.

catch’s picture

@sun. Memory limit is just a notice, not a warning. So if you have no other warnings, the initial requirements screen doesn't display at all - we skip that screen if there are no warnings. It's only a notice because it's a complete guess - actual memory requirement can't be predicted and varies by php install.

David_Rothstein’s picture

I think @sun was suggesting that that information should be in the code comments :)

I guess this approach is OK if we can't come up with anything else, but I think it really would be better to fix this for all REQUIREMENT_WARNING messages rather than doing a workaround for this one case.

I also don't understand the argument in #3 that the current behavior is by design. If you are setting a REQUIREMENT_WARNING in your code, then by definition that means you want to warn the user about it! If you don't want your warning to interrupt the installation and only want the user to see the message after installation is complete, then your hook_requirements() implementation should set it during $phase = 'runtime' only, rather than $phase = 'install'.

Maybe I'll take a look and see how easy this would be to do.

catch’s picture

I think if we show the status report thing with warnings or errors that can work - but there'll need to be an extra link or something on there to allow people to continue the installation with remaining warnings. That seems potentially backportable, especially since any string change is before translations are available anyway.

I do think it makes sense to automatically skip it if it's all green, or at least showing it all the time would be a bigger UI change that'd likely need to be D8 only.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
13.5 KB

The attached patch is what I had in mind (basically the same thing @catch described in #11). It has a few rough edges still, but it seems to work. I also wrote a test for the update.php behavior (we can't really test install.php yet).

I think this is definitely the right direction to go for Drupal 8, and so far I don't see anything obvious about it that couldn't be backported to Drupal 7, but that might be more borderline.

Status: Needs review » Needs work

The last submitted patch, requirements-warning-951644-12.patch, failed testing.

David_Rothstein’s picture

The test passes when run on its own, but fails when run as part of the entire suite.

Turns out this is because I added a module called 'update_test' in the patch, but there's already a module with the same name elsewhere in the codebase... oops!

I'll fix the test and clean up a few other things too, sometime soon.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
80.42 KB
71.25 KB
15.38 KB

Here's a new patch:

  • Fixed the tests and also expanded them a bit.
  • Consolidated some duplicate code.
  • Got rid of the @todo for printing REQUIREMENT_WARNING information on non-interactive installs (explained in the code comments why it's not worth the effort).

One thing to point out is that with this patch, regardless of what kind of requirement problem you hit we give you the same link to try to proceed. The attached screenshots show, for install.php, (1) the case where there are only warnings, and (2) where there are also errors. The link in the first case will always bypass the warning and continue, whereas the link in the second case will always reload the page (and check to make sure you actually fixed the problem that led to the error) - i.e., the latter is current Drupal behavior.

One question (at least for Drupal 8) is whether the "only warnings" case should actually have two links, one to skip the warnings and proceed, and a second one to reload the page. (This second link would allow people to e.g. experiment with their PHP memory settings and keep reloading the page in order to see if it worked.) I think this might be useful, but since you can accomplish the same thing by actually reloading the page in your browser, it's probably not critical. And the wording of these links has some usability issues anyway (especially on update.php) which can be addressed in a different issue.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Lovely. In the hope that it will pass, ready to fly.

jct’s picture

Added issue summary

David_Rothstein’s picture

Thanks for the issue summary. However, it's not quite true that there are no user interface changes; the patch does change a string or two. (We could remove that for the D7 version if we have to.)

Also, it doesn't change the API, but it does under some circumstances change the URLs you have to go through during install/update. However, I can't imagine anyone out there actually has scripts that depend on these URLs (since just about everyone should be using something like drush site-install or drush updatedb instead).

David_Rothstein’s picture

Issue summary: View changes

Adding issue summary

xjm’s picture

Added the string changes from the patch to the issue summary. Maybe a D7 version of the patch should not include this string change, since it is a fairly minor/technical one?

xjm’s picture

Issue summary: View changes

Added string changes from the patch.

xjm’s picture

Here's a D7 version, identical except it preserves the word "error."

xjm’s picture

Issue summary: View changes

Clearer formatting.

catch’s picture

Issue tags: -Needs backport to D7
catch’s picture

Issue tags: +Needs backport to D7
catch’s picture

This still looks good to me but I'll try to grab webchick to see if she's happy with this approach for D7 first.

catch’s picture

Assigned: Unassigned » webchick

Had a final pass on the code and it still looks good, but since it's the very first screen of the installer assigning to webchick just to sanity check the workflow.

webchick’s picture

Anyone know how I can trigger this to happen? I set my memory_limit variable down to 20M which should trigger the warning. However, the first screen of the installer still prompts for an installation profile.

catch’s picture

You need to click past install profile/language screens then it'll show up.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Oops. I accidentally committed this with #1289364: SchemaCache generates empty cid and broke testbot. :( Rolled back.

And sorry, guess I took "the very first screen of the installer" a little too literally. :D Here's what it looks like:

Verify requirements check with a warning (not error)

I think this is okay. It would be better if it was "message" instead of "error message", but I agree it's not really worth breaking strings for this.

We'll need a re-roll here for D7 that at least changes the version = line to 7.x instead of 8.x for the test module's .info file. Then we'll see how testbot likes it.

catch’s picture

Assigned: webchick » Unassigned
Status: Needs work » Needs review
FileSize
15.38 KB

Re-uploading David's patch from #15. Also de-assigning webchick now that she's reviewed this - I can commit to D8 assuming it applies and passes tests now :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

The actual requirement warning text about the memory limit reads a bit wordy/lengthy, but I just recognized it's not even part of this patch -- was probably introduced, but never really seen or reviewed, as it didn't always appear. Would be worth to create a separate issue for slimming down that message.

Because of that, this patch looks good to go for me.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
15.38 KB

I think we just need a D7 re-roll. I've committed this to 8.x

Here's the D7 re-roll with version specific stuff find/replaced. There was an 8.x in there and update_foo_8000() to switch.

catch’s picture

Wrong patch..

catch’s picture

#31: requirements-warning-951644.patch queued for re-testing.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Since this was just a re-roll for 7.x I'm marking it RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Let's try that again. :)

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

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

Anonymous’s picture

Issue summary: View changes

Added links to patches.