the upload_js function stores the updated form back into the cache before invoking hook_form_alter, because of this modifications made to the form in hook_form_alter won't work correctly when submitting the form as the form that is stored in the cache is different from the one displayed to the user. The attached patch seems to fix this.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | upload-js-fix.diff | 967 bytes | mkruisselbrink |
| #7 | uploadbugtest.tar.gz | 1.12 KB | mkruisselbrink |
| #4 | upload-js-fix.diff | 1.02 KB | mkruisselbrink |
| #3 | upload-js-fix.diff | 1.02 KB | mkruisselbrink |
| fix to call hook_form_alter before saving the form in the cache | 773 bytes | mkruisselbrink |
Comments
Comment #1
mkruisselbrink commentedbtw, the same issue seems to apply to 7.x as well, although I haven't actually tested that.
Comment #2
mkruisselbrink commentedchange version to 7.x as the bug exists in both 6.x and 7.x
Comment #3
mkruisselbrink commentedupdated patch with no more trailing whitespace
Comment #4
mkruisselbrink commentedupdate patch to cleanly apply after string-concatenation-operator spacing changes.
Comment #5
maartenvg commentedPatch still applies correctly, but with some offset. No Simpletest fails are introduced after applying the patch.
Can you describe step by step how to reproduce the bug, because I haven't been able to check whether it actually fixes the bug? Thnx.
Comment #6
drewish commentedmkruisselbrink, do you have a simple example module that we could use to test this out?
Comment #7
mkruisselbrink commentedAttached a simple example module to test this, and a updated patch against a newer cvs revision. To reporoduce the problem, install this module, and than (without saving in between) upload a new attachment for a node, and enter a value in the Test field for an attachment. Without the patch applied, this will result in some warnings when you save the node; with the patch applied you'll get correct messages showing what values you entered in those fields.
Comment #8
drewish commentedsorry for waiting so long on this but i just got around for it and couldn't test this because i'm getting fatal errors when saving nodes with uploads attached (Fatal error: Exception thrown without a stack frame in Unknown on line 0)... need to do some checking to see if i can figure out the cause.
Comment #9
drewish commentedi can confirm that the fix works but i don't know enough about the code to say that it's the correct fix.
Comment #11
lilou commentedSee: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
Comment #12
mkruisselbrink commentedMarking this issues as reviewed, as after all it was reviewed and no problems were found.
Comment #13
damien tournoud commentedRule number 1: never, ever, mark your own patches as RTBC.
Comment #14
damien tournoud commentedTo be perfectly consistent, I think the call to
drupal_alter('form', <the upload partial form>, array(), 'upload_js');should be moved to_upload_form(). This way, we will insure that the form alter is consistent between javascript and non-javascript mode.This definitely requires more discussion.
Comment #15
mkruisselbrink commentedthat might be a better fix, but it would be a change that potentially breaks/changes behavior of modules that current alter both the node-edit form and the upload_js form, as that change would mean that the already altered upload_js form would be inserted in the node-edit form. If that is okay as far as behavior changes go, I'll make a new patch that does that. I just tried to make this patch as non-intrusive as possible.
Comment #16
damien tournoud commented@mkruisselbrink: as far as I understand, this never worked (ie. the form was always form_altered *after* being cached back), so it can't possibly break anything :)
By the way, if you move the form_alter to _upload_form, please rename the form_id from 'upload_js' to 'upload_form'.
Comment #17
damien tournoud commentedCNW as per #14 and #16.
Comment #18
webchickUpload module has been removed from Drupal 7. Moving down to 6.x.