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...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka’s picture

Version: 5.x-dev » 7.x-dev

Form API still requires #attributes['enctype'] to be set. This logic could still be centralized.

Wim Leers’s picture

Forgetting to set that has bitten me once or twice… so I'd love to get this done automatically :) Subscribing.

Alan D.’s picture

Status: Active » Needs work
FileSize
5.5 KB

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.

Darren Oh’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
1.09 KB

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.

Damien Tournoud’s picture

Status: Needs review » Needs work

Why 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').

Darren Oh’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
1.39 KB

$file was supposed to be a static variable. Missed it somehow.

Alan D.’s picture

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');
?>
Darren Oh’s picture

Sounds good. You've been using Drupal for a while now. Why not contribute a patch?

Alan D.’s picture

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.

Dave Reid’s picture

+1 on this feature. Will test shortly.

lilou’s picture

FileSize
2.13 KB

enctype-d7-dev.patch no longer applies. Reroll against CVS/HEAD.

cburschka’s picture

Tested - I hope this can get in soon! :)

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

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?

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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.

cburschka’s picture

I'm slightly confused... how were the t()-schema and this form-api patch related?

cburschka’s picture

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.

Alan D.’s picture

In relation to Dries comment, maybe it would be best to unset $enctype similar 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?

lilou’s picture

Status: Needs work » Needs review
FileSize
2.45 KB

Reroll and add Alan remark.

Alan D.’s picture

FileSize
1.92 KB

$file and $enctype related to different versions of the same patch.

cburschka’s picture

Status: Needs review » Needs work

I'm confused now. This patch adds a static $enctype variable alongside the $file variable - shouldn't the $file be removed?

Alan D.’s picture

Status: Needs work » Needs review
FileSize
4.1 KB

@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.

lilou’s picture

@Alan :

static $complete_form, $cache, $file, $enctype;

Static $file was added in rev 1.303.

Alan D.’s picture

Status: Needs review » Needs work

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

lilou’s picture

@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;
 }
Alan D.’s picture

Bartezz’s picture

subscribing

Alan D.’s picture

Status: Needs work » Needs review

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Alan D.’s picture

Status: Needs work » Needs review
FileSize
4.08 KB

Updated patch 22 above.

Status: Needs review » Needs work

The last submitted patch failed testing.

Alan D.’s picture

Looks like the eclipse patches are failing here. I'm going to leave this for someone else if this feature is required.

Dave Reid’s picture

Think some of this patch was committed accidentally...
See http://drupal.org/node/337820#comment-1122629

webchick’s picture

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. :(

drewish’s picture

subscribing.

grendzy’s picture

It looks like maybe this patch wasn't fully backed out? This is still in HEAD:

    if (isset($file)) {
      $form['#attributes']['enctype'] = 'multipart/form-data';
    }

(Not really complaining, it's very nice that FAPI handles this)

grendzy’s picture

Status: Needs work » Needs review
FileSize
3.87 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

grendzy’s picture

Status: Needs work » Needs review
FileSize
3.87 KB

reroll.

cburschka’s picture

I see no obvious problems, but a second set of eyes would be good.

Alan D.’s picture

Same, it would be nice to get this closed...

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.