Steps to reproduce:

  1. Create a node type with an image field with minimum dimensions of 300x500 pixels
  2. Create a new node of that type and attempt to upload an image which is smaller than 300x500 pixels
  3. Error message presented: "The specified file test.jpg could not be uploaded. The image is too small; the minimum dimensions are 300x500 pixels."
  4. Attempt to upload an image which is at least 300x500 pixels

Expected results:
Image is successfully uploaded and the error message is removed.

Actual results:
Image is successfully uploaded but the error message from the previous upload is still present.

Analysis:
From what I can tell the problem is in file_ajax_upload() in file.module. The markup generated by theme('status_messages') is prepended to the rendered form and ends up outside the form's ajax-wrapper div. When the form is submitted a second time only the ajax-wrapper div is replaced and the message is not removed.

The simplest way to get the status messages inside the ajax-wrapper is to replace line 238 of file.module:
$output = theme('status_messages') . drupal_render($form);
with
$form['#prefix'] .= theme('status_messages');
$output = drupal_render($form);

I verified that this change fixes the problem however I'm new enough to Drupal that someone else should look at this. There might be a better way to solve this or an edge case which is broken by this change.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Version: 7.15 » 8.x-dev

Does this issue exist in 8.x-dev?

lxs’s picture

Yes, I am able to reproduce the same issue with 8.x-dev (September 24, 2012 - 08:19).

It also looks like $output = theme('status_messages') . drupal_render($form); still exists in file_ajax_upload() in Drupal 8 (now on line 951). I checked and my above patch does still fix the problem.

larowlan’s picture

Issue tags: +Needs tests, +#pnx-sprint

Tagging

penyaskito’s picture

Issue tags: +Novice, +Needs backport to D7

This could be a good starting point for a novice in creating patches and writing tests. It should be backported to D7 once is fixed in 8.x.

balsama’s picture

Status: Active » Needs review
FileSize
541 bytes

Drupal 8 patch attached.

andymartha’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
73.13 KB

The latest patch, file_validation_error_message-1792032-5.patch, worked for me in correcting the error message bug in the latest version of drupal 8 as of March 6, 2013. Please see attached screenshot.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Still tagged "Needs tests".

sidharthap’s picture

FileSize
39.8 KB
93.99 KB

I tested the patch #5, It works for me. Please find the attached before and after screen shots.

Before :
before
After :
after

bjorpe’s picture

Assigned: Unassigned » bjorpe

Started working on an automated test.

bjorpe’s picture

Test added, see attached patches.

This bug/fix seems to be applicable to all validations, not just image size. To keep the test simple, it checks file extension validation.

phl3tch’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Great issue summary, nice test, nice patch :)

Committed ff1eea3 and pushed to 8.x. Thanks!

bjorpe’s picture

Assigned: bjorpe » Unassigned

Can't backport at the moment

karthikkumarbodu’s picture

Hi, Thanks for the fix can the patch be applied in the current Drupal 7.22 core?

Karthik Kumar Bodu

ndobromirov’s picture

Added a patch for drupal 7

ndobromirov’s picture

Status: Patch (to be ported) » Needs review
balajidharma’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Closed #1020916: File ajax upload doesn't clear previous error message after successful ajax upload as a duplicate (although technically this was the duplicate since it's newer).

Is there a reason the tests aren't being backported to Drupal 7 also?

ndobromirov’s picture

I needed the patch in 7 and I was in a hurry in the middle of a project.
Probably sometime this week(end) I'll attempt to back-port the tests also.

Can confirm that the patch works on live with no side effects for almost 2 months now.

karthikkumarbodu’s picture

Hi, i believe there is a fix for this issue in Drupal 7 and its not fixed in Drupal 6.23.

mgifford’s picture

acbramley’s picture

Patch correctly fixes the issue for me, but the steps to reproduce need to be slightly tweaked:

1) Choose a file that doesn't pass validation
* Error appears
2) Click the upload button
3) Choose a file that doesn't pass validation again
4) File appears in dialog
5) Click upload
* Error appears
6) Choose a file that does pass validation
7) Click upload
* Error remains

After applying the patch, the error disappears after clicking upload, but it might be better to remove it after you choose a correct file?

lmeurs’s picture

As acbramley says in #22: moving the messages to inside the AJAX wrapper works great, thank you! His remark about error messages being removed right after selecting a file could be easily fixed by wrapping the error messages. The patch would look like:

- $output = theme('status_messages') . drupal_render($form);
+ $form['#prefix'] .= '<div class="file-upload-js-error">' . theme('status_messages') . '</div>';
+ $output = drupal_render($form);

Explanation

There are two types of file validation:

  1. The file extension is validated client side,
  2. The file size is validated server side.

When selecting a file, Drupal.file.validateExtension() is fired which removes possible earlier inserted error messages right away. This only happens to error messages inserted client side by this same function, since these messages have the .file-upload-js-error class. By wrapping the server side messages in a DIV with the same class, they also immediately get removed as soon as a new file is only selected.

But there is a catch...

Situation: After uploading a too large file, a server side error is returned. When now selecting a file with an invalid extension the client side extension validator is not being fired.

This is because the onchange event has not been bound to the file field after the server side validation error. Drupal.behaviors.fileValidateAutoAttach.attach uses selectors to find the file field (child element), but file_managed_file_process() bases these selectors on the managed file element's ID (parent element). This ID gets incremented on each AJAX file upload / removal, but the ID of the file field only gets incremented in case of an invalid upload.

On top of this, the file field's selector is composed by appending "-upload" to the managed file element's ID which results in selectors like "#edit-attachment--5-upload" when the file field's ID at the same time could be "edit-attachment-upload" or "edit-attachment-upload--3" (the incrementation and position of "-upload" differ).

Conclusion

The server side error messages will be removed immediately when a new file is selected, as long as they are being wrapped as suggested and the selector problem is fixed (or use my workaround from #2235977-1: JS Client-side file validation is broken (because ajaxPageState is broken?)).

jessum’s picture

Assigned: Unassigned » jessum
jessum’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
107.83 KB

I reproduced the issue and found the error due to the wrong picture size (see attached file: Pics of result of patch.zip).

After finding the problem error I retried the small picture with the patch and then with the large picture and the error message disappeared (see attached file: Pics of result of patch.zip).

Thanks to Leslie Glynn for mentoring my first review.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

The tests still need to be backported - see above.

sarjeet.singh’s picture

Thanks lmeurs, your solution in #23 is working fine for me. We can commit it core.

Anonymous’s picture

I can confirm that the patch in #15 works. When can we expect to see this in core?

lmeurs’s picture

I attached a patch based on #15 which also handles the problem as described by #22, solved by #23 and confirmed by #27.

Now all we need are some tests... Anyone? :-)

sarjeet.singh’s picture

Status: Needs work » Needs review
basvredeling’s picture

I prefer patch #15 over #29.
#29 introduces a new div. I don't like the classname and don't really understand why the div is added in the first place.

lmeurs’s picture

@basvredeling: I do not like another DIV as well, but by adding the DIV.file-upload-js-error wrapper Drupal.file.validateExtension (see modules/file/file.js) can remove earlier generated messages (see #22). The class is already being used for JS generated messages, so why not also use it for PHP generated messages?

basvredeling’s picture

Status: Needs review » Reviewed & tested by the community

@lmeurs fair enough.
I'm still not very keen on adding another div and giving a class file-upload-js-error to generic messages output. Even though it is very likely to be an error, and not a status, info or warning message.
But I see you point.

Regarding the tests... there were no tests before, this should be a separate issue probably: "create tests for file upload validation".

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

The tests from the Drupal 8 patch (see #12) still need to be backported.

Also, if we need to adjust the fix, it should be done in Drupal 8 first. However, I quickly tested #15 vs. #29 and wasn't able to see the practical difference - are there steps to reproduce a difference? If #15 doesn't break anything that currently works, it looks to me like the more reasonable fix for this particular bug (maybe another issue could deal with other related bugs).

lmeurs’s picture

@David_Rothstein: Expected behaviour is:

  1. Step 1: Select a file of a wrong type and the client side file type validator immediately inserts an error message (div.messages.error.file-upload-js-error) into the DOM.
  2. Step 2: Select a too large file of the right type, the client side file type validator immediately removes the previous error message.
  3. Step 3: Upload the file, the server side file size validator returns an error message (div.messages.error) through AJAX.
  4. Step 4: Again, select a file of a wrong type, the previous error messages will be removed and a new error message will be inserted.

Now there are two problems preventing this expected behaviour from happening.

Problem 1: server side generated error messages are not being removed

At step 4 the client side file type validator cannot find any div.file-upload-js-error elements, so the server side generated error message (referring to another file selected at step 2) remains in the DOM and a client side generated error message (referring to the file selected at step 4) will be added. The patch from #29 wraps the server side generated message in a div.file-upload-js-error element so it will be automatically removed.

Problem 2: the client side file validator "seizes working" after ie. a server side error message

The bug from problem 1 cannot be easily reproduced due to another bug. The client side file validator (file type) "seizes working" after the server side file validation (file size) has returned an error message! So at step 4 the file type is not being validated client side, the user first has to upload the file to get it's type validated server side. Since it's type is wrong, the AJAX return (field + error message) will replace the previous version of the field + error message.

For more info on this bug see "But there is a catch..." at #23 and #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?).

Without #2235977 being fixed, the only benefit of the patch from #29 compared to the one from #15 is that not only client side, but also server side generated errors will immediately be removed as soon as the user selects another file (as noticed by #22).

basvredeling’s picture

How can we push this to a fix? Just by porting the tests to D7?

@lmeurs
I reread this issue. And though I can see the use of patch #29 in relation to #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?), a simpler patch without the wrapper class has already been committed to D8. I think it's wise to keep these patches similar. So either we create a patch for both D8 and D7 with the wrapper class or we don't and just try to commit the simpler solution from #15.
Since #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?) is still undergoing D8 patch review I would suggest adding the wrapper class as a patch there and then porting it to D7 in that issue.

  • alexpott committed ff1eea3 on 8.3.x
    Issue #1792032 by balsama, bjorpe | lxs: Fixed File validation error...

  • alexpott committed ff1eea3 on 8.3.x
    Issue #1792032 by balsama, bjorpe | lxs: Fixed File validation error...
basvredeling’s picture

Current status: @alexpott added a patch to the 8.x branches. The patch itself is trivial, porting the test to 7.x probably isn't.

stefan.r’s picture

Issue tags: -Novice

@basvredeling yes this still needs tests for D7

aerozeppelin’s picture

aerozeppelin’s picture

Status: Needs work » Needs review

The last submitted patch, 41: test-only-fail-1792032-41.patch, failed testing.

Ajay Jagtap’s picture

Status: Needs review » Reviewed & tested by the community

Hi,

I have applied "https://www.drupal.org/files/issues/1792032-41.patch" on D7 and its working as expected.

Hoping to get this patch included in core.

Thanks,
Ajay Jagtap.

joekers’s picture

Applied patch in #41 for Drupal 7 and it's working fine.

  • alexpott committed ff1eea3 on 8.4.x
    Issue #1792032 by balsama, bjorpe | lxs: Fixed File validation error...

  • alexpott committed ff1eea3 on 8.4.x
    Issue #1792032 by balsama, bjorpe | lxs: Fixed File validation error...
stefan.r’s picture

Issue tags: -Needs tests +Pending Drupal 7 commit
stefan.r’s picture

Assigned: jessum » Unassigned

  • stefan.r committed f1a3f53 on 7.x
    Issue #1792032 by aerozeppelin, bjorpe, lmeurs, ndobromirov, balsama:...
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit +7.54 release notes

This was committed above, so marking fixed.

Should have a followup to remove t() from the assertion messages (affects Drupal 8 too):

+      $this->assertRaw($error_message, t('Validation error when file with wrong extension uploaded (JSMode=%type).', array('%type' => $type)));
....
+      $this->assertNoRaw($error_message, t('Validation error removed when file with correct extension uploaded (JSMode=%type).', array('%type' => $type)));

Status: Fixed » Closed (fixed)

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

oguraandre’s picture

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

This issue is still happening on 8.4.x-dev (June 14, 2017).

oguraandre’s picture

FileSize
710 bytes

This patch should fix it.

diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index a6fc3f6ebe..508fa6902d 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -870,9 +870,12 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
       ];
       // @todo Add support for render arrays in drupal_set_message()? See
       //  https://www.drupal.org/node/2505497.
+      drupal_get_messages();
       drupal_set_message(\Drupal::service('renderer')->renderPlain($message), 'error');
       $files[$i] = FALSE;
       continue;
+    } else {
+      drupal_get_messages();
     }
 
     // Move uploaded files from PHP's upload_tmp_dir to Drupal's temporary
sreenivasparuchuri’s picture

FileSize
1.03 KB

I tried to fix the issue by using this patch where in file.js before client side validation error are generated, i am removing the old ajax error messages