Posted by Berdir on August 28, 2012 at 11:14pm
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | image system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
<?php
if ($cache = cache()->get("image_effects:$langcode") && !empty($cache->data)) {
$effects = $cache->data;
}
?>Who can spot the error?
Exactly, this can never be TRUE, because the () around the assignment are missing, so $cache is never defined. This is like that since the very day image styles were added to Core back in 2009 :)
I have no idea what the !empty() is supposed to do, that can be removed IMHO. Patch coming up in a second.
I'm seeing a cache_set() on every page with this in 8.x when displaying an image derivate.
Comments
#1
#2
Nice catch.
Should we add tests?
#3
Not sure if it makes sense or what the best way to do it would be. Maybe implement a hook that is invoked and count how many times it's invoked?
#4
Are you sure that removing that !empty() is ok?
#5
Here's a test that uses a call to drupal_alter() to check whether image effects are coming from the cache or not.
The patch in #1 seems good to go if my test is not flawed (a big if :P).
#6
+++ b/core/modules/image/tests/image_module_test.moduleundefined@@ -38,3 +38,14 @@ function image_module_test_image_effect_info() {
+ * Used to keep a count of cache hits in image_effect_definition().
cache hits or 'non-cache' hits?
#7
Duh! Nice pickup.
#8
Tests look ok to me. Who wants to RTBC this? :)
#9
#10
Great catch. Committed to 8.x. Moving to 7.x. for backporting.
#11
backporting in progress
#12
#13
The last submitted patch, image-effects-caching-1761086-12.pass_.patch, failed testing.
#14
The D7 patch still has the !empty($cache->data) in it, maybe that is causing the fail? Seems odd though.
#15
Yeah, weird because it passed locally.
I left the empty($cache->data) in because I've a recollection in D7 that cache_get can still return an object with no data, happy to be wrong on that.
#16
#12: image-effects-caching-1761086-12.pass_.patch queued for re-testing.
#17
The last submitted patch, image-effects-caching-1761086-12.pass_.patch, failed testing.
#18
If that would really be the case, then we would have to check it on every single cache_get(), no? But we don't., we only do it here and in one other instance in update.module, which is using the custom pseudo-cache API.
#19
Thanks Berdir, glad to have that cleared up. Will re-roll when I get a chance.
#20
The reason this fails is because ImageEffectsUnitTestCase extends ImageToolkitTestCase, which in turn extends DrupalWebTestCase. ImageEffectsUnitTestCase calls parent::setUp('image_test'), but doesn't deal with any arguments from ImageEffectsUnitTestCase, so when this happens:
<?phpclass ImageEffectsUnitTest extends ImageToolkitTestCase {
function setUp() {
parent::setUp('image_test', 'image_module_test');
}
?>
...those arguments never make it up to ImageToolkitTestCase and image_module_test is never enabled. That makes the test fail.
I'm just working out how best to move things around, and will post a patch soon. If anyone has any ideas, please let me know.
#21
Can we change ImageToolkitTestcase->setup to include image_module_test instead?
#22
We might be able to, but a bunch of other tests inherit from it, and it might make those fail and/or stress out the testbot more than necessary. I'll give it a shot though and see if it works locally. These are the tests that inherit:
./modules/image/image.test:248:class ImageEffectsUnitTest extends ImageToolkitTestCase {./modules/simpletest/tests/image.test:72:class ImageToolkitUnitTest extends ImageToolkitTestCase {
./modules/simpletest/tests/image.test:465:class ImageFileMoveTest extends ImageToolkitTestCase {
#23
Looks ok here I think, lets try it.
#24
+++ b/modules/image/image.testundefined
@@ -255,7 +255,7 @@ class ImageEffectsUnitTest extends ImageToolkitTestCase {
- parent::setUp('image_test');
+ parent::setUp('image_test', 'image_module_test');
+++ b/modules/simpletest/tests/image.testundefined
@@ -14,7 +14,7 @@ class ImageToolkitTestCase extends DrupalWebTestCase {
- parent::setUp('image_test');
+ parent::setUp('image_test', 'image_module_test');
Rather than enabling the module in the parent's
setUp(), we should modify the parent class to enable modules passed from its children. See the pattern here: http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/no...#25
Much nicer, thanks for the advice. New patches attached.
#26
Backport looks good and has tests.
#27
Looks good to me.
Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/4d39299
#28
Automatically closed -- issue fixed for 2 weeks with no activity.