Updated: Comment #N

Problem/Motivation

When a file field is used more than once on a page, the AJAX upload or remove will always target the first instance.
One example for reproducing this is if two entities reuse the same field, and their edit forms are displayed on the same page.

Steps to reproduce in #22

Proposed resolution

Use drupal_html_id() for the ajax-wrapper added by file_managed_file_process()

Remaining tasks

Find solution.

User interface changes

N/A

API changes

N/A

Original report by @eme

When you have multiple forms containing the same file, it is not possible to remove it.

Let's take an example : I have nodes with comments and the comment forms on the page, and I have a file field in the comment form. I can load the file, but not remove it.

Why ? Because when you load multiple forms you have additionnal ID, like form-id--x, and this additionnal ID is incremental for any form loaded to make sure they got a unique id. Issue is that when you load the file via ajax, it has the issue of not taking again the original ID, but the normal one, and therefore it acts on the first form ! In some configurations, ajax callback breaks and you are just redirected.

Note that this does not happen to the first one that has no additionnal id and therefore works fine.

Attached is a little video demo.

Must say I'm working on that for some days and it seems quite tricky...

CommentFileSizeAuthor
#29 file_field_ajax_wrapper-1811100-29-D7-do-not-test.patch2.13 KBredndahead
#21 file-field-ajax-wrapper-1811100-21.patch2.34 KBjyotisankar
#20 1811100-file_upload_ajax_id-20-D7-do-not-test.patch2.14 KByched
#20 1811100-file_upload_ajax_id-20-D8.patch2.34 KByched
#17 file_upload_ajax_id-D7-1811100-17.do-not-test.patch2.14 KByched
#17 file_upload_ajax_id-D8-1811100-17.patch2.17 KByched
#12 file-field-ajax-1811100-12-D8.patch612 bytestim.plunkett
#12 file-field-ajax-1811100-12-D7-do-not-test.patch590 bytestim.plunkett
#9 file-field-ajax-1811100-9-D8.patch1.19 KBtim.plunkett
#9 file-field-ajax-1811100-9-D7-do-not-test.patch1.04 KBtim.plunkett
screens.zip1.53 MBeme
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Version: 7.9 » 8.x-dev

Is this issue present in 8.x-dev?

eme’s picture

eme CreditAttribution: eme commented

Just tried to test with Drupal 8, but this issue is hard to test easily because Views 8 does not work (it call neither comments nor comment form on its page when option is selected).

Any idea to call easily multiple comment forms (or other) ?

eme’s picture

eme CreditAttribution: eme commented

I just digged into file.module, and what is for sure is that the bug should be in D8 as well : we define the ajax wrapper for all filefield the same way : see lignes 1043 && 1114 :

'wrapper' => $element['#id'] . '-ajax-wrapper',

So if we have the same wrapper for all file fields of same element ID, we will for sure have an issue. As I do not see anything unique in $element, I tested with a uniqid() added and it seems to solve this first issue.

I think there is another issue and will post it when I have additionnal data, but it occurs in more complex situation where we modify multiple files adding / deleting (seems to be related to the process that comes toward drupal_html_id.

eme’s picture

eme CreditAttribution: eme commented

Indeed, last bug that is quite difficult to explain and for now disappears with a quite horrible hack in drupal_html_id : a uniqid() instead of strtr(drupal_strtolower($id), array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));

This is no solution so it still to be digged, but it has an interesting seems that with multiple AJAX call from different forms, we loose some data in ajax_html_ids that aggregate all ids that should be.

For me, the bug do not come from drupal_html_id that works fine, but from file module that forgot to add dynamically its ids.

To resume how file modue does, here is the nesting of functions :
file_ajax_upload (ajax callback) > drupal_process_form > drupal_rebuild_form > form_builder > drupal_html_id

During this process, ids corresponding to files seems to be just lost and therefore not incrementing (so AJAX behavior applies to anoter file field).

Hope this is clear enough...

eme’s picture

eme CreditAttribution: eme commented
Component: file system » file.module
eme’s picture

eme CreditAttribution: eme commented

The following hack in common.inc (function drupal_html_id) solves the issue as well :

	if (strpos($id, 'upload-button') || strpos($id, 'remove-button')) {
		$id = uniqid(); //EMERYA
	}

Which means that the issue comes specifically from the naming of the upload / remove buttons that do not take the correct ID. Still the issue is not to be search in this function : correct naming are not added to $_POST['ajax_html_ids'] for me.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Probably it's duplicate of
#1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)
#1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests
#1414510: Remove drupal_reset_static for drupal_html_id() when form validation fails

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Closed (duplicate)

Duplicate of #1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: Impossible to remove when multiple forms with same file field » File field AJAX wrapper always targets first instance on the page
Issue summary: View changes
Status: Closed (duplicate) » Needs review
Issue tags: +Needs backport to D7
FileSize
file-field-ajax-1811100-9-D7-do-not-test.patch1.04 KB
file-field-ajax-1811100-9-D8.patch1.19 KB

This is not a dupe, it's very much its own issue.
I hit this on D7, and the offending code is identical in D8.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue summary: View changes
Status: Needs review » Needs work

This is the wrong fix. $element['#id'] is already run through drupal_html_id(). The actual bug is that since there is never an actual form element with that ID printed to the page, Drupal.ajax.prototype.beforeSerialize does not add it to $_POST['ajax_html_ids'].

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

yep, this bug is different...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
file-field-ajax-1811100-12-D7-do-not-test.patch590 bytes
file-field-ajax-1811100-12-D8.patch612 bytes

So multiple file fields do not suffer from this problem because their element ID is used by the fieldset wrapper, and ajax.js is able to discover the ID.
But single file fields do not have their ID printed, so it is never added to $_POST['ajax_html_ids'].

Merely printing this div on the page is enough to fix the bug.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
Issue tags: +Needs tests, +Needs manual testing

makes sense, but needs test coverage, also second div could break styling

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Since drupalPostFormAjax doesn't actually run ajax.js, the only test we could add is that the ID is displayed on the page. Which seems like a useless test...

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

So manual testing is needed, also having a test for elemnt IDs is in page markup could be helpful

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I've manually tested it on D8 and D7. And it is directly added to the HTML being output, how could it not be on the page?

yched’s picture

yched CreditAttribution: yched commented
FileSize
file_upload_ajax_id-D8-1811100-17.patch2.17 KB
file_upload_ajax_id-D7-1811100-17.do-not-test.patch2.14 KB

Got hit by this too, and attached patch fixes it.
Wondering, however, whether we really need two different IDs there - $element['#id'] and $element['#id']-ajax-wrapper.

Why don't we just use $element['#id'] as the ['#ajax']['#wrapper'] ?

Attached patches (D7 & D8) do that, seems to work.

atrocity’s picture

atrocity CreditAttribution: atrocity commented

Patch in post #12 for D7 works fine for me. Thanks for this.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Needs review
FileSize
1811100-file_upload_ajax_id-20-D8.patch2.34 KB
1811100-file_upload_ajax_id-20-D7-do-not-test.patch2.14 KB

Rerolled the patches in #17.
- D8 patch needed a refactor now that the code moved from file_managed_file_process() to ManagedFile::processManagedFile()
- D7 patch is a straigth reroll (#17 still applied with fuzz)

jyotisankar’s picture

jyotisankar CreditAttribution: jyotisankar commented
FileSize
file-field-ajax-wrapper-1811100-21.patch2.34 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes
Status: Needs review » Needs work

Steps to test, done manually via simpletest.me
1) Configure comment type with file field admin/structure/comment/manage/comment/fields
2) Configure node type teaser to display comment field /admin/structure/types/manage/article/display/teaser
3) Create 2 nodes with comments enabled /node/add/article
4) Try to upload file, FAIL

Recent log contains exception and warning

 /file/ajax?element_parents=field_file%2Fwidget%2F0&form_build_id=form-XmW4i2jWEzZl13qkRCCs0yIHDd2Ga_l0bnsUJ9_-NsY
/file/ajax?element_parents=field_file%2Fwidget%2F0&form_build_id=form-kCzJfe8fW6j-Tvl4CC1ZKId-Zz5emL-6eRX7dW9Zyj8

<em class="placeholder">Exception</em>: Serialization of &#039;Symfony\Component\HttpFoundation\File\UploadedFile&#039; is not allowed in <em class="placeholder">serialize()</em> (line <em class="placeholder">19</em> of <em class="placeholder">/home/s22596f0eced320e/www/core/lib/Drupal/Component/Serialization/PhpSerialize.php</em>).

Preview is broken

comment/reply/node/2/comment

<em class="placeholder">Notice</em>: Trying to get property of non-object in <em class="placeholder">Drupal\comment\CommentForm-&gt;buildEntity()</em> (line <em class="placeholder">275</em> of <em class="placeholder">/home/s22596f0eced320e/www/core/modules/comment/src/CommentForm.php</em>).
yched’s picture

yched CreditAttribution: yched commented

@andypost: why did you reset to NW ?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

The ajax wrapper is doubled btw., which is another bug that should be fixed here, too.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Related issues: +#2245901: Trim trailing EOF whitespace from templates

ajax wrapper is doubled

There's special issue #2245901: Trim trailing EOF whitespace from templates

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

No, it is doubled even before the first AJAX takes place once by Element and once by FileField.

Both use #prefix, but its at different levels.

idebr’s picture

idebr CreditAttribution: idebr commented
Related issues: -#2245901: Trim trailing EOF whitespace from templates +#2346893: Duplicate AJAX wrapper around a file field

I'm assuming @andypost in #25 was referring to #2346893: Duplicate AJAX wrapper around a file field

dcam’s picture

dcam CreditAttribution: dcam commented
Issue tags: -Needs reroll

#21 still applies. Removing "Needs reroll" tags in preparation for Drupalcon Los Angeles sprints.

redndahead’s picture

redndahead CreditAttribution: redndahead commented
FileSize
file_field_ajax_wrapper-1811100-29-D7-do-not-test.patch2.13 KB

Reroll for d7

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Closed (outdated)
Issue tags: +Bug Smash Initiative

I tested this on Drupal 10.1.x, standard install using the steps in #22. It worked fine, there were no errors. The steps in #22 seem different that what is described in the Issue Summary. So, I tried again.

I made a new content type with two file fields. I tested creating and save with different files for each field, with the same file for each field, and only a file in the first field and only a file in the second field. In all cases, there were no errors and the files were related to the correct field.

Considering that there has been no discussion here for 8 years, I am going to say that this is now outdated.

Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!