Error in image_styles of image.module on database update

i had this error:
Warning: array_merge() [function.array-merge]: Argument #1 is not an array in image_styles() (line 582 of modules/image/image.module).

the reason was:
In my case i forgot to install the imagecache_actions module after deploying the image styles from an old website to the new one. The problem was that some image styles had actions from the imagecache_actions module.

the solution:
After installing the missing module the error disappeared and everything worked fine.

Maybe it will work for you, too.

I''m sure the trouble is in your sql database. When I updated my website from 7.0 to 7.22 i had no such warnings. Image module is cored, so if the core is original then... Btw, I had to rebuild all my image styles after update, so you should do i think

You can reproduce this error if you have some image style with enabled effects from Imagecache actions module and then disable Imagecache actions module (together with its submodules).
If you're disabling Imagecache Actions module make sure you have all its effects removed from image styles and you won't get this error.

I'm re-opening this issue because the image_effect_definition_load() function from image.module should return an empty array if there are no image effect definitions.
It is returning FALSE in this case now and breaks image_styles() function with this warning:
Warning: array_merge(): Argument #1 is not an array in image_styles() (line 582 of /var/www/.../modules/image/image.module).

The warning is coming from these lines of code:

         foreach ($style['effects'] as $key => $effect) {
            $definition = image_effect_definition_load($effect['name']);
            $effect = array_merge($definition, $effect);
            $style['effects'][$key] = $effect;
          }

More specifically, where array_merge() tries to merge the $definitions with $effect, but $definition variable is (boolean) FALSE instead of empty array to fit the array functions.

To reproduce this error see comment #3.

Patch is attached.

Ive been seeing this issue for weeks and not had a good solution for it. The patch in #5 works, and is a simple fix.

Could this use some tests?

Also, while the patch definitely makes sense, at this stage of Drupal 7 development it might be preferable to not change the API at all here, and just make the calling code deal with the fact that this can be FALSE. I think we should at least discuss that...

My 2-cents: Yes patch from #5 works. We really needed it to clear up problems with Imagecache Actions module.

OK, here's a patch that fixes the notice in the calling function by checking the return value, instead of changing the API.

leaving on needs review.

This error happened to me as soon as I installed the colorbox module.
Patch #5 worked.

[EDIT]
Previously I claimed that there might be a problem with Patch #5 thinking there was a problem with the Media module. I was doing something very wrong in my templates that had nothing to do with the discussed situation here.

Patch #9 works fine, doesn't change the existing API for other callers, and testbot continues its personal vendetta against me.

Not quite sure how I add a file to this comment but i applied #9 and it's worked for me:

Done a git diff and this could be the next patch:

diff --git a/modules/image/image.module b/modules/image/image.module
--- a/modules/image/image.module
+++ b/modules/image/image.module
@@ -583,9 +583,11 @@ function image_styles() {
           $style['storage'] = IMAGE_STORAGE_DEFAULT;
           foreach ($style['effects'] as $key => $effect) {
             $definition = image_effect_definition_load($effect['name']);
-            $effect = array_merge($definition, $effect);
-            $style['effects'][$key] = $effect;
-          }
+            if ($definition) {
+              $effect = array_merge($definition, $effect);
+              $style['effects'][$key] = $effect;
+            }
+           }
           $styles[$style_name] = $style;
         }
       }

attaching patch file for testbot

re-attaching patch for testbot.

@drasgardian, could you explain the diff you introduced? unfortunately none of these patches made it to 7.28, so i'd like to RBTC the proper patch.

am RBTC'ing #16

I was getting this error on cache clear. Patch #16 worked for me!

hey peeps, this patch didnt make it to D7.32.
what other RBTC does it need? let me know how i can help.
thanks!

I think David_Rothstein said it needed tests?

Added "Needs tests" tag per #22 and #7.

I can reproduce this error and can confirm that the patch in comment #16 from drasgardian fixed this error message in Drupal 7.32 on our side. Thanks!

My steps to reproduce:
Step1) Login to Drupal
Step2) Flush all caches
Step3) Press a link on the website which redirects to the frontpage
Step4) Now press F5 and the error appears on your screen.

Although it could be that the error occurres because of another module on our website which is using this module incorrectly.

I was running into this in a simpletest I was writing for a custom module, which required a feature module to be enabled in the process. This fixed the warnings that were being thrown when enabling the feature module. +1 for the patch fixing this, not sure what kind of test should be written for this though...

+1 Patch #16. Thank you.

This addition of patch #16 needs to be committed to the core image.module that goes out in new core - three four releases of core and the error re-appears each time in core - now 7.40 7.41 and has to be re-patched AGAIN and AGAIN

It cannot be committed as such. The maintainer asked for regression testing to be added to the patch.

Now version 7.50 had to patch this AGAIN - see #27

It works . . . but it keeps erasing itself without committal to core - and makes more work over and over

Forget regression testing - just comit it to core - since obviously no one is doing the testing anyway

A core maintainer requested that someone write tests to ensure it doesn't cause regressions and actually fixes the bug. While it's great that the patch fixes the problem for some existing sites, it'd be really, really helpful if someone could document the steps to trigger this bug so that tests could be written for it.

David_Rothstein did not say a test was required, he just asked if it would make sense. If he meant it was required, he would set the status to "needs work" himself, and not "needs review".

Keeping a known bug in the codebase for 3 years arguably does way more damage than committing a fix without tests. :( The amount of patches in my makefiles for fixed issues that haven't been committed yet is ridiculous.

Can we mark this RTBC?

fuzzy76 - yeah - I actually keep a renamed module in the directory with the patch in it ready to go when the core is updated next time - you are right it does more damage having to redo the patch over and over again - makes one wonder WTF is going on with the powers that be

I realize there is frustration about this patch not going in. But there are standards we follow.

As far as I understand this, the issue is about image effects that are still activated on image styles disappearing when the modules providing the effects don't clean up after themselves when they are uninstalled or are missing. Patch #16 acts as a band aid if a missing effect is referenced in an image style. Assuming we are ok with that, this is testable. Re #25, if I were writing a test I would do something like:

1. Enable image_module_test module.
2. Add the some effects to a style, including 'image_module_test_null'.
3. Uninstall image_module_test module.
4. Do whatever it is in the UI that triggers a call to image_styles(). Most likely that is just the image styles list page.

Several people have asked what they could do to help. Write that test or ping David_Rothstein for another opinion on the necessity for a test on this change.

same issue on 7.59

Been patching this every time a new Core of Drupal comes down - some one needs to commit something to core - to stop this lunacy

See #33.

Do you honestly believe leaving a known bug in for 6 years is better for Drupal than comitting a patch without a test?

Closed #2678886: array_merge(): Argument #1 is not an array image.module:586 as duplicate.

Added this to #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77 in preparation for transition to new meta for priority bug fixes as a simple fix. Does it really need a test? There seems to be confusion on the effort and value of a test for this.

I am marking the patch in #16 as RTBC.

In comment #7 @David_Rothstein suggested it is "preferable to not change the API at all here, and just make the calling code deal with the fact that this can be FALSE" and the patch in #16 follows this approach.

The patch is seven years old, it is very straight forward, and still applies to the current drupal 7.78 release and it has been reviewed and tested by several posters in this thread including me.

The patch in #16 adds a check in the section of image_styles() that builds "module-defined styles" based on hook_image_default_styles().

So to have this problem, you have to have an image style defined in a hook that references a broken effect.

The test I've added here adds an extra test module to the image module, where one test module defines an effect and the other a style.

We then disable only the module providing the effect and try to load the admin UI for image styles.

This reproduces the problem from the IS if the patch from #16 is not applied. The patch prevents the error and the test passes.

However...

I think this is really what's sometimes referred to as "core babysitting broken code"; if a module defines a style that uses an effect from another module that dependency should be captured in the info file such that Drupal will not allow you to disable the effect module and break the style.

Also, the patch alone doesn't fix all problems on a site that has a style with a broken / orphaned effect.

I've commented out the last couple of lines in the test, but if they're left in the admin edit page for the broken style is requested in the test and that results in more errors / warnings - e.g.:

Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Notice     image.module      1343 image_effect_apply()               
    Undefined index: effect callback
Exception Notice     image.admin.inc     91 image_style_form()                 
    Undefined index: label
Exception Notice     image.admin.inc     98 image_style_form()                 
    Undefined index: label
Exception Notice     image.admin.inc    100 image_style_form()                 
    Undefined index: weight

Those happen when the admin UI tries to apply the broken style to the example image.

So this seems like an incomplete fix for a situation where a site is set up such that it has at least one style defined in code that is broken because it depends on an effect that no longer exists.

I'm not questioning that the patch prevents some real life sites emitting errors, but writing the test reveals exactly how broken I think those sites have to be in the first place.

I'll review this with @Fabianx and discuss whether we want to commit the patch (and test).

One thing we could consider is logging a (watchdog) error when image_styles() skips a broken effect in order to notify site owners of the situation?

Urghh I think those failures were because of the way I had to mess with the setUp method of the parent test class.

Probably don't want to keep the fix for that in these patches but hopefully the result will be cleaner.

Borrowed the setUp approach from CommentHelperCase.

+++ b/modules/image/image.test
@@ -573,6 +578,10 @@ class ImageAdminStylesUnitTest extends ImageFieldTestCase {
+    parent::setUp('image_module_test', 'image_module_styles_test');

Shouldn't these modules be in an array?

Obviously we'd also want to remove commented out lines in the test etc..

Tidied up the test (the module list passed to setUp can be an array but doesn't have to be).

Also added a watchdog error when image_styles() skips a broken effect in order to notify site owners of the situation as it's likely to cause other problems and should be fixed.

The log message looks like this:

12/May 11:02  image     error     Image style test_image_style has an effect image_module_test_null with no definition.

The test checks for the presence of this log message.

N.B. instead of disabling a module that provides the effect, we could just have a test module that defines a style that uses a non-existent effect. However, I think it's better not to create a broken state when the test module is enabled. I've also added a dependency to the module info file, but calling module_disable() directly doesn't check that - which is a good thing if we want to simulate the broken state sites can get into.

Oops, the new module was missing from the non-test_only patch.

Arghh and the actual patch was missing from that one.

Sorry for the mess.

• The test only patch in #49 is the correct one, with 2 failures as expected.
• The test + fix patch is #51 and tests passed.

RTBC + 1, approved for Merge! Thanks all!

Thank you to everyone that contributed!