If a custom form element adds a managed_file form element during a #process block the file_ajax_upload() function issues a notice in file.module on line 256. In file_ajax_upload() the logic is trying to get to the form element by traversing the form before the #process callback has been executed.

For instance:


function example_element_info() {
  return array(
    'example_file' => array(
      '#process' => array('example_process'),
    ),
  );
}

function media_file_process($element) {
  $element['#tree'] = TRUE;
  $element['upload_file'] = array(
    '#type' => 'managed_file',
    '#title' => t('Upload file'),
  );
}

Then when the Upload button is pressed a notice is shown:

Notice: Undefined index: upload_file in file_ajax_upload() (line 256 of /home/prarie/Documents/Workspace/Drupal/modules/file/file.module).

This looks like it will only mess up the display when a class will never get added at line 270 (and the PHP notice message) and not actual file handling.

It appears to be as simple as adding a call to form_builder before the traversing the form to find the managed_file element. Will roll a patch once I get my local environment ready to do so.

CommentFileSizeAuthor
#27 D7_managed_file_1067470-27.patch622 bytesjoseph.olstad
#12 managed_file_1067470.patch788 bytesPrarie
#6 managed_file_1067470.patch675 bytesPrarie
#1 managed_file.patch581 bytesPrarie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Prarie’s picture

Prarie CreditAttribution: Prarie commented
Assigned: Unassigned » Prarie
FileSize
managed_file.patch581 bytes

Patch that adds the form_builder call before the element traversal.

Prarie’s picture

Prarie CreditAttribution: Prarie commented
Status: Active » Needs review
Prarie’s picture

Prarie CreditAttribution: Prarie commented
Version: 7.0 » 7.x-dev
Issue tags: +Quick fix

Changing version and adding Quick fix tag.

Prarie’s picture

Prarie CreditAttribution: Prarie commented

#1: managed_file.patch queued for re-testing.

bfroehle’s picture

bfroehle CreditAttribution: bfroehle commented

This seems like the wrong fix given that form_builder() isn't called anywhere outside of includes/form.inc.

Would it work to just move the file count to below the drupal_process_form() call?

Prarie’s picture

Prarie CreditAttribution: Prarie commented
FileSize
managed_file_1067470.patch675 bytes

You are probably right that calling form_builder is not the right solution. But, moving the count after the the form is processed would mean the delta will always equal and the class wouldn't be assigned to highlight the new files. I'm not sure it would be different with form_builder called earlier either.

Instead, if it's not set then it's not relevant and this patch handles it in a better way, I think.

jasondecamp’s picture

jasondecamp CreditAttribution: jasondecamp commented

This patch works for me. I'm surprised this hasn't been committed already.

BassPlaya’s picture

BassPlaya CreditAttribution: BassPlaya commented

This patch worked for me as well. Cheers!

gagarine’s picture

gagarine CreditAttribution: gagarine commented
Status: Needs review » Reviewed & tested by the community

#6 Works fine thanks

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs backport to D7

Thanks for the patch and testing. One thing to note is that bugs are fixed in D8 first and then backported to prevent regressions. (Usually the fixes are committed to both branches within a couple weeks of each other.) Reference: http://drupal.org/node/767608

Also, we should add an automated test that reproduces the bug. The test should fail without the patch above, and pass with it. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

It's hard to write a test for this case because it require to implement custom element with filefield inside

So let's re-roll this and make inline comment about iteration logic

Prarie’s picture

Prarie CreditAttribution: Prarie commented
Status: Needs work » Needs review
FileSize
managed_file_1067470.patch788 bytes

When I initially opened this issue Drupal 8 hadn't started development (I think). Anyhow, this is rerolled for Drupal 8. Also, added a comment as to why the check is happening.

I remember seeing some instructions for naming the patch so a separate one gets applied to D7 (I would re-roll that one as well to include the comment), but I couldn't find how to do that.

ivanhi’s picture

ivanhi CreditAttribution: ivanhi commented

I have written the text below, but I have just realized that It doesn't have to do with the patch because before the patch I wasn't able to see the error of "Referencing to the file used in the "Video" field is not allowed".

I know that it's not the place but I have read a lot of documentation about the problem but not with Drupal 7 and Video module. The problem is only with the thumbnail of the Video Module Update because I upload images in articles and there aren't any problem.

Thank you

-----------
Hi,

First of all thanks for the patch managed_file_1067470.patch. But now I have another problem.

When I edit a node I get the next error: Referencing to the file used in the "Video" field is not allowed.
May be has something to do with the comment "Make sure the path is set: custom form elements might not have added it yet."

But I have created a new form and there is still the error.

Thank you very much.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

@ivanhi, you should file an issue in that module's queue. This issue is only about the bug that is being patched in #12. Thanks.

ivanhi’s picture

ivanhi CreditAttribution: ivanhi commented

I have found a solution in the next link about the problem or "Referencing to the file used in the "Video" field is not allowed." with the module video and the thumbnails.
http://drupal.org/node/1441626#comment-5660568

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

There's is a duplicate issue with different approach #1404480: Notice: Undefined index: in file_ajax_upload() (line 258 of modules/file/file.module).
Still no test on quick fix issue

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
CreditAttribution: klonos commented

...coming from #1404480: Notice: Undefined index: in file_ajax_upload() (line 258 of modules/file/file.module).

offroad’s picture

offroad CreditAttribution: offroad commented
Issue tags: -Quick fix, -Needs tests, -Needs backport to D7

#12: managed_file_1067470.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, managed_file_1067470.patch, failed testing.

Prarie’s picture

Prarie CreditAttribution: Prarie commented
Version: 8.x-dev » 7.x-dev
Assigned: Prarie » Unassigned
Issue summary: View changes

It appears to me that this issue is not relevant to 8.x-dev -- the function file_ajax_upload() doesn't exist anymore -- changing the version to reflect. I haven't been that close to version 8 and obviously don't care about this change very much. Removing the assignment to me and will leave this to someone to pick up if anyone cares enough to get the change in version 7.

Prarie’s picture

Prarie CreditAttribution: Prarie commented
Status: Needs work » Needs review

12: managed_file_1067470.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 12: managed_file_1067470.patch, failed testing.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

The code has moved to \Drupal\file\FileWidgetAjaxController in Drupal 8, but I'm not sure if the bug exists there or not anymore (the code looks quite a bit different).

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Related issues: +#1404480: Notice: Undefined index: in file_ajax_upload() (line 258 of modules/file/file.module).

This issue was created in 2011.

However, back in 2012 someone created a duplicate issue of this one.
Here is the dupe:
#1404480: Notice: Undefined index: in file_ajax_upload() (line 258 of modules/file/file.module).

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

8.x branch:
- patch #12 no longer applies

7.x branch:
- patch #6 appears to be still valid, however would be helpful to trigger recent simpletest

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs work » Needs review
FileSize
D7_managed_file_1067470-27.patch622 bytes

reroll and test

Status: Needs review » Needs work

The last submitted patch, 27: D7_managed_file_1067470-27.patch, failed testing.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs work » Needs review

php 7 pass, php 5.3 looks like just a testbot issue, requeued.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs review » Needs work

ok it passed second time around

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs work » Needs review
parasolx’s picture

parasolx CreditAttribution: parasolx commented
Status: Needs review » Needs work

I confirmed at the patch #27 still given the error notice after implement.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
CreditAttribution: sjerdo at Synetic commented

Seems like the reroll in #27 is incorrect.

+++ b/modules/file/file.module
@@ -260,6 +260,13 @@ function file_ajax_upload() {
     $current_element = $current_element[$parent];

This line should be removed.