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.
Comment | File | Size | Author |
---|---|---|---|
#34 | image.validate.patch | 20.78 KB | sun |
#31 | image-298702-31.patch | 10.57 KB | sp3boy |
#27 | image.validate.patch | 8.23 KB | sun |
#25 | image-298702-25.patch | 7.98 KB | sp3boy |
#24 | image.test-upload-validation.patch | 1.05 KB | sun |
Comments
Comment #1
sp3boy CreditAttribution: sp3boy commentedAlthough 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 ofimage_form_validate()
is returning TRUE when you did not expect it to.Comment #2
sp3boy CreditAttribution: sp3boy commentedThought as no follow-up has occurred, I'd change the status to reflect my findings.
Comment #3
smk-ka CreditAttribution: smk-ka commentedThat was indeed a good idea ;)
Easy fix.
Comment #4
sp3boy CreditAttribution: sp3boy commentedI 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
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():
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.
Comment #5
smk-ka CreditAttribution: smk-ka commentedAfter 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?
Comment #6
smk-ka CreditAttribution: smk-ka commentedComment #7
sp3boy CreditAttribution: sp3boy commentedI 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!
Comment #8
sp3boy CreditAttribution: sp3boy commentedOK 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 functionimage_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.
Comment #9
sp3boy CreditAttribution: sp3boy commentedJust 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.
Comment #10
batbug2 CreditAttribution: batbug2 commented+1 for the fix to this issue. This is critical.
Comment #11
iLLin CreditAttribution: iLLin commentedJust 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.
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.
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
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.
Comment #12
iLLin CreditAttribution: iLLin commentedBetter 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
Comment #13
sp3boy CreditAttribution: sp3boy commentedThanks 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.
Comment #14
drewish CreditAttribution: drewish commentedTried 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.
Comment #15
sp3boy CreditAttribution: sp3boy commentedThanks for spending some time on this. I will try to make time to re-roll the patch in the next few days.
Comment #16
sp3boy CreditAttribution: sp3boy commentedHere's the patch with all the updates on image.module revisions 1.279 to 1.282. I tested it against HEAD today.
Comment #17
sp3boy CreditAttribution: sp3boy commentedForgot status change...
Have changed the assignee too.
Comment #18
iLLin CreditAttribution: iLLin commentedMy 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-
Comment #19
iLLin CreditAttribution: iLLin commentedYou know, now that I'm thinking about it, you could add in those default values when you alter the form. Like so:
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!
Comment #20
sp3boy CreditAttribution: sp3boy commentedPatch against latest HEAD.
Comment #21
sunI do not believe we have to define those form callbacks, as FAPI will execute those default callbacks anyway (if defined).
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.
OT for this patch: I think image_get_info() should already provide the aspect ratio.
This comment has no value, please describe the what's and why's instead.
Comment #22
sp3boy CreditAttribution: sp3boy commentedTaking the points in order:
Comment #23
sp3boy CreditAttribution: sp3boy commentedAlways forget the status change.
Comment #24
sunAttached 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.
Comment #25
sp3boy CreditAttribution: sp3boy commentedAdditional patch added.
Comment #26
sunTest 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:
This a) needs to be fixed and b) has to be covered in the test.
Comment #27
sunComment #28
sp3boy CreditAttribution: sp3boy commentedI 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?
Comment #29
sunStrange. 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)
Comment #30
sp3boy CreditAttribution: sp3boy commentedI 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?Comment #31
sp3boy CreditAttribution: sp3boy commentedI'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.
Comment #32
Encarte CreditAttribution: Encarte commentedsubscribing
Comment #33
sun#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.
Comment #34
sunCommitted attached patch to 6.x.
Any takers for a 5.x-2.x backport?
Comment #35
Hetta CreditAttribution: Hetta commentedsorry, can't backport - this looks way too complicated for my php skills.
Comment #36
sp3boy CreditAttribution: sp3boy commentedGreat 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.
Comment #37
sunIdeally, 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.