Posted by cburschka on April 20, 2007 at 4:57pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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...
Comments
#1
Form API still requires #attributes['enctype'] to be set. This logic could still be centralized.
#2
Forgetting to set that has bitten me once or twice… so I'd love to get this done automatically :) Subscribing.
#3
This 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.
#4
We 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.
#5
Why not, but I suppose you will have to make
$filea 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').#6
$file was supposed to be a static variable. Missed it somehow.
#7
Is 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:
<?php
static $complete_form, $cache, $enctype;
...
// If an element requires to set the forms content type enctype
// attribute, we need to store this info in a static $enctype
// flag to update the parent form element.
// E.g. For files, non-ASCII data, and binary data.
if (isset($form['#enctype'])) {
$enctype = $form['#enctype'];
}
if (isset($form['#type']) && $form['#type'] == 'form') {
// We are on the top form, we can copy back #cache if it's set.
if (isset($cache)) {
$form['#cache'] = TRUE;
}
// If there is a file element, we set the form encoding.
if (isset($enctype)) {
$form['#attributes']['enctype'] = $enctype;
}
}
?>
This would allow any hook_elements to set any required enctype.
<?php$type['file'] = array('#input' => TRUE, '#size' => 60, '#enctype' => 'multipart/form-data');
?>
#8
Sounds good. You've been using Drupal for a while now. Why not contribute a patch?
#9
LOL, 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.
#10
+1 on this feature. Will test shortly.
#11
enctype-d7-dev.patch no longer applies. Reroll against CVS/HEAD.
#12
Tested - I hope this can get in soon! :)
#13
#14
This 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?
#15
It 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.
#16
I'm slightly confused... how were the t()-schema and this form-api patch related?
#17
Oh, 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.
#18
In relation to Dries comment, maybe it would be best to unset
$enctypesimilar to$cache:<?php// About line 866
if (isset($form['#type']) && $form['#type'] == 'form') {
$cache = NULL;
$enctype = NULL;
$complete_form = $form;
?>
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?
#19
Reroll and add Alan remark.
#20
$file and $enctype related to different versions of the same patch.
#21
I'm confused now. This patch adds a static $enctype variable alongside the $file variable - shouldn't the $file be removed?
#22
@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.
#23
@Alan :
static $complete_form, $cache, $file, $enctype;Static $file was added in rev 1.303.
#24
Now 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
#25
@alan : rev 1.303 -> 1.304 in #332123: Remove t() from all schema descriptions
-// $Id: form.inc,v 1.303 2008/11/15 13:01:04 dries Exp $
+// $Id: form.inc,v 1.304 2008/11/15 15:32:36 webchick Exp $
/**
* @defgroup forms Form builder functions
@@ -945,7 +945,7 @@
}
// If there is a file element, we need to flip a static flag so later the
// form encoding can be set.
- if ($form['#type'] == 'file') {
+ if (isset($form['#type']) && $form['#type'] == 'file') {
$file = TRUE;
}
if (isset($form['#type']) && $form['#type'] == 'form') {
static $file was commited in rev 1.303 :
In D:\Serveur\www\drupal\7.x\includes: "C:\Program Files\CVSNT\cvs.exe" -q diff -u -r 1.302 -r 1.303
form.inc
CVSROOT=:pserver:anonymous@cvs.drupal.org:/cvs/drupal
Index: form.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/form.inc,v
retrieving revision 1.302
retrieving revision 1.303
diff -u -r1.302 -r1.303
--- form.inc 10 Nov 2008 05:22:59 -0000 1.302
+++ form.inc 15 Nov 2008 13:01:04 -0000 1.303
@@ -1,5 +1,5 @@
<?php
-// $Id: form.inc,v 1.302 2008/11/10 05:22:59 webchick Exp $
+// $Id: form.inc,v 1.303 2008/11/15 13:01:04 dries Exp $
/**
* @defgroup forms Form builder functions
@@ -852,7 +852,7 @@
* $_POST data.
*/
function form_builder($form_id, $form, &$form_state) {
- static $complete_form, $cache;
+ static $complete_form, $cache, $file;
// Initialize as unprocessed.
$form['#processed'] = FALSE;
@@ -943,9 +943,20 @@
if (!empty($form['#cache'])) {
$cache = $form['#cache'];
}
- // We are on the top form, we can copy back #cache if it's set.
- if (isset($form['#type']) && $form['#type'] == 'form' && isset($cache)) {
- $form['#cache'] = TRUE;
+ // If there is a file element, we need to flip a static flag so later the
+ // form encoding can be set.
+ if ($form['#type'] == 'file') {
+ $file = TRUE;
+ }
+ if (isset($form['#type']) && $form['#type'] == 'form') {
+ // We are on the top form, we can copy back #cache if it's set.
+ if (isset($cache)) {
+ $form['#cache'] = TRUE;
+ }
+ // If there is a file element, we set the form encoding.
+ if (isset($file)) {
+ $form['#attributes']['enctype'] = 'multipart/form-data';
+ }
}
return $form;
}
#26
@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
#27
subscribing
#28
Hi 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.
#29
The last submitted patch failed testing.
#30
Updated patch 22 above.
#31
The last submitted patch failed testing.
#32
Looks like the eclipse patches are failing here. I'm going to leave this for someone else if this feature is required.
#33
Think some of this patch was committed accidentally...
See http://drupal.org/node/337820#comment-1122629
#34
Sheesh. :( 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. :(
#35
subscribing.
#36
It looks like maybe this patch wasn't fully backed out? This is still in HEAD:
<?phpif (isset($file)) {
$form['#attributes']['enctype'] = 'multipart/form-data';
}
?>
(Not really complaining, it's very nice that FAPI handles this)
#37
Here'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.
#38
The last submitted patch failed testing.
#39
reroll.
#40
I see no obvious problems, but a second set of eyes would be good.
#41
Same, it would be nice to get this closed...
#42
Looks good.
#43
The last submitted patch failed testing.
#44
#45
Committed. Thanks.
#46
Automatically closed -- issue fixed for 2 weeks with no activity.