This patch corrects the following issue when creating or editing image nodes: Due to validation taking place in the submit handler, there is no way to prevent nodes without a valid image being created. Currently this can happen by submitting a new node without any image at all, or by uploading an image which exceeds the allowed file size.

This issue has been solved by moving the image validation from #submit into the #validate handler, where it is supposed to be.

Additionally, the old D5-style $_SESSION code has been replaced by state-of-the-art $form_state handling and some debugging code has been removed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sp3boy’s picture

Although your strategy looks neater than mine (in http://drupal.org/292039) and I'd be happy to fall in behind it, I have applied your patch to image.module and was still able to create Image nodes without uploads.

To reproduce:
1 Start at /node/add/image.
2 Enter title.
3 Click Save.
4 Messages: "Image -title- has been created." and "The selected file -drupal root- could not be copied."

I suspect that your test for empty($form_state['values']['images']) at the end of image_form_validate() is returning TRUE when you did not expect it to.

sp3boy’s picture

Status: Needs review » Needs work

Thought as no follow-up has occurred, I'd change the status to reflect my findings.

smk-ka’s picture

Status: Needs work » Needs review
FileSize
6.56 KB

That was indeed a good idea ;)
Easy fix.

sp3boy’s picture

I have done quite a bit more testing. The patch is validating correctly, I just had one problem which is not really down to the patch at all.

If the Image node is created via /node/add/image and the Title (or a mandatory Taxonomy field) is left unset (assume user oversight), the selected image file is uploaded OK in the new function image_form_validate(), but when the form is rerendered with a suitable error message ('Title field is required' for example), the $form_state['value']['images'] information is "lost". The image file and its derivatives have been created in files/images/temp but the next Save or Preview submit will not receive the $form_state['values']['images'] from the previous invocation of image_form_validate() so the user will get the 'You need to upload an image' error.

Furthermore if they reselect the file to upload, the functionality in the image module will upload the file with an altered filename to avoid overwriting the existing one in /temp, even though as far as the user is concerned no previous upload has happened.

Not being very knowledgeable about FAPI I struggled with this for a couple of days. I tried saving a copy of the

    $form_state['values']['images'][IMAGE_ORIGINAL]
    $form_state['values']['rebuild_images']
    $form_state['values']['new_file']

variables in $form_state['storage'] and retrieving them in a new submit callback function but this caused more problems than it solved. I had to unset $form_state['storage'] at the end of the submit callback otherwise FAPI behaved as if I was constructing a multi-step form, then at one point an error in taxonomy was causing the Title and Body fields to be reset to empty even though the $form_state['storage'] still had the upload file details.

Without spending a lot more time getting into FAPI I am not sure this "problem" can be fixed easily. My best suggestion therefore is to add something like the following check into the very beginning of image_form_validate():

  if (form_get_errors()) {
    form_set_error('image', 'Form has errors - upload not processed.');
    return;
  }

The idea of this is to avoid processing the upload at all if there is an existing error on the form, and to warn the user that once they have corrected the other problems they will need to reselect the upload file.

That saves creating a set of image files in /temp that cannot be used and saves the user clicking Save or Preview and then discovering that the system has forgotten about their first upload attempt.

I guess my own validation fix in #292039: Image node can be created without any image might have a better chance of retaining the original upload details if other parts of the form have errors but the use of $_SESSION is laborious in concept and I'm just a bit frustrated that FAPI does not fully support the approach of saving information in $form_state.

smk-ka’s picture

After fiddling around with this issue I came to the same conclusion: $form_state['storage'] can't be used to store persistent data, at least not in D6 (there are plans to change that in D7). Storing a value in storage triggers a very special FAPI behavior that makes it forget about the most recently submitted values in case of a validation error, and display default values instead (could also be empty).

Seems like $_SESSION needs to get back in. However, I'm wondering if the amount of extra code for session handling in #292039: Image node can be created without any image could be reduced somehow?

smk-ka’s picture

Status: Needs review » Needs work
sp3boy’s picture

I have also been tempted to go back to a $_SESSION approach and was pondering a blend of our two solutions. I spent a bit of time re-testing my fix in #292039: Image node can be created without any image before posting comment 4 above and I had found another problem, again 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.

The cure for that was fairly simple: save the new_file flag in $_SESSION as well as the other stuff. However I didn't update my patch in #292039: Image node can be created without any image because I still favoured the simplicity of yours.

I'll not be able to spend much time on this between now and early next week but it's encouraging that we're still thinking the same way!

sp3boy’s picture

Status: Needs work » Needs review
FileSize
7.25 KB

OK I have gone back to this and added a minimal use of $_SESSION again in image_form_validate(). Details of the uploaded file are stored to $_SESSION and I've added a new submit callback function image_form_submit(), the sole purpose of which is to copy the file info. into $form_state and unset the $_SESSION item.

I also had to add a little bit of code into image_form() to make sure the relevant item in $_SESSION is cleared when a new edit is started, otherwise it's possible that an abandoned upload (for example one where other form errors prevented the submit callback from ever being called) could be picked up by a subsequent Image node creation or edit.

I attach a patch against HEAD with the whole set of changes. If I should have made a patch on the last patch from comment 3, apologies - just let me know.

sp3boy’s picture

Just wondering if there's any chance this patch will get adopted? Do I need to do more to progress it? Every time image.module is changed, I have to check whether my patch will still work.

batbug2’s picture

Priority: Normal » Critical

+1 for the fix to this issue. This is critical.

iLLin’s picture

Just to provide some insight to this issue. Please note that I don't use this module so my information could be completely wrong in your guy's case. Anyway I am creating a custom module that provides file uploads along with other fields that are required. If they uploaded a file and did not fill out a required field, I would "lose" my image information just as you guys seem to be doing. This is what I did after hours and hours of research.

I 'tricked' the form into thinking it was a multistep form if there were errors present. In my validation function I have this at the end.

if(is_array(drupal_set_message())) {
    $form_state['rebuild'] = true;
    $form_state['storage'] = $form_state['values'];
  }

Basically if there are errors in the form them we want to force the form to rebuild. In doing this we store all our values in a storage container. The problem of the image information not storing is the form is built FIRST on submit and cached, THEN your validation function is called, so no matter how I tried to rig it, it would never actually hold my data after the page is submitted due to the caching.

Now since we force the form to rebuild, the form is indeed rebuilt after the validation function and is passed the storage information. Now in your form you just need to modify the '#default_value' attribute accordingly, and I moved the storage back to the post (doubt you HAVE to do that, anyway like this.

if(!empty($form_state['storage'])) {
    $form_state['post'] = $form_state['storage'];
}

$form['title'] = array(
  '#type' => 'textfield',
  '#name' => 'title',
  '#title' => t('Title'),
  '#required' => true,
  '#default_value' => $form_state['post']['title'],
);

I setup hidden fields for the fid and filepath. But this allowed me to "hold" my image information on a form validation error and then I set status to 1 for the image

file_set_status($file, 1);

in my submit function and save all the necessary information to my custom module.

Anyway I thought I would share how I did it without the use of the $_SESSION.

iLLin’s picture

Better error check as I was recieving "status" type messages and was preventing the form from being submitted. Status messages like Your image was resized to wxh... etc. then i would have to submit the form again. Anway I replaced that check at the end of my validation with this

//check for errors and rebuild if necessary
  if(drupal_get_messages('error', FALSE)) {
    $form_state['rebuild'] = true;
    $form_state['storage'] = $form_state['values'];
  }
sp3boy’s picture

Thanks for showing an interest!

I spent quite a bit of time trying to fool D6 with the storage feature but see comment #4 above, particularly about how taxonomy errors were causing the Title and Body fields to be reset to empty even though the $form_state['storage'] still had the upload file details.

For quite a while I thought I had it working but would then come across unacceptable behaviour like that, triggered by using the multi-form functionality in a way it wasn't designed.

The proposed use of $_SESSION is a fair bit simpler than the current (unpatched) version of the image.module was. At least as far as I can remember - it was quite a while ago that I spent all that time on it.

drewish’s picture

Status: Needs review » Needs work

Tried to apply this and 2/3 of the blocks fail. Could you re-roll it? I'd removed the stray dsm() before looking at this patch.

I'd really love to get the $_SESSION out of there but I guess that's not really possible is it? I think taxonomy module caused the same problems for the filefield module.

sp3boy’s picture

Thanks for spending some time on this. I will try to make time to re-roll the patch in the next few days.

sp3boy’s picture

Here's the patch with all the updates on image.module revisions 1.279 to 1.282. I tested it against HEAD today.

sp3boy’s picture

Assigned: smk-ka » sp3boy
Status: Needs work » Needs review

Forgot status change...

Have changed the assignee too.

iLLin’s picture

My form is its own, so I include all the fields myself so its easier for me to make sure the values are passed back if an error occurs. I guess you alter forms with your module and thats the problem? when the form is rebuilt (using multistep), it clears out the post array AND the $form['#post'] as well so when the form is built again it builds with no values set. that is why I had to add in the '#default_values', BUT i also have complete control over my entire form.

Yea looks like your stuck... Hopefully drupal will improve on this aspect.

later-

iLLin’s picture

You know, now that I'm thinking about it, you could add in those default values when you alter the form. Like so:

hook_form_alter() {
  //if this is rebuild code
  $form['taxonomy']['#default_value'] = $storage_value;

I bet if you were to do that it would fix your problems and allow you to "trick" drupal. I'm about 99% sure you can do that.

Anyway good luck!

sp3boy’s picture

FileSize
7.22 KB

Patch against latest HEAD.

sun’s picture

Status: Needs review » Needs work
+  $form['#validate'][] = 'image_form_validate';
+  $form['#submit'][] = 'image_form_submit';

I do not believe we have to define those form callbacks, as FAPI will execute those default callbacks anyway (if defined).

+  $dest = file_create_path(variable_get('image_default_path', 'images') .'/temp');

Please do not use abbreviations for variables. $destination is even misleading I think, since this is the temporary file storage. The variable name should be self-explaining.

+    $image_info = image_get_info($file->filepath);
+    $aspect_ratio = $image_info['height'] / $image_info['width'];
+    $original_size = image_get_sizes(IMAGE_ORIGINAL, $aspect_ratio);

OT for this patch: I think image_get_info() should already provide the aspect ratio.

+    // We're good to go.

This comment has no value, please describe the what's and why's instead.

sp3boy’s picture

Status: Needs review » Needs work
FileSize
7.23 KB

Taking the points in order:

  1. I believe those form callbacks are necessary because as I understood it the default FAPI behaviour would be to call image_validate() which does not receive $form_state and to call image_submit() ... except that's deprecated under D6.
  2. Variable name now changed.
  3. I interpret your comment to mean that no change is actually proposed.
  4. "...good to go." ... gone.
sp3boy’s picture

Status: Needs work » Needs review

Always forget the status change.

sun’s picture

Attached patch by smk-ka adds a test for this functionality. (moved from #298644: Upgrade to SimpleTest 2.x)

Please integrate this test in your patch.

sp3boy’s picture

Status: Needs work » Needs review
FileSize
7.98 KB

Additional patch added.

sun’s picture

Status: Needs review » Needs work

Test was failing due to changed validation error message. Fixed.

Tests are passing, but when I test this manually, i.e. node/add/image, and

- upload no image -> validation error appears

- upload an image, image is attached, but after hitting "Preview" multiple times and "Save" afterwards, I'm getting the following error messages:

# The selected file  could not be copied, because no file by that name exists. Please check that you supplied the correct filename.
# notice: Undefined index: _original in sites\all\modules\image\image.module on line 451.

This a) needs to be fixed and b) has to be covered in the test.

sun’s picture

FileSize
8.23 KB
sp3boy’s picture

I have installed a fresh D6.9 system with Image HEAD and this patch. I have not so far been able to reproduce that problem either with node/image/add or by uploading a new image to attach to a node/add/page dialogue.

Does it happen every time?

sun’s picture

Strange. I do not get this error anymore. It only appeared after final submit.

However, another issue instead: The node I created, which lead to the error message in #26, can no longer be deleted (only via mass-operations on admin/content/node). The error message "you must upload an image" appears. ;) I guess we should exclude this validation from the delete submit handler.

Ideally, we add a test for this, too - called "Verify Image node can still be deleted when image file got lost somehow."

Oh, and just now I realized that this patch also changes Image Attach's upload behavior - can we add a test for that part as well? (if we can, this should go into a separate image_attach.test file, next to image.test)

sp3boy’s picture

I have not used the test module before so I would welcome suggestions as to how to define these other tests.

And // We're good to go. is back in your last patch?

sp3boy’s picture

Status: Needs work » Needs review
FileSize
10.57 KB

I've added a check for node deletion in the validate callback of image.module so that partly-corrupt nodes can still be deleted via the node edit form.

I've also figured out how to test for this case in image.test - the method by which I generate a "corrupt" image node (i.e. a node without any {files} data) is a bit crude. Also, to replicate the user's activity in this case it is necessary to start at node/%nid/edit and then post a separate Delete action - just posting once to node/%nid/delete does not trigger the validation callback.

I've moved the testMissingImage test function up before testCreateNode to give a more logical order of tests.

I've retained all the other changes from comment 27. Except // We're good to go.

This patch should not change the Image Attach upload behaviour as far as I know. Image attach makes the uploaded file "permanent" before the main node is saved, which requires a less complicated upload validation check.

Encarte’s picture

subscribing

sun’s picture

#183422: image_create_node_from creates image if file is too big has been marked as duplicate of this issue. Not sure whether we're already validating the filesize properly, but based on the code in this patch, it seems we do.

sun’s picture

Status: Needs review » Fixed
FileSize
20.78 KB

Committed attached patch to 6.x.

Any takers for a 5.x-2.x backport?

Hetta’s picture

sorry, can't backport - this looks way too complicated for my php skills.

sp3boy’s picture

Great news that the patch was committed.

Is a backport desirable mainly to get the code as close to 6.x as possible, because as far as I can remember the validation problem itself does not affect the 5.x version, due to different FAPI behaviour? I could be wrong there of course.

sun’s picture

Ideally, also functionality-wise (since [form]_validate() and _submit() also existed in D5, but of course, form_state stuff won't fly), but mainly to keep the code as comparable as possible.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.