Javascript based uploads do not properly support hook_form_alter
mkruisselbrink - March 27, 2008 - 03:37
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | upload.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Description
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| uploadjs.diff | 773 bytes | Idle | Failed: Failed to install HEAD. | View details | Re-test |
#1
btw, the same issue seems to apply to 7.x as well, although I haven't actually tested that.
#2
change version to 7.x as the bug exists in both 6.x and 7.x
#3
updated patch with no more trailing whitespace
#4
update patch to cleanly apply after string-concatenation-operator spacing changes.
#5
Patch 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.
#6
mkruisselbrink, do you have a simple example module that we could use to test this out?
#7
Attached 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.
#8
sorry 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.
#9
i can confirm that the fix works but i don't know enough about the code to say that it's the correct fix.
#10
The last submitted patch failed testing.
#11
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#12
Marking this issues as reviewed, as after all it was reviewed and no problems were found.
#13
Rule number 1: never, ever, mark your own patches as RTBC.
#14
To 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.
#15
that 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.
#16
@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'.
#17
CNW as per #14 and #16.