warning: array_filter() The first argument should be an array ... image_attach.module on line 349

A dsm($form_state['values']) at the top of image_attach_validate() shows that $form_state['values']['iids'] is an empty array, and I get no error.

Could you be more specific about how to reproduce this please?

Sorry, without further information this issue can only be marked as won't fix.

Feel free to re-open this issue if you want to provide further information. Thanks.

Hello.

I am experiencing the same error message.

6.x-1.0-beta5

Rather odd, actually, because everything seemed to be working fine. Then, this error started creeping up.

Currently, the site in question only has image_attach enabled for forum nodes. Every time one of these is created or edited, the message is displayed. Despite the error, the post seems to succeed.

Please advise on any additional information requirements.

Thanks.

For those curious, the offending code is:

$num_images = count(array_filter($form_state['values']['iids']));

For the moment, I've hidden the message with the following:
(If the array is empty, manually set the value of $num_images to 0).

$num_images = (empty($form_state['values']['iids']) ? 0 : count(array_filter($form_state['values']['iids'])));

I realize it's very crude, and simply masks the problem, but it works for my client's website until a better solution can be found.

I'm a Java developer, not a PHP developer - but what I've read seems to indicate that the problematic code is used properly... it's just misbehaving for some reason.

I'm new here... and haven't yet read an etiquette mentioning it's bad form to re-open an issue when you're interested in a solution, and willing to help work towards it... so I'll mark this active. If I'm wrong in doing so, please say so.

If it was closed due to insufficient information, it's fine to reopen it if you provide more details. Which is what you have done :D

I'll look into it.

This has to do with the recent feature that allows image_attach to attach multiple images.

Were those nodes created before you updated image_attach? If so, that array would likely never have been set. Could this be the problem?

EDIT: Maybe not. I just added a page without image_attach enabled; then enabled image_attach; then edited the page again. It did not crash at all.

The problem occurs for both existing nodes, and new nodes for the site I am working on.

As I mentioned before... the problem is slightly odd as everything worked splendidly for quite some time (2-3 weeks) without any errors or problems. Then suddenly this cropped up. There were no major changes to the site, and no addition of modules or themes. I'm wondering if there's some non-deterministic behavior with the array_filter() function.

I can't reproduce this at all -- I create forum topics, either with or without attaching some images, and they work fine.

I managed to get that when I was programmatically creating nodes with images attached to them, but that was a problem in my module's code (which is not public ATM).

Do you have any third-party modules installed that create nodes programatically (that is, not through a user form)? If so, it might be a problem with that module.

Theoretically any node that even potentially has attached images will have $form_state['values']['iids'] set as soon as you start editing the page.

That's all I can think of. I can't reproduce the problem either...

There was an issue a while ago about a module that was simulating a form submission... I think that was on image nodes, but there are probably modules out there doing wacky things with image attach too...

I'll take a patch for the fix in #4 :)
(I've not looked at the code, but I wonder if it would be better to just check that value and only carry on if (isset(...)) rather than set it to an empty array.)

I am having this issue as well. It occurs every time I create a new thread as an anonymous user in the forums. (I do not attach any images.)
The quick solution I implemented to take care of it is similar to #4, except that I find it to be more readable code-wise:

$num_images = is_array($form_state['values']['iids']) ? count(array_filter($form_state['values']['iids'])) : 0;
to replace on line 349:
$num_images = count(array_filter($form_state['values']['iids']));

Please keep us posted, if you find the real reason why $form_state is not always an array.

I may have found the culprit. Look in image_attach_form_alter() on line 252:

if ($may_attach_existing || $has_existing_images) {
(...)

This statement wraps the entire assignment of $form['image_attach']['iids']. This means that if there are no existing images, and the user cannot attach existing images, then 'iids' never gets set at all.

EDIT: I just confirmed this.

I also tried setting $form['image_attach']['iids'] to an empty array, but that didn't work.

I'm not sure what value this should actually hold in this particular case. (The user hasn't uploaded an image yet, there isn't an image already attached, and since you can't attach existing images, it should not be the "None" value.) It might be better just to test that it's set in the validation function.

> This means that if there are no existing images, and the user cannot attach existing images, then 'iids' never gets set at all.

You mean no images attached on the node being edited, rather than no image nodes existing at all?
I've just set this up, tried saving a node like this both as an authenticated user and anonymous user, and I still don't get the error message.

Here's how I did it:

1. Created my custom node with no images attached to it.
2. Made sure that image_attach was enabled for this node type, but that "Attach existing images" was not.
3. Edit the node, hit "Preview."
4. warning: array_filter() [function.array-filter]: The first argument should be an array...

I have the Devel module installed, and I can see that in the node itself, "iids" is set to an empty array. $form['#node'] also contains the "iids" field, also an empty array. Therefore, $has_existing_images is FALSE.

Since "Attach existing images" is disabled, $may_attach_existing is also FALSE.

One or the other must be true to set the "iids" array, but since they're not, $form['image_attach']['iids'] is never even defined.

...But here's the odd thing: I've tried this on the core content types, and can't reproduce the error, even though I should be able to. This should happen with ANY node type, where attached images are allowed, but attaching existing images is not, and there are no images actually attached to the node.

I'll dig a bit more to find out why that is.

But, honestly, checking to see if an array exists is a standard technique, and it does fix this problem.

Aha!

One more condition needs to happen: The maximum number of images must be a finite number.

The entire validation function is wrapped in this conditional (on line 347):

if ($maximum_images = variable_get('image_attach_maximum_' . $form['#node']->type, 0)) {

It took me a bit to figure this out, but it's a check to make sure that the maximum number of images is something other than zero (unlimited images).

In other words, if the number of attached images is set to "Unlimited," no validation occurs at all.

I had it set to one image on my custom node type, but I've just confirmed it with the core Page content type as well.

So, to reproduce the problem:

1. Go to admin/settings/image/image_attach and set "Attach existing images" to "Disabled."
2. Enable attached images on a content type.
3. Set the maximum number of images to something other than "Unlimited."
4. Create a node of that content type, but don't attach any images. Hit either "Save" or "Preview."
5. warning: array_filter() [function.array-filter]: The first argument should be an array...
6. ?
7. Profit!

Wow! Thank you so much for debugging that, Karlheinz. I can finally reproduce this! :)

I've found you don't actually need step 1. So:

# Enable attached images on a content type.
# Set the maximum number of images to something other than "Unlimited."
# Create a node of that content type, but don't attach any images. Hit either "Save" or "Preview."
# warning: array_filter() [function.array-filter]: The first argument should be an array...

It's late and I'm falling asleep, so not sure how to tackle it. Probably just a check on isset() in the validation...

Hm. If it were me, I would actually just rewrite the image_attach_validate() function altogether. Here's what I'd change:

1. It seems like the only thing you're doing in this function is handling file uploads. Since that is the case, I'd change the conditional on line 347 to check $form_state['clicked_button']['#value'] instead. This would also eliminate the conditional on line 389, and make $uploading_new_image totally unnecessary.
2. Check to make sure $form_state['values']['iids'] is set before assigning its count to $num_images.
3. On line 378, I would run the $_POST values through check_plain().
4. The comment on line 346 is inaccurate, since $form_state['values']['iids'][0] is not filtered out in this function. Is it actually supposed to be? If so, I would check for it, and subtract 1 from $num_images if true.

While we're at it, I would also change line 402 to make sure $form_state['values']['iids'][0] is set before unsetting it.

I uploaded a patch here. It's a unified diff file between two local files, created with TortoiseSVN, so I don't know if the format is entirely correct. (Bear with me, I'm a noob.)

Thanks for the patch.

I will need to sit down with the existing code and carefully go over it, but in the meantime, giving it a simple readthrough it does seem this patch is changing too many things. Simpler patches are easier to commit! :)

@@ -343,17 +343,20 @@
-  // Validate the number of attached images. Filter out the 'None' with array_filter.
-  if ($maximum_images = variable_get('image_attach_maximum_' . $form['#node']->type, 0)) {

I don't understand why this check is getting removed. The point of this if() is that we only need to check how many images there are if there is a maximum set.

@@ -343,17 +343,20 @@
+    $maximum_images  = variable_get('image_attach_maximum_' . $form['#node']->type, 0);

This will sometimes be 0... yet later on we seem to only be checking things are less than it.

@@ -343,17 +343,20 @@
+    $num_images      = isset($form_state['values']['iids']) ? count(array_filter($form_state['values']['iids'])) : 0;
+    // Filter out the "None" value from the count
+    $num_images      = isset($form_state['values']['iids'][0]) ? $num_images-- : $num_images;

Can't we just take 1 off the count()?

@@ -375,20 +378,18 @@
+      $image_title = $_POST['image_title'] ? check_plain($_POST['image_title']) : basename($file->filepath);

Check_plain is not run on data saved, only on output. If I've misunderstood and it is needed, separate issue please.

@@ -375,20 +378,18 @@
-      $image = image_create_node_from($file->filepath, $image_title, '');
+      $image = image_create_node_from($file->filepath, $image_title);

What's the last param for? Doesn't image_create_node_from() have sane defaults?

@@ -375,20 +378,18 @@
-        drupal_set_message(t("Created new image to attach to this node. !image_link", array('!image_link' => l($image_title, 'node/'. $image->nid) )));
+        $message = t("Created new image to attach to this node. !image_link", array('!image_link' => l($image_title, 'node/'. $image->nid)));
+        drupal_set_message($message);

This change doesn't seem to be needed.

Powered by Dreditor.

Hey, Joachim. Thought I'd explain myself.

I don't understand why this check is getting removed. The point of this if() is that we only need to check how many images there are if there is a maximum set.

Actually, you're right about that. I thought that this conditional wrapped the entire validation, including uploading images. I just looked again and I'm wrong. Must have mis-counted curly braces...

In the patch, the validation is instead run only if the user submitted an upload. That makes all the checks to $uploading_new_image unnecessary, which cleans up the code a bit.

This will sometimes be 0... yet later on we seem to only be checking things are less than it.

You're right, that's my mistake. I only took out the $uploading_new_image variable, and forgot to check if $maximum_images was zero.

Can't we just take 1 off the count()?

If $form_state['values']['iids'][0] isn't set, then we shouldn't do that, right? The idea is that this value shouldn't be counted, but all others should? I guess we could unset it (if set) before we counted the iids array, but I don't know if that would screw up the form.

Check_plain is not run on data saved, only on output. If I've misunderstood and it is needed, separate issue please.

I just get really itchy when $_POST values aren't filtered for injection attacks. It probably isn't necessary here, but it never hurts to be cautious.

What's the last param for? Doesn't image_create_node_from() have sane defaults?

It's for the body text, and defaults to an empty string. I actually took it out, you were the one who set it in the first place :).

This change doesn't seem to be needed.

You're right, it just makes it a bit easier to read the code. I figured, hey, since I'm in the neighborhood...

I guess this patch has alterations that go beyond fixing this specific issue. Maybe I shouldn't have made them, but I was just trying to help. If that's bad etiquette, I won't do it again.

To make it up to you, I created a second patch. This one only has the checks to make sure $form_state['values']['iids'] is set, it has no other alterations.

This changes in this patch looks good, but it's not a valid patch file. I guess it was altered manually. Hence, leaving as needs work.

Yeah, I used TortoiseSVN to do a diff file between two local copies, so there was some renaming stuff in there that I took out by hand.

If you know how to create a patch between local copies using Eclipse, I'm all ears.

> You're right, it just makes it a bit easier to read the code. I figured, hey, since I'm in the neighborhood...

Hehe, easily done and very tempting :)
But: http://www.webchick.net/please-stop-eating-baby-kittens

You should be okay with Tortoise do make the patch again between the local files. Alternatively, do a CVS checkout with Eclipse and then replace the module file with yours, then do a CVS diff.

Glad to see that there was actually a minor problem, as opposed to finding out I had lost my mind somehow. I apologize for not posting feedback sooner - have been busy.

I do confirm that the configuration of the node causing the problem had the "Attach Existing Images" feature disabled. However, the nodes which caused the problem actually did not have the "Maximum Number of Images" property set to "Unlimited", it was set to "10".

Settings my site configuration uses for image attach:
Attach existing images: Disabled

Settings my problem nodes use:
Attach images: Enabled
Maximum number of images: 10
Teaser image size: Thumbnail
Full node image size: Thumbnail

However, I do note that several of my other nodes have the attach images feature disabled, but also have the maximum number of images set to unlimited. I doubt this could have any effect, I simply note it as an observation.

My site does not use any abnormal 3rd party modules. The nodes that my problems occur on are the standard forum nodes that come with the core drupal installation, with only the images module additionally enabled.

Okay, obviously doing an SVN diff between two local files isn't the right way to create a patch file.

So, I checked out the HEAD code, with the intent to do a patch between the local and remote copies (via CVS from within Eclipse).

But, when I got the HEAD code, it appears that the changes were already applied!

Can you confirm this, or did I just screw up the CVS checkout on my end? (The latter is entirely possible, I'm no CVS expert.)

You've done something weird at your end.

My CVS HEAD has:

  unset($form_state['values']['iids'][0]);

which is the line your patch removes.

An SVN diff should be okay, but the trickier part is doing it from within the main image folder.

'Twas indeed a problem on my end. I've fixed it now.

I made a new patch file, which is attached.

The thing is, I think it also counted all of the places where Eclipse trimmed the whitespace off of line endings, even though I checked "ignore white space" in the settings. I'm not sure what to do about that.

Other than that it should be good to go.

EDIT: To be clear, this is a patch against HEAD, not the beta version.

Regarding whitespace -- one of you Eclipse users could maybe post a purely whitespace-fixing patch and then we'd be done with these errors once and for all :D

Just to confirm: the only actual change is this chunk, right?

+++ image_attach.module	20 Apr 2010 22:53:46 -0000
@@ -397,9 +397,11 @@
  * Extra submit handler for node forms.
  */
 function image_attach_node_form_submit(&$form, &$form_state) {
-  // Clear the 0 key in the iids array that arises from selecting the 'None' 
+  // Clear the 0 key in the iids array that arises from selecting the 'None'
   // option. We do this here so image_attach_nodeapi() gets clean data.
-  unset($form_state['values']['iids'][0]);
+  if (isset($form_state['values']['iids'][0])) {
+    unset($form_state['values']['iids'][0]);
+  }
 }
 
 /**

Powered by Dreditor.

That section, and also this section:

@@ -346,24 +346,24 @@
   // Validate the number of attached images. Filter out the 'None' with array_filter.
   if ($maximum_images = variable_get('image_attach_maximum_' . $form['#node']->type, 0)) {
     $uploading_new_image = ($form_state['clicked_button']['#value'] == t('Attach'));
-    $num_images = count(array_filter($form_state['values']['iids']));
-    
+    $num_images = is_array($form_state['values']['iids']) ? count(array_filter($form_state['values']['iids'])) : 0;
+

...followed by a bunch of lines that had extra whitespace at the end.

Incidentally: By default, Eclipse trims whitespace at the end of lines whenever you save the file. You can turn this off, but I didn't want to do that, because trailing whitespace is actually against Drupal's coding standards:
http://drupal.org/coding-standards#indenting

If you're not joking, I'd be more than happy to do a "whitespace patch." It's just a matter of opening and re-saving the files, should take me like 10 minutes or so. Give me the go-ahead and I'll create an issue/patch right now.

I wasn't joking, but I also realized that I know regexps... and as such I can clean up the whitespace with a quick trip to the find & replace in my text editor :)

I've just committed #777966: Clean up whitespace in image attach. Do you think you could roll your patch again against the latest CVS HEAD, so it has just your actual changes?

The CVS HEAD still has the whitespace. Did you actually commit the changes, or just make the patch?

Anyway - for right now, I disabled the automatic whitespace deletion in Eclipse, and made the attached patch. I double-checked, and it only has the actual code changes, not the whitespace changes.

It's patched against the current HEAD. Actually it should work with even with the "non-whitespace" version of HEAD, since the portions I changed didn't have extra whitespace anyway.

Apologies for jumping in here, but I found my way to this thread and wonder if it's similar to the problem I'm experiencing.

I'm using node_import, so programmatically creating nodes, and I get:

Cannot unset string offsets in /home/kil10000/public_html/bka.org.uk/sites/all/modules/image/contrib/image_attach/image_attach.module on line 345

Line 345 is
unset($node->iids[0]);

I'm running a slightly earlier beta (6.x-1.0-beta2) version. Will this patch fix this as well (after upgrading)?

Cheers
Graham

That rings a bell... try doing a search on node_import though, as it's maybe a bug we fixed earlier. Also, upgrade to the latest beta.

The steps in #17 no longer produce the bug.

This must have been fixed by other work on image attach form validation.

I can't reproduce with the steps in #17 any more.

Yup, with that extra step I can reproduce this.

subscribe..

I don't think we need the second part of the patch in #31.

Here's a reroll. Please test :)

Test failure is caused by an unrelated problem: #916128: Failed test: Undefined variable: uploading_new_image.

Back to needs review.

#40: 747900.image-attach-validate-array.patch queued for re-testing.

Gah I just fixed that!

Does anyone know if there's a delay between the test setup getting the latest code?

#40: 747900.image-attach-validate-array.patch queued for re-testing.

#40: 747900.image-attach-validate-array.patch queued for re-testing.

Okay, test bot is having issues.

Could some humans please try this?

The patch does not apply to DRUPAL-6--1 -- perhaps was against HEAD?

EDIT: Note that I didn't test *anything*!

Oh.

It's because the other bug I fixed (#916128: Failed test: Undefined variable: uploading_new_image, which I found when the test here failed) affected the line just above it. Doh.

Thanks for testing!

If you've tested the patch and it works, the correct issue status is 'RTBC'.

It gets set to fixed once a maintainer (that's sun or me) commits the patch. Which I will do soon.

Patch committed.

#747900 by Karlheinz, joachim, sun: Fixed warning when no images are attached.

Hehe, *now* we set it to 'fixed' :)