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.

Files: 
CommentFileSizeAuthor
#65 drupal-check-memory-limit-1453984-65.patch6.53 KBrahulbile
#60 drupal-check-memory-limit-1453984-60.patch6.61 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,455 pass(es).
[ View ]
#55 1453984-drupal-check-memory-limit.patch6.61 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1453984-drupal-check-memory-limit_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#55 interdiff.txt2.06 KBDave Reid
#54 1453984-drupal-check-memory-limit.patch5.72 KBDave Reid
PASSED: [[SimpleTest]]: [MySQL] 40,591 pass(es).
[ View ]
#54 interdiff.txt2.84 KBDave Reid
#48 drupal-1453984-49-test.patch1.95 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#48 drupal-1453984-49-combined.patch6.53 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,072 pass(es).
[ View ]
#45 memory-limit-145398-45.patch6.61 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,374 pass(es).
[ View ]
#45 interdiff-34-45.txt800 bytesxjm
#34 memory-limit-145398-34.patch6.44 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,349 pass(es).
[ View ]
#34 interdiff.txt4.3 KBxjm
#25 color-memory-limit-1453984-25.patch6.34 KBdags
PASSED: [[SimpleTest]]: [MySQL] 35,084 pass(es).
[ View ]
#25 interdiff.txt981 bytesdags
#18 color-memory-limit-1453984-18.patch6.35 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,797 pass(es).
[ View ]
#18 interdiff.txt1.63 KBxjm
#17 color-memory-limit-1453984-17.patch6.34 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 34,796 pass(es).
[ View ]
#15 color-memory-limit-1453984-15.patch6.05 KBnaxoc
PASSED: [[SimpleTest]]: [MySQL] 34,801 pass(es).
[ View ]
#14 color-memory-limit-1453984-14.patch4.45 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,695 pass(es).
[ View ]
#10 color-memory-limit-1453984-10.patch4.45 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,638 pass(es).
[ View ]
#5 color-add_memory_test-1453984-5.patch797 bytesunderq
PASSED: [[SimpleTest]]: [MySQL] 34,635 pass(es).
[ View ]
limit-parse-size-negative.patch1.67 KBHeine
PASSED: [[SimpleTest]]: [MySQL] 34,626 pass(es).
[ View ]
limit-flag.patch1.39 KBHeine
PASSED: [[SimpleTest]]: [MySQL] 34,631 pass(es).
[ View ]

Comments

Status:Active» Needs review

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 ;)

Status:Needs review» Reviewed & tested by the community

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?

StatusFileSize
new797 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,635 pass(es).
[ View ]

Add memory test

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.

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

<?php
  $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():

<?php
 
// 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.

Something like:

<?php
/**
* 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;
}
?>

Assigned:Unassigned» xjm

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

StatusFileSize
new4.45 KB
PASSED: [[SimpleTest]]: [MySQL] 34,638 pass(es).
[ View ]

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

I like this idea !

Waoo you are very impressive ;)

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.

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

<?php
if (is_null($memory_limit)) {
?>

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

StatusFileSize
new4.45 KB
PASSED: [[SimpleTest]]: [MySQL] 34,695 pass(es).
[ View ]

@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().

StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 34,801 pass(es).
[ View ]

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!

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.

StatusFileSize
new6.34 KB
PASSED: [[SimpleTest]]: [MySQL] 34,796 pass(es).
[ View ]

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

Issue tags:-Needs tests+Novice
StatusFileSize
new1.63 KB
new6.35 KB
PASSED: [[SimpleTest]]: [MySQL] 34,797 pass(es).
[ View ]

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.

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.

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

Issue tags:-Novice

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

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');

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.

Assigned:Unassigned» dags

Status:Needs work» Needs review
StatusFileSize
new981 bytes
new6.34 KB
PASSED: [[SimpleTest]]: [MySQL] 35,084 pass(es).
[ View ]

Fixing issue in #22.

Alright, that change looks correct. Thanks @davidjdagino!

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

Oops, and untagging.

Status:Needs review» Reviewed & tested by the community

Looks great to me!

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

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.

Assigned:dags» xjm

Good point. Rerolling this.

Issue tags:+Needs manual testing
StatusFileSize
new4.3 KB
new6.44 KB
PASSED: [[SimpleTest]]: [MySQL] 35,349 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Assigned:xjm» Unassigned

Status:Needs review» Reviewed & tested by the community

Looks great to me :)

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

Issue tags:-Needs manual testing

Removing tag.

Thanks for re-rolling xjm :)

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.

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))

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

StatusFileSize
new800 bytes
new6.61 KB
PASSED: [[SimpleTest]]: [MySQL] 35,374 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new6.53 KB
PASSED: [[SimpleTest]]: [MySQL] 39,072 pass(es).
[ View ]
new1.95 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Rerolled.

Assigned:Unassigned» ryanissamson

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.

Assigned:ryanissamson» Unassigned

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 :)

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

Status:Needs work» Needs review
StatusFileSize
new2.84 KB
new5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 40,591 pass(es).
[ View ]

StatusFileSize
new2.06 KB
new6.61 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1453984-drupal-check-memory-limit_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed the missing chunk in system.install.

Status:Needs review» Reviewed & tested by the community

Almost 13 months later... RTBC!

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.

Issue tags:+Needs reroll

.

Status:Needs work» Reviewed & tested by the community
Issue tags:-needs backport to D7, -Needs reroll
StatusFileSize
new6.61 KB
PASSED: [[SimpleTest]]: [MySQL] 40,455 pass(es).
[ View ]

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.

Issue tags:+needs backport to D7

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

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

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.

StatusFileSize
new6.53 KB

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