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

Files: 
CommentFileSizeAuthor
#39 137932_enctype_cleanup.patch3.87 KBgrendzy
Unable to apply patch 137932_enctype_cleanup_0.patch
[ View ]
#37 137932_enctype_cleanup.patch3.87 KBgrendzy
Failed: Failed to install HEAD.
[ View ]
#30 issue-137932-30.patch4.08 KBAlan D.
Failed: Invalid PHP syntax.
[ View ]
#22 issue-137932-22.patch4.1 KBAlan D.
Failed: Invalid PHP syntax.
[ View ]
#20 issue-137932-enctype.patch1.92 KBAlan D.
Failed: Invalid PHP syntax.
[ View ]
#19 issue-137932-19.patch2.45 KBlilou
Failed: Failed to apply patch.
[ View ]
#11 issue-137932-11.patch2.13 KBlilou
Failed: Failed to apply patch.
[ View ]
#9 enctype-d6-dev.patch2.59 KBAlan D.
Failed: Failed to apply patch.
[ View ]
#9 enctype-d7-dev.patch2.08 KBAlan D.
Failed: Invalid PHP syntax.
[ View ]
#6 form.inc-137932-6_DRUPAL-6.patch1.39 KBDarren Oh
Failed: Failed to apply patch.
[ View ]
#6 form.inc-137932-6_HEAD.patch1.37 KBDarren Oh
Failed: Failed to apply patch.
[ View ]
#4 form.inc-137932-4_HEAD.patch1.09 KBDarren Oh
Failed: Failed to apply patch.
[ View ]
#4 form.inc-137932-4_DRUPAL-6.patch1.11 KBDarren Oh
Failed: Failed to apply patch.
[ View ]
#3 enctype.patch5.5 KBAlan D.
Failed: Invalid PHP syntax.
[ View ]

Comments

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

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

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

Status:Active» Needs work
StatusFileSize
new5.5 KB
Failed: Invalid PHP syntax.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.11 KB
Failed: Failed to apply patch.
[ View ]
new1.09 KB
Failed: Failed to apply patch.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.37 KB
Failed: Failed to apply patch.
[ View ]
new1.39 KB
Failed: Failed to apply patch.
[ View ]

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

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');
?>

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

StatusFileSize
new2.08 KB
Failed: Invalid PHP syntax.
[ View ]
new2.59 KB
Failed: Failed to apply patch.
[ View ]

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.

+1 on this feature. Will test shortly.

StatusFileSize
new2.13 KB
Failed: Failed to apply patch.
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

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?

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.

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

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.

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?

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
Failed: Failed to apply patch.
[ View ]

Reroll and add Alan remark.

StatusFileSize
new1.92 KB
Failed: Invalid PHP syntax.
[ View ]

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

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?

Status:Needs work» Needs review
StatusFileSize
new4.1 KB
Failed: Invalid PHP syntax.
[ View ]

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

@Alan :

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

Static $file was added in rev 1.303.

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

@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;
}

subscribing

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.

Status:Needs work» Needs review
StatusFileSize
new4.08 KB
Failed: Invalid PHP syntax.
[ View ]

Updated patch 22 above.

Status:Needs review» Needs work

The last submitted patch failed testing.

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

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

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

subscribing.

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

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

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

Status:Needs work» Needs review
StatusFileSize
new3.87 KB
Failed: Failed to install HEAD.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.87 KB
Unable to apply patch 137932_enctype_cleanup_0.patch
[ View ]

reroll.

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

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

Status:Needs review» Reviewed & tested by the community

Looks good.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed. Thanks.

Status:Fixed» Closed (fixed)

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