#managed_file element does not work when #extended not TRUE, or when ancestor element doesn't have #tree=TRUE

What's happening here is that managed_file elements need to find the original element in the AJAX callback, since all that we have to start with is the button that was clicked. Previous to this patch we were using #parents to find this element, but in some situations #parents will not reflect the path within the $form variable to get to the element.

So for example:

$form = array(
  '#tree' = FALSE;
);
$form['foo']['bar'] = array(
  '#type' => 'managed_file',
  // etc...
);

When we check #parents in the AJAX callback, it only contains array('bar'), because #tree is FALSE. If #tree were true, it wouldn't contain array('foo', 'bar'), which is why managed file currently works on existing forms (the node form and user profile forms for example). Using #array_parents however will always contain the full tree, which is why we should use it instead of #tree. Moving to needs review for test bot to have a look.

eojthebrave-file_array_parents.patch queued for re-testing.

ugh. Is this code still in HEAD?

If it still is, then we need to refactor this code. #ajax triggers form caching, and so the ajax callback can rely on form caching. Effectively, it can re-use already existing information.

See #641670: Move $form['#field'] meta information into $form_state for how the entire field handling was updated in this manner. Perhaps File module's code can even re-use the information that's available now.

Needs a reroll at least.

subscribing

Subscribing.

Raising priority. This seriously limits the scope of the supposedly generic #managed_file FAPI element.

I bumped into this when testing an image field inside a (work-in-progress) combofield.

Unbelievably, the patch eojthebrave uploaded back in March *still* applies, and apparently it passes tests. Here's a reroll with line numbers updated. yched: could you try it out with your test version of combofield? It seemed to work for us back when eojthebrave wrote the patch. It already has my approval.

@quiksketch : Thks, I'll try to check that tomorrow.
Any idea if #932648: Ajax warning when uploading to managed_file field is the same issue or a different one ?

@quicksketch :
Just tested, file ajax still fails in combofield.

Strange, though :
- file_managed_file_process() creates the 'upload' button with
$element['upload_button']['#ajax']['path'] = "file/ajax/field_combo/und/0/field_file/und/0/[form_id]",
(correct, #array_parents is field_combo][und][0][field_file][und][0)
- by the time it enters ajax_pre_render_element(), its ['#ajax']['path'] has become
"file/ajax/field_combo/und/field_file/und/0/[form_id]" (the middle '0' disappeared).
--> The ajax request is sent to this incorrect path, file_ajax_upload() gets the wrong $form_parents array, and fails to find the right $current_element.

Same bug if that numeric index in the middle is 1 instead of 0 (not a stupid '0 is empty' bug somewhere)

If I change file_managed_file_process() to set $element['upload_button']['#ajax']['path'] to any dummy arbitrary path including number parts, the path always comes in ajax_pre_render_element() with the 1st numeric part missing :
'foo/1/2/3/bar' ---> 'foo/2/3/bar'
foo/bar1/2/baz" ---> 'foo/bar/2/baz'

For now, I can't make any sense of this...

Heh, that's because file_field_widget_process() comes after file_managed_file_process() and removes the 1st numeric index in the path, which it considers as the filefield's delta. That would rather be the last numeric index instead - or a path built on $element['#parents'], but popping the last part. I'm not a regex guru, so attached patch goes that way.

Once this is fixed, I get no error in combofield - but the file widget comes back 'enpty' after the ajax roundtrip as if nothing was uploaded. At this point, might be an issue with my prototype combofield, though.

Remaining bug within combofield is because file_field_widget_form() reads values directly from $form_state['values'][$field_name], which is kosher in current HEAD.

Changing this is part of #942310: Field form cannot be attached more than once, which allows field values to live at different #parent places in the form. For the issue at hand, patch #13 should be good enough if quicksketch approves the code.

+++ modules/file/file.field.inc
@@ -648,7 +648,9 @@ function file_field_widget_process($element, &$form_state, $form) {
+    $parents = $element['#array_parents'];
+    array_pop($parents);
...
     $field_element = drupal_array_get_nested_value($form, array_slice($element['#array_parents'], 0, -1));

It looks like the following array_slice() results in the same value we already have in $parents now?

Powered by Dreditor.

Er, doh :-p

Thanks! Code looks good, it seems you tested it, so RTBC.

Is there any hope of getting the #managed_file FAPI element fixed soon? The last time it actually worked was Drupal 7 - Alpha 6. Broke in Alpha 7, Beta 1 and still in Beta 2. I was going to use this functionality in a D7 module but if it's going to languish I need to switch to an alternative file upload method.

@Oscar Toirneach the best thing to do is try out this patch and see if it allows you to do what you want it to do. Even without this patch it should work properly as long as it's at the root of the form. Obviously everything needs to get fixed "soon", Drupal 7 isn't going to come out if you can't upload a file.

Hm, patch doesn't seem to fix the 'default image' upload element on "Field settings form" for image fields.

On pressing 'Upload', the ajax request chokes on the drupal_array_set_nested_value() call at the end drupal_validate_form().

Marked #953356: Default Image Upload Fails HTTP request terminated abnormally as duplicate.

A pristine install of Drupal-7-beta2, the patch supplied in #16 and the simple image_example in the D7 version "examples for developers" module (http://drupal.org/project/examples) fails to work. You click upload, get the spinner but seemingly nothing happens. In the error log you get:

PHP Fatal error:  Cannot create references to/from string offsets nor overloaded objects in /var/www/drupa7b2.drupaltestbed.net/drupal-7.0-beta2/includes/common.inc on line 5977, referer: http://drupa7b2.drupaltestbed.net/image_example/styles

Test it in drupal 7 Alpha 6 and it works like a dream come true.

Obviously everything needs to get fixed "soon", Drupal 7 isn't going to come out if you can't upload a file.

Yeah, I'm kind of baffled that such fundamental functionality has been left broken and unassigned through an alpha and two beta releases. I guess there are more exciting bugs to squash :)

@Oscar Toirneach : please refrain statements like "how come this isn't fixed yet". It won't make the bug fixed faster. Everyone is on volunteer time here.

back to CNR, and this does qualify as critical IMO.

Working on it.

#16 is a fine fix for the originally titled issue. #20 and #21 are an additional bug for when the #managed_file element is allowed to keep its default value for #extended (FALSE).

Agreed that both bugs are critical. It basically means #managed_file can only be used with a field widget of a top-level field, which while covering 99% of the use-cases in core, defeats the entire intent of the element type being reused in contrib.

I don't really understand the reason for why #managed_file supports a #extended property at all, and why it defaults to FALSE. I'm guessing there's some historical basis for this, but it results in pretty odd string to array and back conversions being needed. But it's too late for D7 to drop it, so we might as well fix it.

I left #16 alone, but added the fix for the other bug. Note that this fix changes form_set_value() to have the same behavior it has in D6 and in D7A6, and fixes a related flaw introduced by #763376-107: Not validated form values appear in $form_state.

CNR for bot, but this needs a test added. Easy test: just a #managed_file element inside a FAPI element. Should break in HEAD, and be fixed by the patch. I don't know when I'll have time to add that, so if someone else wants to, that would be awesome!

Easy test: just a #managed_file element inside a FAPI element. Should break in HEAD, and be fixed by the patch.

Correction: this will not be an easy test to write. The test would need to submit a file using drupalPostAJAX(), which is not yet supported. I'll try to find some time to do this, but if someone wants to beat me to it, go for it.

Thanks for tackling this, eff.

Patch fixes image field's 'default image' in Field UI.
Also fixes the sample form from http://drupal.org/project/examples

The drupal_validate_form() hunk, about button's #value in $form_state['values'], is above my head, so I'm a little uneasy pushing the RTBC button.

+++ includes/form.inc	5 Nov 2010 19:57:32 -0000
@@ -2301,7 +2320,7 @@ function form_type_token_value($element,
 function form_set_value($element, $value, &$form_state) {
-  drupal_array_set_nested_value($form_state['values'], $element['#parents'], $value);
+  drupal_array_set_nested_value($form_state['values'], $element['#parents'], $value, TRUE);
 }

looks like it fixes the worries I had in #913528-54: Create new boolean field "Cannot create references to/from string offsets nor overloaded objects" (and which Karen struggled with in Date, IIRC)
Assigning an array value to an element that previously had a scalar value. Great !

Powered by Dreditor.

This looks good but why the removal of isset in drupal_array_get_nested_value ? It should be obvious to me but it's not

Because after the first time through the loop, each additional time, $ref might be a string, and isset($ref[$parent]) might therefore be TRUE, but not in the way we expect. So, we need the is_array() check regardless. We could do is_array($ref) && (isset($ref[$parent]) || array_key_exists($parent, $ref)) but since we need the is_array() anyway, I don't know if it's worth it.

drupalPostAJAX() is indeed a beast. eojthebrave has some documentation in-progress at #789186-28: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage. I don't know yet how hard it would be to get it to support file uploads. It might not be too hard, if it can just pass off the needed file data and headers to drupalPost(). I'll be in IRC Wednesday, and can help review and/or work on this then. I hate to be a bottleneck on this though, so if a test is working before then, no need to wait for me, and also, I think it's okay for this patch to be committed without a test, and let the test come later. According to #944308-24: Update to jQuery 1.4.4, we'll need a beta-3 before we start with RCs, and my vote would be to get this in (even if without a test) before releasing beta-3, because hopefully, each beta release attracts more contrib developers porting their modules, and not having a broken 'managed_file' element would make their experience more pleasant. That's not a requirement though, so not adding the "beta blocker" tag.

We do need this in sooner than later, and much preferably before the next release, be it beta or RC.

As I said earlier, I'd be happy to push RTBC, but I have no clue about the drupal_validate_form() hunks in eff's #24. If someone more FAPI-savvy wants to push this ?

Back to needs work, as per first sentence of #32.

Thanks, justinrandell. Looks like you figured out the key part here, which is that passing 'files' data as part of $edit to drupalPostAJAX() works. Awesome! I'll put some time into this today to clean up the test.

@justinrandell: Great start! As discussed in IRC, I was working on an alternate version that goes in a similar direction, but covers more test cases. This tests the Upload and Remove buttons, with and without AJAX, and the whole process with #tree=FALSE and #tree=TRUE, and #extended=FALSE and #extended = TRUE. You should be in a good position to review it now :) Also, thanks for the tip about making the fid checks more robust.

Here's the tests only, to prove they fail, and then the tests combined with #24.

i've included the doc patch from #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage because its awesome and helped me get this working, but i'll take it out if that will help this land.

I took it out of this patch. I'm glad they were helpful for you (eojthebrave++), but let's review those separately as part of the other issue. Both your patches and mine ended up not needing to modify drupalPostAJAX() itself, so those docs really are a separate issue.

meta comment - for D8, i think we need to revisit the basic premise here. i'm not sure whether faking the DOM manipulation is worth the effort. perhaps we should skimp on that and just concentrate on faking up the data structures so we can send the right POST data, and test the json matches what we expect, rather than the html. anyway, surely needs more thought.

The DOM manipulation within drupalPostAJAX() is necessary to test consecutive clicks, like "Upload" then "Remove" then "Submit". Prior to that, we were missing a lot of necessary AJAX test coverage. What's challenging about the function though is how many parameters it takes. That came about, because each enhancement of the function happened late in the D7 cycle, and tried to preserve BC. I'm not sure if we really need to be strict about BC for functions only used by tests, so it may be possible to greatly simplify the function's signature. But let's discuss that over in that other issue.

#37: 745590-managed_file-37.patch queued for re-testing.

With nits from chx's review in IRC.

[EDIT: Oops. Ignore the file name. This is the full patch, code and tests.]

That looks great.

 function form_set_value($element, $value, &$form_state) {
-  drupal_array_set_nested_value($form_state['values'], $element['#parents'], $value);
+  drupal_array_set_nested_value($form_state['values'], $element['#parents'], $value, TRUE);
 }

I'm unsure whether we are fixing really the bug here or are just working around it.

The cause for this is probably the same problem I mentioned in http://drupal.org/node/929494#comment-3566776 - perhaps we can have improve the value assignment process instead. 'll have a look at that later.

double post

We need folks to Get Real, Get Dirty, and Give up (on perfection). We are in the 'least of all evils' stage. Easy resolutions are unavailable. All software ships with bugs, and sometimes serious ones.

Ok. Indeed there is no cause to hold up this patch coming with great tests. Also I'm unsure whether a proper fix is still possible for d7 at all - if I manage to do that we can still do so in a follow-up. Issue: #768312: Cleanup form element processing and value assignment

So let's get this in.

Committed to CVS HEAD.