Update requires PHP memory limit warning if below recommended minimum

I've had this happen by not having enough memory allocated to PHP.

How much memory do you have allocated?

-Myke

Please try Beta5 resp. RC1, as soon as it's available. The problem should be solved soon in #197720.
If you need to update ASAP, you might want to try HEAD.

Not duplicate yet.

Updates between beta versions are not actually supported, so even if there would not be a white screen, you might end up with a bad database.

Refining the title, and explaining the essence of the issue here. (If this is a critical bug for install, should this be a critical issue for update?)

http://drupal.org/node/197720 provides a patch for a recommended minimum PHP memory limit and a warning. During the install process a warning will be given when the limit is set lower than the recommended minimum.

A similar check needs to be implemented for the update process against the recommended minimum memory. This is to avoid a white page during update (aka White Screen Of Death = WSOD), or the cryptic PHP error when there isn't enough memory to run update or Drupal.

Sorry for setting this to duplicate. Thought it would be covered in #197720 as well. As this doesn't seem to be the case, I would agree vjordan that this is critical as well.
Aside from a PHP memory limit warning we should check all possibilities to make update.php work with < 8MB memory limit.

Removing 4.7 updates would cut out a chunk of code to be loaded - as it would for install http://drupal.org/node/197722

@catch, even with the patch http://drupal.org/node/197722#comment-663744, I still reach 9M memory before starting the update from 5.x to 6.x

scor: out of interest - how much different is it unpatched?

not much actually, only about 500KB.

That seems like quite a big saving to me considering it's free, but thanks!

this patch checks at the very beginning of the update script the PHP memory_limit through the hook_requirements of system.module, and therefore reused part of the work which has been done in the related issue http://drupal.org/node/197720. see screenshot on a 8M config - it's a requirement at the moment, we need to decide whether it should just be a warning.
my very first attempt to tackle this - so it definitely needs reviews + advice/ideas on how to improve the patch.

I think the behaviour should be the same as for install - it's a warning there and should be a warning here rather than a requirement. As we know, some configurations will get away with updating on less than 16M for update; I've upgraded with 15M, for instance. So let's allow those people who are willing to take a chance do just that.

Yeah this should definitely be a warning rather than a hard requirement, so CNW.

scor: The patch in #14 applies against head and does what it claims, so you're on the right track. But as noted it needs to be coded as a recommendation.

Furthermore http://drupal.org/requirements needs to be updated as per http://drupal.org/node/197720#comment-650279 Would it be helpful to open another issue on the documentation change?

vjordan, yes go ahead with the documentation change issue, and include links to the 2 related issues (this one and 197720).

Quick cross post against Gabor's most recent comment on #197720 since that also affects this issue.

It would probably be a good thing to have an RTBC patch waiting here with the warning anyway though I guess - given time constraints.

Note that the recent changes to run update functions of all disable but previously installed modules also makes update require more memory (but it helps keep the database consistent). So we did not make the situation any better recently.

Looking at the latest patch, t(), st(), $t() and friends should not be used in the update process, so you should remove any code which is there to initialize the environment for them to run and the calls to them as well. Use literal English strings in the update process.

sorry for the delay. I had to work around the latest changes made to update.php.

The full bootstrap is loaded right away on the first page displayed by update.php, which causes an out of memory fatal error on a real site (2000 nodes) (8M). Therefore we need to have a dedicated maintenance page displaying the update warning, with a minimum memory load: DRUPAL_BOOTSTRAP_CONFIGURATION, which takes less than 4MB on my machine.

I also fixed include_once './includes/bootstrap.inc'; which should be require_once to be consistent with install.php and index.php.

OK, "parsing" the patch:

- update_check_requirements() serves as an abstraction, so we can set messages as needed when requirement problems are detected
- install.inc defines drupal_requirements_severity(), so we need it there
- good that we have comment on why we do this minimal bootstrap on its own, sounds reasonable
- we cannot do much better but a static form for the continue stuff (unless we have a link instead...) this comes down to usability here...
- install_goto() also comes from install.inc and despite its name, it is reasonably used here (to prevent caching)
- theme_update_page() updated to display the messages
- thanks to cleanups in the system.install code, a tiny chunk of update time checking there

Hm, all in all, this is repetition of the same patterns from the installer, but applied for the update code. In several cases, such as in update_check_requirements(), we do similar stuff as in the runtime or the installer checks. I did not look into this, but I wonder whether we can reuse anything from there.

Except the possibility for code reuse, my human based parser says this looks very reasonable and fitting into what we are doing elsewhere. Let's validate or invalidate the code reuse possibility!

as Gábor emphasized, I reused many functions from the installer, or mimic them for update.php when necessary (such as update_check_requirements).

- we cannot do much better but a static form for the continue stuff (unless we have a link instead...) this comes down to usability here...

I used a button to remain consistent with the rest of the update process which uses similar buttons. There is a point in favor of a normal link, when you have an access denied, and then try to reload the page, you have to confirm (due to the POST data).

In several cases, such as in update_check_requirements(), we do similar stuff as in the runtime or the installer checks. I did not look into this, but I wonder whether we can reuse anything from there.

I already reused some code from the installer: update_check_requirements() is a simpler version of install_check_requirements() which specifically checks for the requirements of the system module only (I also took off the install profile dependency).

the installer - typically system_requirements() with $t = get_t(); - requires the localization to be set up, and spits PHP Notices otherwise. So I added drupal_init_language() during the update requirements check phase only.

OK for some reason I can't get the warning to come up. I've hacked system module to make the requirement 100M - this worked for the installer, but not here.

catch: were you redirected to update.php?op=info right away, or did you get an empty maintenance page screen?

update.php?op=info straight off. Note I did this on an existing D6 install with no valid database updates to run - but this ought to fire before getting to that point anyway right?

the warning message is displayed on update.php
you get redirected to update.php?op=info if there are no warnings (i.e. no requirement problem)

I can't reproduce this. How much PHP memory limit do you have set on your server?
can you add var_dump($requirements); at the end of update_check_requirements() to see what comes out of the system module check?

edit: yes, this should also fire on a fresh Drupal 6 install with no updates to be made.

Well that's weird, I refreshed my install for another patch, tried this one again and it works fine now, lack of sleep I guess.

I'm not sure why we have to do this when we have no t or $t usage introduced by the patch though:

the installer - typically system_requirements() with $t = get_t(); - requires the localization to be set up, and spits PHP Notices otherwise. So I added drupal_init_language() during the update requirements check phase only.

I removed drupal_init_language() from update.php, didn't get any notices in the requirements check.

without drupal_init_language(), I get 12x:
PHP Notice: Trying to get property of non-object in /Applications/MAMP/htdocs/dhead/includes/common.inc on line 757
and 2x:
PHP Notice: Trying to get property of non-object in /Applications/MAMP/htdocs/dhead/themes/garland/maintenance-page.tpl.php on line 16

When system_requirements() is executed, the $requirements table is populated with the various warning or status messages of each requirements (Apache version, PHP version etc....) and this is where the $t is used. I don't see an easy way to work around this without refactoring system.install, which is out of the question at this stage.
Note that it's only during the requirements check that the localization is used and not during the rest of the update.

OK well I can't reproduce the notices, but that's not an argument to take it out. I was wondering if we should allow contrib modules to show warnings on this page as we did in install (say a move to requiring php5) - but contrib modules have to be upgraded and installed before users get to update.php anyway, so any damage is already done. Could do with another set of eyes on it but seems fine to me.

For clarification: I of course mean the PHP notice of php_error.log
catch: maybe your error_reporting is set to E_ALL ^ E_NOTICE?

Patch applied to head tonight. But I can't get the warning message to appear. Tried running with memory_limit = 15M and the update proceeds fine - no sign of the expected PHP Memory Limit warning.

Also tried with memory_limit = 14, 12, 10, 8 and in all cases update failed with the PHP Memory error we're trying to avoid.

So now I'm curious to know what other patch catch applied in #30 - could this be somehow helping this change?

vjordan, I think it's unlikely that it was one of the other patches, was nothing connected to this iirc. But I can't remember what exactly I had running. Very odd.

Just confirmed no message with the lastest -dev tarball.

Patch testing in IRC:

[4:34pm] goba: scor: ok, got the problem
[4:34pm] scor: goba: great
[4:34pm] goba: scor: there is a drupal_requirements_severity() call in update_check_requirements()
[4:34pm] goba: which identifies the highest severity issue with the system
[4:34pm] scor: yup
[4:34pm] goba: you only set messages if ($severity == REQUIREMENT_WARNING)
[4:35pm] goba: so if there is a higher problem, warning messages are not set
[4:35pm] goba: in my case, the higher problem is
[4:35pm] goba: ["file system"]=> array(3) { ["value"]=> string(12) "Not writable" ["severity"]=> int(2) ["title"]=> string(11) "File system"

That'll be testing like this then in my case:

tar -zxvf drupal-6.x-dev.tar.gz
mv drupal-6.x-dev drupal6
cp d6settings.php sites/default/settings.php

When I made the files directory and chmodded, there's my warning. Well spotted!

file system is writeable in my situation. Status report shows "Cron maintenance tasks" as not run recently and "Update notifications" as not enabled. I guess these too are at higher severity levels than REQUIREMENT_WARNING

this is what I have done so far (patch still needs work and proofreading). working with a non writable files folder.
Again it follows the installer, the errors are blocking (must be resolved) and the warnings are just non blocking notifications.
Note that anybody could skip these tests by going to update.php?op=info right away, but we can assume than users updating their site know what they are doing and will browse to update.php first.

This code duplication stuff does not look right. Why not

$levels = array(REQUIREMENTS_ERROR => 'error', REQUIREMENTS_WARNING => 'warning');

and then foreach through this and break out after messages are logged for the first match.

For consideration this is #25 with if (!empty($severity)) - I'm sure we particularly need to show installer warnings on update.php - they appear at runtime anyway (and install). Works with various severity levels and none at all I think.

A period at the end of a comment found by keith smith via irc, and bonus one found by me when I fixed that one.

Through some IRC discussions, we realized that we only load system module anyway (to avoid going over the memory limit), so we should only be concerned by the errors and warnings output by it. From IRC discussion:

[7:47pm] goba: what if the new updated system requires a certain database version or PHP version?
[7:47pm] goba: that would come as an error right?
[7:47pm] goba: not warning
[7:48pm] goba: it sounds logical not to start the update then, but report the error
[7:48pm] goba: these are checked by system module

So with this, I go back to my code duplication note in #41.

!=REQUIREMENT_OK due to discussion with goba on irc.

OK to summarise conversations on IRC. Since php and mysql requirements are also handled by system.module we should show them if possible. #40 does this, but it also stops an update if your file system isn't writable without helpful feedback.

Just looked and it appears this warning is for sites/default, rather than sites/default/files - so better to suppress it/ignore completely than a new warning message.

Apologies for some discontinuity in the discussion here...

I tested #40 with scor this evening (we're almost neighbours and got to sit in the same room). It worked as documented. But as we tested various use cases and error conditions, such as non writeable files directory, and a does not exist files directory, we came across a further issue which would be useful to fix here. I reckon this is closely related to #47.

The issue needs some further exploration - scor is going to have a look and revert here. FWIW: If I understand it correctly, the files path is defaulting to sites/default/files irrespective of the site's settings. It's possible additional bootstrapping will locate the files directory path correctly so the appropriate file system error shows up when it should.

vjordan: I think we should simply suppress the file system error here. It's not necessary for upgrade, and it's in both install and runtime error checking now.

catch: a module update which touches the file system may not be able to locate the files it needs to touch.

I'm not sure if this is a real case (e.g. maybe an image module regenerates thumbnails on update) or a theoretical one. What I am sure about is I'm at (or beyond) the limit of my expertise here and might contribute more by staying quiet!

let's make it simple. in this patch, the various requirements messages from the system.module appear as warning messages on update.php, whether they are REQUIREMENT_WARNING or REQUIREMENT_ERROR. That way, we don't block the update if say the 'files' folder is missing, but we still warn the user.

I tested this with no files directory setup, got The directory sites/default/files does not exist, a continue button, all well and good.

Then I set up "files" as my file directory, and got the same message. I still think the file system warning should be suppressed completely, it's quite confusing as it stands. For example, everyone upgrading from 5.x with the standard file system settings will get this message. Imagine the support requests^TM.

OK so how about this? Just makes file system REQUIREMENT_OK in $phase == 'update'. Otherwise identical to scor's patch.

catch: people upgrading from 5.x should not get that file error message, since they have a different config setting already saved into the database IMHO (not tested yet).

Gabor: for me, with #52, if I set up my files directory as /files both as writable and in admin/settings/file-system, I'm getting The directory sites/default/files does not exist. on update.php

catch: are you updating from 5 to 6 or from 6 to 6?

scor: 6 - 6.

I'm getting the same behaviour as catch in #56 on a 5->6 update.

this behavior is normal as we haven't done a full bootstrap yet during the requirements check. To retrieve the current files directory, file_directory_path() uses variable_get('file_directory_path', conf_path() .'/files'), which returns sites/default/files if the full bootstrap has not been done yet. so #54 makes sense.

It might be expected behavior from the looks of the code, but it is not good behavior in this case. Needs to be fixed.

Gabor, wouldn't that variable_get require DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE? Then surely we go back to the memory issue again? I have over 1,000 rows in my variable table from the 4.5.x era, if those become available, that's an immediate drain on memory again. Maybe I'm misunderstanding here, and obviously it returning the wrong value isn't good, but imo this issue risks going out of scope given the only reason to open it was memory_limit requirements, and I'm still very unconvinced of the value of warning users about their files directory on update.php at all.

catch: agreed, displaying the files directory warning is IMHO a minor point, as we discussed earlier. The files directory is not a requirement for the core update system, so we should not flex our muscles to try and check for that. Just skip that when doing an update phase check.

I'd love to close this issue, it looks like we are going in circles now :|

Gabor, well, I tried to do that in #54, which I'd be the first to admit is a bit of a hack, but I'm not sure how else it'd be dealt with. Would removing the (void) description and adding a comment as to why it's being set to REQUIREMENT_OK be enough?

Gábor: #54 makes sure the files directory requirement is not checked by setting it to REQUIREMENT_OK in $phase == 'update'. Isn't it what we what, or am I confused too :-/
unless you prefer a better way to ignore the files directory requirement?

follow up on catch's patch #54: adding a comment about skipping the files directory requirement during the update.

Where should this skip be implemented - in system_requirements('update'), I guess?

fixing minor whitespace issue.

I'd only init the file check to REQUIREMENTS_ERROR if runtime or installer phase is used, not mask out it later.

doh! :)
back to #52

Patch applies and shows the memory limit warning as expected. But I'm still seeing the "(Currently using File system Not writable)" warning which is to be suppressed (#62, #63).

edit: I'm doing a v5->v6 update and my v5 config has no sites/default/files directory. This is what triggers the warning.

OK I just tested a few versions of the patch, and whatever happens, the file system error is set to REQUIREMENT_ERROR unless there's an explicit update $phase set, so it gets printed along side php version errors and the rest.

So I see a couple of options:

1. #68 explicitly set the file system to REQUIREMENT_OK

2. Stop the file system check from running on $phase == update in system_requirements() altogether.

Option 2 is the only version that doesn't have a patch, and I think that's what Gabor's asking for.

We can force the check not to fire on update.php if we wrap line 194 onwards of system.install onwards to the end of the file system check with

if ($phase == 'install' || $phase == 'runtime') {

I won't be able to roll a patch until an hour or so, but posting this progress so far to get opinions on whether that's the way to go.

this patch defaults the files check to REQUIREMENT_OK, and only sets it to REQUIREMENT_ERROR during the install and the runtime.

just saw catch's comments.
re 2.: I think the switch between install, runtime and update should remain in system.install to remain consistent as much as possible.

Much better. If we were able to fix the check to return the correct path during update at a later date, it'll be ready for that too. Tested fine. RTBC.

Instead of

'value' => $t('Not writable'),
'severity' => REQUIREMENT_OK,

Which looks quite contradictory, what about filling in these keys only if we have a description, similar to how things are done in the memory limit checker (for building the description)?

according to Gábor's comments, I refactored some parts of the file directory check to be more similar to the memory limit check. Now severity is set to REQUIREMENT_WARNING only if we have a description i.e. a problem to report.
While I was at it, I also centralized the severity of the memory limit check in the same way.

Tested with php memory above and below the limit on a 5->6 update. Warning shows when expected. No confusing file system warning.

Works well so RTBC.

This one looks much better, thanks! Committed.