Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Note: you will need to include $form['#attributes'] = array('enctype' => "multipart/form-data"); in your form.
Is there any reason why the forms engine can't do this automatically upon finding a file field in the structured array? It would really save a ton of bother, and quite some time spent bug-hunting...
Comment | File | Size | Author |
---|---|---|---|
#39 | 137932_enctype_cleanup.patch | 3.87 KB | grendzy |
#37 | 137932_enctype_cleanup.patch | 3.87 KB | grendzy |
#30 | issue-137932-30.patch | 4.08 KB | Alan D. |
#22 | issue-137932-22.patch | 4.1 KB | Alan D. |
#20 | issue-137932-enctype.patch | 1.92 KB | Alan D. |
Comments
Comment #1
cburschkaForm API still requires #attributes['enctype'] to be set. This logic could still be centralized.
Comment #2
Wim LeersForgetting to set that has bitten me once or twice… so I'd love to get this done automatically :) Subscribing.
Comment #3
Alan D. CreditAttribution: Alan D. commentedThis would be great functionality, but a quick look didn't really show a nice simple way to implement this.
One possible solution is to check for a new attribute from the file info array using
_form_builder_handle_input_element
. This required passing the static form version by reference, and adding a couple of checks. Personally, this didn't seem like a great approach.I've attached a patch, but this has had NO tests run with it installed.
Comment #4
Darren OhWe already do something similar to set the #cache property. To keep things simple, here's a patch that sets the enctype attribute the same way. Tested in Drupal 6.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhy not, but I suppose you will have to make
$file
a static then. In its current shape, your code cannot work (except of course if you can find an element that satisfies$form['type'] == 'form' && $form['type'] == 'file'
).Comment #6
Darren Oh$file was supposed to be a static variable. Missed it somehow.
Comment #7
Alan D. CreditAttribution: Alan D. commentedIs it worth making this more generic to allow other non-core widgets to trigger the switch? I think that there are about four accepted enctypes in Web Forms 2.0
Something along the lines of:
This would allow any hook_elements to set any required enctype.
Comment #8
Darren OhSounds good. You've been using Drupal for a while now. Why not contribute a patch?
Comment #9
Alan D. CreditAttribution: Alan D. commentedLOL, I didn't have a D6 CVS install for doing any testing. Anyway, here are the patches against D7 head & D6 dev.
The only testing has been to enable user pictures, remove the line in user.pages.inc:
<?php $form['#attributes']['enctype'] = 'multipart/form-data'; ?>
Then checked that the attribute was being passed through.
Comment #10
Dave Reid+1 on this feature. Will test shortly.
Comment #11
lilou CreditAttribution: lilou commentedenctype-d7-dev.patch no longer applies. Reroll against CVS/HEAD.
Comment #12
cburschkaTested - I hope this can get in soon! :)
Comment #13
Darren OhComment #14
Dries CreditAttribution: Dries commentedThis looks like a nice clean-up. But, wouldn't the static caching cause problems on pages with multiple forms? The $file-field would be set on subsequent forms.
I assume this change is covered by the existing file-related tests?
Comment #15
webchickIt looks like this got accidentally committed in http://drupal.org/cvs?commit=153116 (the "Remove t() from Schema API" patch) (thanks for swentel for noticing, and damZ for the detective work! :))
So... I'm not sure if I should make this "fixed" or "code needs work" based on Dries's comments above. I'm going to go with "code needs work." :)
Also, it introduced a notice which I committed a fix for in #334732: Notices in form.inc in form_builder line 948.
Comment #16
cburschkaI'm slightly confused... how were the t()-schema and this form-api patch related?
Comment #17
cburschkaOh, never mind - you mean it got committed along with it, even though they were unrelated. That's annoyingly hard to track down. ;)
Edit: Incidentally, as far as I know forms without files can be sent as multipart with no problem, so this static cache bug should at most cause avoidable overhead, no critical problems.
Comment #18
Alan D. CreditAttribution: Alan D. commentedIn relation to Dries comment, maybe it would be best to unset
$enctype
similar to$cache
:Should a new patch be created to take into account the existing patch already being applied, or will the older patch get rolled back first?
Comment #19
lilou CreditAttribution: lilou commentedReroll and add Alan remark.
Comment #20
Alan D. CreditAttribution: Alan D. commented$file and $enctype related to different versions of the same patch.
Comment #21
cburschkaI'm confused now. This patch adds a static $enctype variable alongside the $file variable - shouldn't the $file be removed?
Comment #22
Alan D. CreditAttribution: Alan D. commented@Arancaytar Yes
I forgot to add the changes to the system file element. This patch also removes the attributes enctype from being set in the user, theme and upload forms.
Comment #23
lilou CreditAttribution: lilou commented@Alan :
static $complete_form, $cache, $file, $enctype;
Static $file was added in rev 1.303.
Comment #24
Alan D. CreditAttribution: Alan D. commentedNow I'm getting confused. Wasn't that commit that added $file related to a mistaken commit of an earlier patch by webchick, http://drupal.org/node/137932#comment-1109140
Comment #25
lilou CreditAttribution: lilou commented@alan : rev 1.303 -> 1.304 in #332123: Remove t() from all schema descriptions
static $file was commited in rev 1.303 :
Comment #26
Alan D. CreditAttribution: Alan D. commented@lilou This part of that commit relates to http://drupal.org/files/issues/form.inc-137932-6_HEAD.patch in http://drupal.org/node/137932#comment-978915
Comment #27
Bartezz CreditAttribution: Bartezz commentedsubscribing
Comment #28
Alan D. CreditAttribution: Alan D. commentedHi all
The earlier applied patch in Drupal 7 will happily handle form elements of type file and add the correct enctype to the form.
The question is, should this be marked as closed?
The latter patch removes the coupling to the form file element and makes this more generic. This would allow other elements to manually insert a file based HTML widget without needing to add a file child element. (Albeit, IMHO they should be using the FAPI for this.) And in the very rare case of requiring an enctype of "text/plain", this would still require the enctype attribute of the form to be set. I've only ever needed once in about 10 years of playing around with web forms, for a third party integration with some SMS software.
Comment #30
Alan D. CreditAttribution: Alan D. commentedUpdated patch 22 above.
Comment #32
Alan D. CreditAttribution: Alan D. commentedLooks like the eclipse patches are failing here. I'm going to leave this for someone else if this feature is required.
Comment #33
Dave ReidThink some of this patch was committed accidentally...
See http://drupal.org/node/337820#comment-1122629
Comment #34
webchickSheesh. :( I forgot to cvs up -dPC before committing #337820: Rename menu path 'logout' to 'user/logout' for consistency :( I was taking a look at this patch earlier to see if I could replicate the syntax errors locally (which I can't).
Anyway, reverted for now. Really sorry about the headache, folks. :(
Comment #35
drewish CreditAttribution: drewish commentedsubscribing.
Comment #36
grendzy CreditAttribution: grendzy commentedIt looks like maybe this patch wasn't fully backed out? This is still in HEAD:
(Not really complaining, it's very nice that FAPI handles this)
Comment #37
grendzy CreditAttribution: grendzy commentedHere's a clean-up patch that removes all the extra $attributes assignments. I didn't merge the #enctype stuff back in, though I can see the utility of it, in case someone needs to use application/x-www-form+xml or something.
Comment #39
grendzy CreditAttribution: grendzy commentedreroll.
Comment #40
cburschkaI see no obvious problems, but a second set of eyes would be good.
Comment #41
Alan D. CreditAttribution: Alan D. commentedSame, it would be nice to get this closed...
Comment #42
Darren OhLooks good.
Comment #44
webchickComment #45
Dries CreditAttribution: Dries commentedCommitted. Thanks.