Image node can be created without any image

Having been looking closely at image.module with a view to implementing the functionality of the D5 patch in http://drupal.org/node/103793 against D6, I agree with you.

Although new to Drupal and having never used D5, I think the problem is based around the fact that image_validate is called before image_node_form_submit. If there are no pre-existing session details, the details of the uploaded image (or lack thereof) are not established until the call to file_save_upload in image_node_form_submit. However the existing code in image_validate is merely checking whether $node->images[IMAGE_ORIGINAL] exists, not whether it has any meaningful value.

If there are pre-existing session details (even from an unrelated prior image node creation), I have been seeing the last "good" image upload being re-used to satisfy a new node creation where the upload has been omitted. I think that's because the session details are not cleared on successful node save, perhaps because the code is still in image_submit which is a deprecated hook in D6 as far as I know?

Of course, as I say I'm new to Drupal and don't want to be telling people what they already know!

sp3boy,

ditto, same issues reported (similar) http://drupal.org/node/292603

in a nutshell. Stand alone, I have not experienced nodes being created w/o images. Using the Image Fupload feature, there is a sporadic processed where nodes are created by "Guest" and the Node Type is "blank". Administer > Content

Did you try the patch?

James

If you mean the patch from http://drupal.org/node/103793 , I have made some progress in bending it to fit D6 but have not yet tested all the situations in which images can be uploaded or created by the module and its bundled contribs, e.g. whether image_import still works OK. I was intending to hold off putting a comment on the original issue until I then (1 to 2 days).

Plus there are various bugs like the empty image node one that crop up in my testing to distract me!

It would be useful to know whether someone developing this sort of patch against a module like Image is expected to test against all other contrib modules that depend on Image? I hadn't looked into how many of those there might be but Image_Fupload looks like a good example. Perhaps there are guidelines I should read ASAP?

sp3boy,

Got your take on it. Per your questions:

"It would be useful to know whether someone developing this sort of patch against a module like Image is expected to test against all other contrib modules that depend on Image? "I haven't seen any current ones. There are patches via search which have been developed as you have found

"I hadn't looked into how many of those there might be but Image_Fupload looks like a good example. Perhaps there are guidelines I should read ASAP?" Image Fupload is pretty straight forward. Grandcat (author) has done a great job with it. Beta 3 is up. You may want to try this.

I'm running Core 6x... I haven't had any issues with creating node w/o images on a one by one basis. I am having issues in pics being uploaded (mass upload), but sporadically not all the pics are processed. The pics end up in the Temp folder. If you these issues, let me know.

Regards,
James

As I mentioned already, I'm fairly new to Drupal so apologies if I'm inadvertantly cutting corners or missing any steps. However, here goes...

I was able to reproduce the following scenarios against 6.1-1.0-alpha2 and I'm pretty sure they are still true for alpha3/dev:

1. Image node can be created with no upload
2. Image node that has no uploaded file can be partially edited and re-saved with no image
3. Image node that has no uploaded file can have image file added
4. Adding upload to Image node that has no uploaded file can preview OK but fail on save if edit re-started
5. Creating Image node without upload can reference details of upload from previous saved node
6. Saved image node file can be unexpectedly overwritten after interrupted edit

(Scenario 3 is not exactly a bug but is possible due to scenario 1.)

I have step-by-step test plans for each of these if anyone wants to try them.

The attached patch is my attempt to fix this problem for D6. I have moved the essential validation from image_node_form_submit to a new callback function _image_upload_validate() which gets fired earlier in the processing sequence, allowing form_set_error() to send the user back to the form with the empty upload field correctly highlighted. (As it is a callback it can make changes to $form_state.) There is also the benefit that if a user accidentally leaves the title field blank, any uploaded file is still processed before Drupal reacts to the empty title. Currently I think the upload is "lost".

I have also put in more controls over when the details of a saved file are cleared out on creating a brand new node, and also discarded in image_insert() and image_update(). Additionally the file information is explicitly placed in the session array when an image_load() is called for an edit, so that re-edits of an existing image node don't unexpectedly grab a prior upload file as per scenario 5.

Having tested this patch with image node creation and image import, I took the view that uploading to files/tmp and then creating derivative images in files/images/temp for preview etc. was illogical, especially as image_cron() and system_cron() apply different age checks when clearing these directories.

Hence I have included a change to make the file_save_upload() function put the "original" file straight into files/image/temp (or whatever the configured equivalent of files/image actually is).

I can re-make the patch without this if it would cause unforeseen problems.

Something I would have liked to change but didn't, is to make it optional whether repeated upload of the same file causes multiple versions to be created. The option would pass FILE_EXISTS_REPLACE to file_save_upload() instead of defaulting to "rename". I can see that a site with many users uploading different pictures all called DSCF*.jpg really needs the rename functionality.

See also #298702: Properly validate image uploads. Most code of our patches is quite similar, the only difference is that I've completely removed the temporary $_SESSION storage, and I'm unable to find a use case when it would actually be required. I've tried to create two image nodes in parallel (ie. upload a new image using 'preview' button, switching windows inbetween, then hitting 'save' in each window), and everything went well, ie. the different sessions didn't get mixed. Could you describe a scenario when this wouldn't work?

I did wonder whether $form_state could take the place of $_SESSION but being new to Drupal, FAPI and the Image module I decided not to go that far. Certainly the fact that $_SESSION persists "outside" of Drupal did not seem to be an advantage when I was looking into the bug.

I'll try to look more closely at your patch in the next two or three days.

Just FYI I have built another patch for http://drupal.org/node/103793 that "sits" on top of my patch for this problem.

I have to quickly say that the ability to save an image node without an image uploaded can actually be useful. Say for example, you use image galleries for videos as well. Uploading the video or embedding the video in an image node makes it easier to sort them into galleries, or have them show up in a Views page.

Its the closest solution to a multimedia gallery I have found. It would be great if there was a single module that could accept images, videos, and music files and organize them into galleries and such, but until then this patch would ruin my solution, so if it is implemented in the module itself, please make it optional.

I wouldn't have thought it were possible to upload a video / audio file into an image node using the unpatched D6 Image module. Any file that is uploaded has to pass through validation by file_validate_is_image() which only allows JPEG, PNG and GIF. Even if someone deliberately creates and saves an image node that initially has no associated file, the video / audio file has to be uploaded at some point and the existing validation would apply, would it not?

The point of this patch and the alternative in #298702: Properly validate image uploads is that under D5 (as I understand it) you're not supposed to be able to create an image node without an associated file. The same should have been true of D6 but currently is not due to the validation being in the wrong place for the D6 process flow. Hence this/these patches qualify as bug fixes. I think you are suggesting that the mandatory validation in D5 is formally made optional for D6?

Having done as much testing on #298702: Properly validate image uploads as on my patch, I think #298702 is the way to go, subject to my latest comments on that issue which aren't really about the Image module itself.

I hesitate to change the status of this issue to closed, duplicate or something similar as I didn't create the issue and there has not been much feedback on #298702: Properly validate image uploads yet apart from by myself??!?

Further to my latest comment in #298702: Properly validate image uploads I have found a problem with my patch in comment 5 above, to do with invalid taxonomy on the form but centred around updating an existing image node with a new (i.e. different) upload. If FAPI throws an error for a mandatory field, the $form_state['values']['new_file'] was getting cleared so when image_update() is executed to save the new node details, the original image was retained even though $form_state['values']['images'] was being retrieved from $_SESSION with the new file details.

Bearing in mind that interest in this issue appears rather low, I'm not going to rush to publish the fix (save new_file in $_SESSION as well as the other stuff) but will look into the idea of taking the approach in #298702: Properly validate image uploads with an absolutely minimal use of $_SESSION.

I'm also wondering how best to merge these two issues as there's a duplication of effort in keeping them both updated?!?

Just added an enhanced patch to #298702: Properly validate image uploads which aims to provide the minimal use of $_SESSION to get round the FAPI limitations. I strongly recommend looking at that rather than my first attempt here.

Marking as duplicate of #298702: Properly validate image uploads