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.
Comment | File | Size | Author |
---|---|---|---|
#31 | requirements-warning-951644.patch | 16.41 KB | catch |
#30 | requirements-warning-951644-15.patch | 15.38 KB | catch |
#28 | requirements-warning-951644-15.patch | 15.38 KB | catch |
#20 | requirements-warning-951644-20-d7.patch | 16.41 KB | xjm |
#15 | requirements-warning-951644-15.patch | 15.38 KB | David_Rothstein |
Comments
Comment #1
avidd CreditAttribution: avidd commentedI 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.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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:
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!
Comment #3
Stevel CreditAttribution: Stevel commentedI 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.
Comment #4
catchRight, 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.
Comment #5
sunComment #6
catchLikely this is not slick enough, but in the interests of getting this out of the active queue here's a patch.
Comment #7
aschiwi CreditAttribution: aschiwi commentedThe 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?
Attaching a patch for this tiny suggestion because I finally wanna try out patching with git :)
Comment #8
sunI'm missing the key information here for why the additional drupal_set_message() is required.
18 days to next Drupal core point release.
Comment #9
catch@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.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #11
catchI 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.
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a new patch:
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.
Comment #16
sunLovely. In the hope that it will pass, ready to fly.
Comment #17
jct CreditAttribution: jct commentedAdded issue summary
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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
ordrush updatedb
instead).Comment #18.0
David_Rothstein CreditAttribution: David_Rothstein commentedAdding issue summary
Comment #19
xjmAdded 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?
Comment #19.0
xjmAdded string changes from the patch.
Comment #20
xjmHere's a D7 version, identical except it preserves the word "error."
Comment #20.0
xjmClearer formatting.
Comment #21
catch#15: requirements-warning-951644-15.patch queued for re-testing.
Comment #22
catch#15: requirements-warning-951644-15.patch queued for re-testing.
Comment #23
catchThis still looks good to me but I'll try to grab webchick to see if she's happy with this approach for D7 first.
Comment #24
catchHad 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.
Comment #25
webchickAnyone 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.
Comment #26
catchYou need to click past install profile/language screens then it'll show up.
Comment #27
webchickOops. 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:
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.
Comment #28
catchRe-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 :)
Comment #29
sunThe 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.
Comment #30
catchI 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.
Comment #31
catchWrong patch..
Comment #32
catch#31: requirements-warning-951644.patch queued for re-testing.
Comment #33
catchSince this was just a re-roll for 7.x I'm marking it RTBC.
Comment #34
webchickCommitted and pushed to 7.x. Let's try that again. :)
Comment #35.0
(not verified) CreditAttribution: commentedAdded links to patches.