Caching in image_effects_definitions() broken

Nice catch.
Should we add tests?

Are you sure that removing that !empty() is ok?

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

+++ 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?

Duh! Nice pickup.

Great catch. Committed to 8.x. Moving to 7.x. for backporting.

backporting in progress

The D7 patch still has the !empty($cache->data) in it, maybe that is causing the fail? Seems odd though.

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.

#12: image-effects-caching-1761086-12.pass_.patch queued for re-testing.

Thanks Berdir, glad to have that cleared up. Will re-roll when I get a chance.

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:

class 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.

Can we change ImageToolkitTestcase->setup to include image_module_test instead?

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 {

Looks ok here I think, lets try it.

+++ 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...

Much nicer, thanks for the advice. New patches attached.

Looks good to me.

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/4d39299