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.
Comment | File | Size | Author |
---|---|---|---|
#65 | drupal-check-memory-limit-1453984-65.patch | 6.53 KB | rahulbile |
#60 | drupal-check-memory-limit-1453984-60.patch | 6.61 KB | David_Rothstein |
#55 | 1453984-drupal-check-memory-limit.patch | 6.61 KB | Dave Reid |
#55 | interdiff.txt | 2.06 KB | Dave Reid |
#54 | 1453984-drupal-check-memory-limit.patch | 5.72 KB | Dave Reid |
Comments
Comment #1
Heine CreditAttribution: Heine commentedComment #2
underq CreditAttribution: underq commentedI 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 ;)
Comment #3
underq CreditAttribution: underq commentedComment #4
xjmThanks 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?
Comment #5
underq CreditAttribution: underq commentedAdd memory test
Comment #6
xjmThanks 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 usest()
. Reference: http://drupal.org/simpletest-tutorial-drupal7#tBut now I'm having second thoughts on whether we should test this. I'll ask for a second opinion.
Comment #7
xjmFor reference, here's how the allowed value of
-1
is handled elsehwhere in core. In simpletest_requirements():In system_requirements():
I almost wonder if we should have an API function.
Comment #8
xjmSomething like:
Comment #9
xjmOK, I like that idea enough to roll a patch for it.
Comment #10
xjmTotally untested patch. Let's see what testbot thinks.
Comment #11
underq CreditAttribution: underq commentedI like this idea !
Waoo you are very impressive ;)
Comment #12
xjmSo, to test this manually:
admin/appearance/settings/bartik
. (Should work.)admin/appearance/settings/bartik
. (Should work.)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.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedBecause we have a separate function, we could have unit tests now :) [read: we should]
This needs to be
!isset($memory_limit)
. We don't useis_null()
.Comment #14
xjm@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()
.Comment #15
naxoc CreditAttribution: naxoc commentedHere 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!
Comment #16
xjmThanks @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.Comment #17
naxoc CreditAttribution: naxoc commentedI made another assert on the -1 limit and cleaned the messages up a bit. Also gave the test function a more useful name.
Comment #18
xjmNice, 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.
Comment #19
star-szrManual 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.
Comment #20
xjmThanks a ton @Cottser! I'll try to follow up with someone who understands color module for the 9th case.
Comment #21
xjmOh and I guess that is not so novice-y either.
Comment #22
adharris CreditAttribution: adharris commentedI 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:
For the 9th test, I did the following:
$ convert -resize 2048x2048\! base.png base.png
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 hereComment #23
xjmGreat 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.
Comment #24
dags CreditAttribution: dags commentedComment #25
dags CreditAttribution: dags commentedFixing issue in #22.
Comment #26
xjmAlright, that change looks correct. Thanks @davidjdagino!
I can't RTBC this since I worked on the patch. :)
Comment #27
xjmOops, and untagging.
Comment #28
tim.plunkettLooks great to me!
Comment #29
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedYes, looks great. Great work on the manual testing, and of course the patch.
Comment #30
bleen CreditAttribution: bleen commentedNot 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?
same comment as above
same comment as above
-18 days to next Drupal core point release.
Comment #31
xjmGood point. Rerolling this.
Comment #34
xjmAttached 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.
Comment #35
xjmComment #36
xjmComment #37
BTMash CreditAttribution: BTMash commentedLooks great to me :)
Comment #38
tim.plunkett1-8 was fine, and 9 failed before the patch and worked afterward. The code is much cleaner now, awesome work!
Comment #39
tim.plunkettRemoving tag.
Comment #40
bleen CreditAttribution: bleen commentedThanks for re-rolling xjm :)
Comment #41
catchCan we just make that
Or ideally make the one-liner a positive comparison.
Comment #42
xjmSo 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))
Comment #43
lotyrin CreditAttribution: lotyrin commentedI agree with catch, and #42 fixes it, in my opinion.
Comment #45
xjmMeh. 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.
Comment #46
ZenDoodles CreditAttribution: ZenDoodles commentedI reviewed. Logic is essentially the same. Let's ship it!
Comment #47
catchThanks! Committed/pushed to 8.x, moving to 7.x for backport.
Comment #48
tim.plunkettRerolled.
Comment #49
ryan.gibson CreditAttribution: ryan.gibson commentedComment #50
xjmThe 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.
Comment #51
ryan.gibson CreditAttribution: ryan.gibson commentedComment #52
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
Is not the same as this:
Should be a simple fix, of course; change the ">" to ">=" and add a test :)
Comment #53
tim.plunkettWhen 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
Comment #54
Dave ReidThis just bit me as well. Revised patch rolling in changes from #1573556: Install suggests PHP memory limit is increased when it's already sufficient and #1630746: Checking for "twice the available memory limit" fails when running on a system with a memory_limit of -1.
Comment #55
Dave ReidFixed the missing chunk in system.install.
Comment #56
tim.plunkettAlmost 13 months later... RTBC!
Comment #57
markhalliwell#55: 1453984-drupal-check-memory-limit.patch queued for re-testing.
Comment #59
markhalliwell.
Comment #60
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedHm, a tag went missing for no apparent reason. Restoring it.
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commentedTests passed, and everything looks good. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/19e65b9
Comment #63
markhalliwellRemoving tag since it was committed against 7.x.
Comment #65
rahulbile CreditAttribution: rahulbile commentedHere is the patch against 7.22, I needed for make file of platform.