Increase PHP Memory Limit for Simpletest

Notice as in a warning that pops up on install?

Or an installation requirement?

Requirements is probably better if we choose to do this.

I also can't run all tests at the same time, 'notice: Undefined index: test_id' .. is this a known issue allready with a patch somewhere ?

@swentel: this last error is probably linked to #290316: Simpletest test_id assignment broken (critical on PostgreSQL): you need to fully reinstall Simpletest tables following a change in the database schema.

@Damien : I'm posting some debug info over on that issue (I'm not making it active yet). Simpletest simply keeps crashing, no idea why yet (even on a fully fresh install)

Again any opinions on what method to enforce/warn about that (as in #1).

There's already a memory limit check in system_requirements, so could be copied from there.

This should do it. Although I ran into an issue that I will post in as separate issue and link to in this thread in a bit.

http://drupal.org/node/267883#comment-979649

Still applies.

Looks good to me. I tested the following:

Tried to install with a memory limit of 16M, received an error message informing me to raise my memory limit to 33.55 (?) megs.
Added an ini_set('memory_limit', '32M') to settings.php, installed simpletest successfully.
Removed the ini_set call and visited admin/reports/status, received a warning about requirements not being met.
Added the ini_set again and the warning went away.

I've rerolled the patch to list the memory minimum in the runtime check as it does on the install check.

I'd say RTBC but dependent on the format_size patch boombatower linked to above, it's pretty confusing to list a memory requirement of > 33MB when in reality a ini_set('memory_limit', '32M') is all you need.

Slight wording change.

Patch applies fine, and works like a charm. I got the warning when installing, which prevents me from installing the module. Therefore, I'm all for this feature, as I keep forgetting to increase my memory limit after refreshing my patch-testing-environment.

But there is no requirement check when enabling the already installed (but disabled) module. So, after the following actions
- Increase memory_limit
- Install module
- Disable module
- Decrease memory_limit
I can enable the module again, even though the requirements aren't met.
Is this an issue for this patch, or a bug in the requirement-checking-mechanism of Drupal itself?

I also recommend making sure that the value shows what one might expect (and need to use for memory_limit): 32MB, not 33,55MB, either via the issue mentioned in #10, or otherwise.

#10 will be the fix for the 1024 stuff.

The requirements checking mechanism would be what is allowing you to get by the requirement, but that is an edge case. Someone shouldn't try to be breaking it as it will only result in the module not working. Either way the requirements checking would be a separate issue.

Thanks for your clarification. In that case, you have my seal of approval. Do we wait for #10, or get this in right now and deal with the outcome of #10 later? If so, feel free to mark it RTBC, seeing at least 3 people have tried and tested this.

That or mark this as postponed, awaiting outcome of #10.

At this point I think this is somewhat confusing, but that is another reason to get #10 on its way.

Why during the install phase could you not just have...

    if ($phase == 'install') {
      ...
      if (!$memory_met) {
        $requirements['simpletest_php_memory_limit'] = array(
          'title' => $t('SimpleTest: PHP memory requirement'),
          'severity' => REQUIREMENT_ERROR,
          'description' => t('SimpleTest requires the PHP memory limit be set to @minimum or greater. For more information
                              on how to change the PHP memory limit see the <a href="@increase">documentation</a>.',
                              array('@minimum' => format_size($memory_minimum), '@increase' => 'http://drupal.org/node/207036')),
        );
      }

...instead of checking $memory_met and adding a value to $requirements that will never be seen?

I agree with Dave. We should drop the $memory_met branch of $phase = 'install'.

Fine by me. New patch attached. Also changes some remaining t()'s into $t()'s.

Two remarks, but not yet implemented:
- The cURL-requirement does the same with the install phase, is it a terrible crime if we remove that one as well in this patch?
- The wording of the runtime requirement is a bit wordy. What do you think of:

 title = SimpleTest: PHP memory requirement
value = $memory_met ? $t('Greater than @minimum', etc) : $t('Less than @minimum', etc)

* Agree with wordyness and changed it to suggested.
* Removed extra cURL requirement entry.
* Changed cURL requirement key to 'simpletest_curl' for consistency with new 'simpletest_php_memory_limit key'.
* Defined $memory_minimum_arg = array('@minimum' => format_size($memory_minimum)); since it is used 4 times in a row.

Tested and fixed a small bug that was introduced by the patch in #21. (You put an array in an array, in stead of merging 2 arrays). Nothing else to add, seems ready to me.

RTBC?

Good catch.

Fixed a few coding consistency issues:
1. Changed t() parameter !curl_url to @curl_url.
2. Removed line wrapping on simpletest_php_memory_limit description.

Code runs locally.

- The $memory_minimum_arg does not actually improve readability. Please don't put it in a variable.
- The the <a href="@increase">documentation</a> is not really helpful. Please remove it, or make the message consistent with the one of system.install.

Thanks.

New version.

Mobile: patch addresses issues in #26.

I've been testing and reviewing this patch some more. I'm now wondering if it wouldn't be better if we could simply overwrite/bump the existing memory limit requirements when Simpletest is enabled. It seems like this functionality would be useful for other contributed modules too. For example, Views and CCK might declare that they need 32MB and Drupal would report the max() of all memory requirements as the actual requirement.

Since we are allowing each module now to specify a php version, etc, having this per-module memory requirement also move to the .info file seems like a better and more general approach. Even better might be to specify a per-module memory delta. i.e. Drupal core will install with only the required modules, but if you go to turn on 5 more, Drupal will sum up the additional required memory and give a requirements failure.

Otherwise, how can each module guess at the requirements of all the other existing enabled modules?

Yep, sounds like a good plan.

There's already work on that here: #198053: GHOP #67: Shows how much memory is in use versus your server configuration's maximum memory

Cross posted with webchick, marking this as a duplicate then.

Sorry for the multiple bumps, I'll try to leave it here.

Even better would be if people would work HARD on lowering Drupal memory requirement.

A couple of comments:

As Dries hinted, there are situations where this patch would lead to more than one memory warning (one for core, one for SimpleTest) on the same screen, which is maybe a bit weird. We ran into a similar situation at Acquia a little while ago (with Drupal 6). Came up with a decent workaround, which I'd be glad to share... but since this is Drupal 7, yes, it's better to try fixing the problem for real.

The idea in #198053: GHOP #67: Shows how much memory is in use versus your server configuration's maximum memory seems really cool, but the idea I came up with (and started to write a patch for) was something a little simpler and also hopefully more general... maybe we could let modules modify each others' requirements? In other words, introduce a hook_requirements_alter() hook? If this seems like a good idea, I can post a patch for it.

Using hook_requirements_alter(), SimpleTest would easily be able to modify Drupal's standard 16MB limit to 32MB if it wanted to. (I wonder if it should, though, given that the 32MB requirement is really only for one page, i.e. when running tests, and not for the whole site?)

As for this specific patch:

Why not just print the '32MB' directly to the screen rather than trying to convert it using parse_size()? That's the way Drupal does it now... and then there'd be no need to worry about the 33.5MB thing, right? Also, I think the "less than"/"greater than" part could be improved a bit. Why not say "32MB or greater" when the requirement is met, but then just print "16MB" (or whatever they are running with) when it fails, with a sentence of explanation below (and in the sentence, explain that it needs to be at least 32)? Again, I think that's a little closer to the way the main Drupal memory check does it now...

Eh, no need to wait... I've posted a patch for hook_requirements_alter() at #309040: Add hook_requirements_alter(). Unfortunately, it's not working 100% correctly yet though.

Anyway, it may or may not be the right solution for the issue at hand here... consider it a possible tool that could be used to resolve this one, if desired ;)

Wow. A lot of changes after I stopped working :) But let's work on the memory stuff in #198053: GHOP #67: Shows how much memory is in use versus your server configuration's maximum memory .

Let's use this issue for prettifying the cURL requirement. Attached patch does that.

weird.
With attachment now.

Makes sense.

Thanks, committed!

Cool. Should we move this back to the original issue then? That one is still not fixed.

Are we waiting on the other patch to add it to info file or something else?

If so postpone this.

Yeah, I think this needs postponing until there is a definitive way to handle module memory requirements. Or at least until #309457: Allow profiles to specify required memory in .info file, then we have a way to tell Core how much (more) memory we need for it to run.

If that is in, we can use #198053: GHOP #67: Shows how much memory is in use versus your server configuration's maximum memory to measure how much memory we actually need and put that in simpletest.info.

This has been completed.

This is back in play, because 32M isn't enough to run SimpleTests out of the box (with only core modules and Testing enabled). We need an actual solution.

And a sane patch, which tries to set and check the limit for the Testing page.

We should be OK on the batch pages. The real problem is the form loading for the first time without all the test data in cache.

I think that is issue don't hold drupal 7 release, so setting non critical..

Better patch, will will not alter limits > 64M.

Cross-post. I don't know. A WSOD on install / testing is pretty bad. Setting back to 'major' for other eyes to review.

#50: 295697-sane-test.patch queued for re-testing.

Any reason not to have simply an additional requirement for Simpletest?

Yeah, this should be in simpletest_requirements().

Alright.

#56 has got some errors, like $new_limit not being defined and not dealing with memory_limit = -1 (i.e., unlimited) correctly.

This code mimics the PHP memory limit detection in system.install. Also rephrased the error message.

Makes sense and seems to adhere to everything mentioned in the comments.

I looked to see if there was a way to do this without breaking strings, but there doesn't seem to be anything suitable in system.install we can steal. Darn. :( Given that the alternative is a WSOD, though, I think this is an acceptable breakage.

This isn't the pie in the sky thing that Dries wanted to do earlier, but that was September 2008 and today is Januay 2011. We're using system_requirements() as documented to warn people before they get themselves into a bind, which is an acceptable fix that we can do for Drupal 7 before release.

Committed to HEAD! Thanks.

+++ modules/simpletest/simpletest.installundefined
@@ -58,6 +61,14 @@ function simpletest_requirements($phase) {
+    $requirements['php_memory_limit']['description'] = t('The testing framework requires the PHP memory limit to be at least %memory_minimum_limit. The current value is %memory_limit. <a href="@url">Follow these steps to continue</a>.', array('%memory_limit' => $memory_limit, '%memory_minimum_limit' => SIMPLETEST_MINIMUM_PHP_MEMORY_LIMIT, '@url' => 'http://drupal.org/node/207036'));

Everyone missed that this needed to use $t() and not t() directly since this is run during install. Luckily this shouldn't change strings at all. Patch available at #1046784: Strings in simpletest_requirements() are not using $t()

Powered by Dreditor.