Posted by eojthebrave on March 18, 2010 at 12:41am
14 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | file.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
The new #managed_file FAPI element doesn't work correctly when the element of '#type' => 'managed_file' is not at the top level of the $form array unless you specify '#tree' => TRUE on the parent element.
This for example will not work.
<?php
$form = array();
// Works as expected.
$form['my_file'] = array(
'#type' => 'managed_file',
'#title' => t('My File'),
);
// Doesn't work.
$form['settings']['my_file'] = array(
'#type' => 'managed_file',
'#title' => t('My File'),
);
?>The second example results in the file being uploaded, but not properly processed and spits out these errors. The function file_ajax_upload() is unable to locate the element for processing within the $form array.
Notice: Undefined index: image_example_block_image in file_ajax_upload() (line 233 of /Users/joe/Sites/drupal_dev/HEAD/drupal/modules/file/file.module).
Notice: Undefined index: image_example_block_image in file_ajax_upload() (line 246 of /Users/joe/Sites/drupal_dev/HEAD/drupal/modules/file/file.module).
Notice: Undefined index: #suffix in file_ajax_upload() (line 255 of /Users/joe/Sites/drupal_dev/HEAD/drupal/modules/file/file.module).The attached patch (solution from quicksketch on irc) fixes the problem. Quicksketch will have to explain what is going on here as I'm not sure I can.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| eojthebrave-file_array_parents.patch | 621 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 18,753 pass(es). | View details |
Comments
#1
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:
<?php$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 containarray('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.#2
The last submitted patch, eojthebrave-file_array_parents.patch, failed testing.
#3
eojthebrave-file_array_parents.patch queued for re-testing.
#4
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.
#5
Needs a reroll at least.
#6
subscribing
#7
Subscribing.
#8
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.
#9
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.
#10
#11
@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 ?
#12
@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...
#13
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.
#14
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.
#15
+++ 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.
#16
Er, doh :-p
#17
Thanks! Code looks good, it seems you tested it, so RTBC.
#18
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.
#19
@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.
#20
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.
#21
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/stylesTest it in drupal 7 Alpha 6 and it works like a dream come true.
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 :)
#22
@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.
#23
Working on it.
#24
#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!
#25
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.
#26
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.
#27
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
#28
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.#29
i've started on adding file upload support to drupalPostAJAX(). ouch, is that code nasty.
i don't have anything worth posting yet, don't let it hold up this issue. effulgentsia, i'll try to catch you in IRC to discuss the best way to do this, then amend what i've done to suit ;-)
#30
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.
#31
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 ?
#32
ok, here's a "hey, i somehow got it to work" patch that clearly needs work. i'm setting it to needs review so the bot can have a go at it. there's a new test testAjaxUploadSingleValuedWidget that uploads a file via ajax, that needs to be refactored in follow ups.
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'm happy to continue this in a follow up if we want to commit #24.
still lots of TODO:
1. make it sane (effulgentsia, review please)
2. make it actually test the bug at hand
3. follow up patches, refactor tests that skip the ajax upload stuff to test the ajax path as well
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.
#33
Back to needs work, as per first sentence of #32.
#34
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.
#35
updated the test to actually exercise the bug, and figured out how to use drupalPostAJAX() properly.
#36
argh, #35 has debug hunks.
this time without the debug.
#37
@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.
#38
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.
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.
#39
The last submitted patch, 745590-managed_file-37.patch, failed testing.
#40
#37: 745590-managed_file-37.patch queued for re-testing.
#41
With nits from chx's review in IRC.
[EDIT: Oops. Ignore the file name. This is the full patch, code and tests.]
#42
That looks great.
#43
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.
#44
double post
#45
#46
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.
#47
Committed to CVS HEAD.
#48
Automatically closed -- issue fixed for 2 weeks with no activity.