When trying to set another colorscheme for Bartik on a system with no memory limit, you will get the following message:

There is not enough memory available to PHP to change this theme's color scheme. You need at least [amount] MB more. Check the PHP documentation for more information.

This happens because the value for no memory_limit is -1 and:

  • parse_size strips the sign
  • validation doesn't test for -1

There are two approaches, both attached.

The first treats limit -1 as a flag and only parses if it is not -1.
The second modifies parse_size to not strip the negative sign. I didn't bother with ensuring it comes before any nums, as parse_size doesn't bother with this for unit-characters either.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Heine’s picture

Status: Active » Needs review
underq’s picture

I tried the two patches and they work.

I prefer the first approach because it didn't update parse_size, but I am a little noob ;)

underq’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests, +Needs backport to D7

Thanks Heine. The first approach makes the most sense to me, though I think we could condense it into one if rather than two?

Also, can we add test coverage for this bug?

underq’s picture

Add memory test

xjm’s picture

Thanks underq! I think we'd need to do slightly more than that--temporarily set the memory limit to -1, and then restore the setting.

Also, just a couple minor notes about the code style -- the indentation is off a little there, and we'd want the assertion to use t() on the full string (since the text we're checking for uses t(). Reference: http://drupal.org/simpletest-tutorial-drupal7#t

But now I'm having second thoughts on whether we should test this. I'll ask for a second opinion.

xjm’s picture

For reference, here's how the allowed value of -1 is handled elsehwhere in core. In simpletest_requirements():

  $memory_limit = ini_get('memory_limit');
  if ($memory_limit && $memory_limit != -1 && parse_size($memory_limit) < parse_size(SIMPLETEST_MINIMUM_PHP_MEMORY_LIMIT)) {

In system_requirements():

  // Test PHP memory_limit                                                      
  $memory_limit = ini_get('memory_limit');
  $requirements['php_memory_limit'] = array(
    'title' => $t('PHP memory limit'),
    'value' => $memory_limit == -1 ? t('-1 (Unlimited)') : $memory_limit,
  );

  if ($memory_limit && $memory_limit != -1 && parse_size($memory_limit) < parse_size(DRUPAL_MINIMUM_PHP_MEMORY_LIMIT)) {

I almost wonder if we should have an API function.

xjm’s picture

Something like:

/**                                                                             
 * Compares the memory required for an operation to the available memory.
 *
 * @param $required
 *   The memory required for the operation, expressed as a number of bytes with
 *   optional SI or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB, 8bytes,
 *   9mbytes).
 * @param $memory_limit
 *   (optional) The memory limit for the operation, expressed as a number of
 *   bytes with optional SI or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G,
 *   6GiB, 8bytes, 9mbytes). If no value is passed, the current PHP
 *   memory_limit will be used. Defaults to NULL.
 *                                                                              
 * @return                                                                      
 *   TRUE if there is sufficient memory to allow the operation, or FALSE        
 *   otherwise.                                                                 
 */
function drupal_check_memory_limit($required, $memory_limit = NULL) {
  if (is_null($memory_limit)) {
    $memory_limit = ini_get('memory_limit');
  }
  if ($memory_limit && $memory_limit != -1 && parse_size($memory_limit) < parse_size($required)) {
    return FALSE;
  }
  return TRUE;
}
xjm’s picture

Assigned: Unassigned » xjm

OK, I like that idea enough to roll a patch for it.

xjm’s picture

Totally untested patch. Let's see what testbot thinks.

underq’s picture

I like this idea !

Waoo you are very impressive ;)

xjm’s picture

Issue tags: +Needs manual testing

So, to test this manually:

  1. Set your allowed memory size to less than 32 MB and try to install Drupal. (Should fail requirements.)
  2. Set your allowed memory size to 32 MB or more and try to install Drupal. (Should work.)
  3. Set your allowed memory size to -1 and try to install Drupal. (Should work.)
  4. Set your allowed memory size to less than 64 MB and try to enable the testing module. (Should fail requirements.)
  5. Set your allowed memory size to 64 MB or more and try to enable the testing module. (Should work.)
  6. Set your allowed memory size to -1 and try to enable the testing module. (Should work.)
  7. Set your allowed memory size to 64 MB or more and try to submit the color scheme form at admin/appearance/settings/bartik. (Should work.)
  8. Set your allowed memory size to -1 and try to submit the color scheme form at admin/appearance/settings/bartik. (Should work.)
  9. And the tricky one, get color.module to fail the requirement. I guess you could test this by setting your memory limit to something quite low before submitting the form, and/or replacing the base image with something giant.

For reference, the memory check was added in #92059: Check memory usage before changing color scheme.

Damien Tournoud’s picture

Because we have a separate function, we could have unit tests now :) [read: we should]

if (is_null($memory_limit)) {

This needs to be !isset($memory_limit). We don't use is_null().

xjm’s picture

@Damien Tournoud -- I was a little hesitant because I'm not sure if it's a good idea to go tinkering with the PHP memory limit during a test run. The optional parameter could allow us to mock the memory limit at least, I guess. Any suggestions on the best way to test it?

Attached corrects the is_null() to !isset().

naxoc’s picture

Here is a go at a unit test. I did not feel confident about changing memory settings in tests either, so I used the optional parameter.

Any other ideas on what to test?

BTW - unit tests are awesome. This one runs in 0 seconds! Not like running web test cases at all!

xjm’s picture

Assigned: xjm » Unassigned

Thanks @naxoc! That test looks pretty good to me. One additional assertion I'd add is for the case where the limit is -1, especially since that's important in this issue.

Minor point is that we can drop the t() from the assertion message text for better performance and decoupling. (References: http://drupal.org/simpletest-tutorial-drupal7#t and #500866: [META] remove t() from assert message)

We could also probably make the assertion messages a bit more specific for easier debugging. For example, maybe:
drupal_check_memory_limit() returns FALSE for twice the available memory limit.

naxoc’s picture

I made another assert on the -1 limit and cleaned the messages up a bit. Also gave the test function a more useful name.

xjm’s picture

Nice, this looks good to me for tests. Attached adds a couple of other minor fixes (removes one remaining unneeded t(), adds a little whitespace for readability, and adds () after the function name per our standard so that it will be automatically linked on api.drupal.org).

Thanks naxoc!

The next step for this issue will be the manual testing as per #12. Tagging novice for that.

star-szr’s picture

Manual testing completed as per #12 on current 8.x.

Tests 1 through 8 passed.

For test 9 - I tried lowering my PHP memory values but I couldn't find a sweet spot where color.module was broken but Drupal wasn't.

xjm’s picture

Thanks a ton @Cottser! I'll try to follow up with someone who understands color module for the 9th case.

xjm’s picture

Issue tags: -Novice

Oh and I guess that is not so novice-y either.

adharris’s picture

Status: Needs review » Needs work

I ran through the tests in #12:
1-7 Worked as expected
I tested 8 both with and without the patch in #18, and got expected results:

  • Without patch: could not change colors with -1 memory limit
  • With patch: color change worked fine.

For the 9th test, I did the following:

  1. Set PHP memory limit to 32M
  2. Confirmed that i could change color scheme
  3. Resized bartik's base.png to 2048x2048 pixels $ convert -resize 2048x2048\! base.png base.png
  4. Confirmed that I could no longer change colors, received a message "You need at least 46.75 MB more"
  5. Set PHP memory limit to 256M
  6. Confirmed that I could change color scheme
  7. Set PHP memory limit to -1
  8. Confirmed that I could change color scheme

It seems to be working as expected, but I questioned the math in the error message. Color calculates the required memory in bytes as length*width*8 using the dimensions of base.png. For my resized image, that is 2048*2048*8 bytes = 33554432 bytes ~= 33.55 megabytes. Assuming that Drupal is using almost all of my available 32MB PHP memory, i should at most need 33.55 Mb more, and yet the message says i need 46MB.

Looking at the message, it amount needed is calculated as $usage + $required - $memory_limit. Doing a little bit of debuging, it appears that $required and $usage are in integer bytes, while $memory_limit is in php.ini format, 32M. Looks like this regression was introduced by removing the parse_size() call here

+++ b/core/modules/color/color.moduleundefined
@@ -324,9 +324,9 @@ function color_scheme_form_submit($form, &$form_state) {
-    $limit = parse_size(ini_get('memory_limit'));
-    if ($usage + $required > $limit) {
-      drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $limit), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
+    $memory_limit = ini_get('memory_limit');
+    if (!drupal_check_memory_limit($usage + $required)) {
+      drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $memory_limit), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
xjm’s picture

Issue tags: +Novice, +API addition

Great work @adharris. So it sounds like the new API addition works fine and we just need to fix the message. Seems like a good novice task.

dags’s picture

Assigned: Unassigned » dags
dags’s picture

Status: Needs work » Needs review
FileSize
981 bytes
6.34 KB

Fixing issue in #22.

xjm’s picture

Alright, that change looks correct. Thanks @davidjdagino!

I can't RTBC this since I worked on the patch. :)

xjm’s picture

Issue tags: -Novice, -Needs manual testing

Oops, and untagging.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me!

Tor Arne Thune’s picture

Yes, looks great. Great work on the manual testing, and of course the patch.

bleen’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/color/color.moduleundefined
@@ -324,9 +324,9 @@ function color_scheme_form_submit($form, &$form_state) {
+    $memory_limit = parse_size(ini_get('memory_limit'));
+    if (!drupal_check_memory_limit($usage + $required)) {

Not that ini_get() is a particularly heavy function, but as long as we are getting the PHP memory limit why dont we pass it in to the drupal_check_memory_limit call so that ini_get isnt called again?

+++ b/core/modules/simpletest/simpletest.installundefined
@@ -63,7 +63,7 @@ function simpletest_requirements($phase) {
   $memory_limit = ini_get('memory_limit');
-  if ($memory_limit && $memory_limit != -1 && parse_size($memory_limit) < parse_size(SIMPLETEST_MINIMUM_PHP_MEMORY_LIMIT)) {

same comment as above

+++ b/core/modules/system/system.installundefined
@@ -201,7 +201,7 @@ function system_requirements($phase) {
+  if (!drupal_check_memory_limit(DRUPAL_MINIMUM_PHP_MEMORY_LIMIT)) {

same comment as above

-18 days to next Drupal core point release.

xjm’s picture

Assigned: dags » xjm

Good point. Rerolling this.

xjm’s picture

Attached passes the memory limit to drupal_check_memory_limit() when it's already available in scope. I also saved it to a variable in the unit test to encourage reuse if we expand this test in the future.

We should probably test this manually again. See #22.

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

Assigned: xjm » Unassigned
BTMash’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me :)

tim.plunkett’s picture

1-8 was fine, and 9 failed before the patch and worked afterward. The code is much cleaner now, awesome work!

tim.plunkett’s picture

Issue tags: -Needs manual testing

Removing tag.

bleen’s picture

Thanks for re-rolling xjm :)

catch’s picture

Status: Reviewed & tested by the community » Needs review
+  if ($memory_limit && $memory_limit != -1 && parse_size($memory_limit) < parse_size($required)) {
+    return FALSE;
+  }
+  return TRUE;

Can we just make that

return !($memory_limit && $memory_limit != -1 && parse_size($memory_limit) < parse_size($required));

Or ideally make the one-liner a positive comparison.

xjm’s picture

So are you saying you would prefer the condition logic inverted like this?
return !$memory_limit || ($memory_limit == -1) || (parse_size($memory_limit) > parse_size($required))

lotyrin’s picture

I agree with catch, and #42 fixes it, in my opinion.

xjm’s picture

Meh. I personally find that harder to read and the line distastefully long, but I'll roll that with an inline comment if that's what people prefer.

ZenDoodles’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed. Logic is essentially the same. Let's ship it!

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed/pushed to 8.x, moving to 7.x for backport.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.53 KB
1.95 KB

Rerolled.

ryan.gibson’s picture

Assigned: Unassigned » ryan.gibson
xjm’s picture

Status: Needs review » Reviewed & tested by the community

The failure in #48 is "wrong" (tests failed to run) because the function is not defined before the patch. So that's not a concern. Diffstat matches and the backport looks correct.

ryan.gibson’s picture

Assigned: ryan.gibson » Unassigned
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

I think we need to fix #1573556: Install suggests PHP memory limit is increased when it's already sufficient before backporting this patch to Drupal 7, because it looks like this patch is what caused that bug.

In particular, this:

+function drupal_check_memory_limit($required, $memory_limit = NULL) {
.......
+  // - The memory limit is greater than the memory required for the operation.
+  return ((!$memory_limit) || ($memory_limit == -1) || (parse_size($memory_limit) > parse_size($required)));

Is not the same as this:

-  if ($memory_limit && $memory_limit != -1 && parse_size($memory_limit) < parse_size(DRUPAL_MINIMUM_PHP_MEMORY_LIMIT)) {
+  if (!drupal_check_memory_limit(DRUPAL_MINIMUM_PHP_MEMORY_LIMIT, $memory_limit)) {

Should be a simple fix, of course; change the ">" to ">=" and add a test :)

tim.plunkett’s picture

When backporting, one of the assertions is useless and will hopefully be removed from D8: #1630746: Checking for "twice the available memory limit" fails when running on a system with a memory_limit of -1

Dave Reid’s picture

Dave Reid’s picture

Fixed the missing chunk in system.install.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Almost 13 months later... RTBC!

markhalliwell’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +API addition, +Needs backport to D7

The last submitted patch, 1453984-drupal-check-memory-limit.patch, failed testing.

markhalliwell’s picture

Issue tags: +Needs reroll

.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs backport to D7, -Needs reroll
FileSize
6.61 KB

The patch still applies for me; this might just be Git being stupid about patch fuzz.

Here's a straight reroll. Since the patch still applied I didn't touch the code at all, so I'll set this back to RTBC for now.

David_Rothstein’s picture

Issue tags: +Needs backport to D7

Hm, a tag went missing for no apparent reason. Restoring it.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes

Tests passed, and everything looks good. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/19e65b9

markhalliwell’s picture

Issue tags: -Needs backport to D7

Removing tag since it was committed against 7.x.

Status: Fixed » Closed (fixed)

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

rahulbile’s picture

Here is the patch against 7.22, I needed for make file of platform.