Problem/Motivation

Javscript upload widget does not return to original state after error

Description from martynpanes:

If a file larger than the permitted limited is uploaded in a file field the response returned is the following generic error message:
"An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (2 MB) that this server supports." and the form is rebuilt with the file upload widget missing.

Whilst a browser refresh will return the file widget - a lot of users wont think to do this resulting in confusion and poor user experience.

Steps to reproduce:

1. Creat file upload field.
2. Attempt to upload file larger than server configuration

There are two different messages and file upload limits:

  1. If the uploaded file is larger than the limit configured in the content type, but smaller than the upload limit configured in php.ini, you will get the message "The specified file %name could not be uploaded. The file is %filesize exceeding the maximum file size of %maxsize." This message is in file.inc, in two parts.
  2. If the uploaded file is larger than the upload limit configured in php.ini, you will get the message, "An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports." This message is in file.module.

In this second case only, the upload widget disappears and any other file already uploaded also disappears. If you save at this point, any other files uploaded will be removed from the node.

Proposed resolution

for an ajax response addCommand(), instead of ReplaceCommand, PrependCommand.

Remaining tasks

  • verify test only changes fail in a meaningful way on 8.1.x and 8.2.x
  • code review

User interface changes

Kinda, fixes a UI bug to the behavior we expect (keep the file upload widget, and files previously upload (but not saved)).

API changes

No

Data model changes

No

Original Post:

If a file larger than the permitted limited is uploaded in a file field the response returned is the following generic error message:
"An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (2 MB) that this server supports." and the form is rebuilt with the file upload widget missing.

Whilst a browser refresh will return the file widget - a lot of users wont think to do this resulting in confusion and poor user experience.

Files: 
CommentFileSizeAuthor
#72 core-file_upload_ajax_message-1489692-68-testonly.patch729 bytesYesCT
#68 core-file_upload_ajax_message-1489692-68.patch2 KBjosmera01
#64 core-file_upload_ajax_message-1489692-D8.patch1.29 KBsudishth
#51 core-file_upload_ajax_message-1489692-51-D8.patch988 bytesLiam Morland
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#32 core_1489692_file_upload_ajax_message.patch694 bytesLiam Morland
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_1489692_file_upload_ajax_message.patch. Unable to apply patch. See the log in the details link for more information. View
#22 core_1489692_file_upload_ajax_message.patch694 bytesLiam Morland
FAILED: [[SimpleTest]]: [MySQL] 41,197 pass(es), 2 fail(s), and 2 exception(s). View
#17 core_1489692_file_upload_ajax_message_D8.patch736 bytesLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 57,481 pass(es). View
#8 core_1489692_file_upload_ajax_message_D8.patch736 bytesLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 55,022 pass(es). View
#7 core_1489692_file_upload_ajax_message.patch694 bytesLiam Morland
PASSED: [[SimpleTest]]: [MySQL] 40,491 pass(es). View
#1 Screen shot 2012-05-20 at 1.38.17 PM.png34 KBkpoeppe

Comments

jasonrichardsmith@gmail.com’s picture

Issue summary: View changes

updated issue for clarity

kpoeppe’s picture

Version: 7.12 » 8.x-dev
Issue tags: +D8UX usability, +tcdrupal2012
FileSize
34 KB

Summary: I could not reproduce the original problem. The file attach widget was available to me after I uploaded a file that exceeded the max size, and it was easy to upload a different file. However, the error message from my first attempt persisted after I successfully uploaded a smaller file. I don't think users need to continue to see the error message once a new file is successfully uploaded. (see attached screenshot)

Test process:
1) Created a file upload field. Set max file size to 40KB.
2) Attempted to upload a file larger than 40 KB.

Results:
I got an error message "The specified file Why Drupal 7.docx could not be uploaded. The file is 92.84 KB exceeding the maximum file size of 40 KB." The file upload widget (Browse button, Upload button) remained visible. I didn't have to refresh my browser.

Next, I successfully uploaded a new, smaller file.

However: the error message from my first attempt to upload a file was still on the screen. I don't think users need to continue to see the error message once a file is successfully uploaded. (see attached screenshot)

Liam Morland’s picture

Liam Morland’s picture

There are two different messages and file upload limits:

  1. If the uploaded file is larger than the limit configured in the content type, but smaller than the upload limit configured in php.ini, you will get the message "The specified file %name could not be uploaded. The file is %filesize exceeding the maximum file size of %maxsize." This message is in file.inc, in two parts.
  2. If the uploaded file is larger than the upload limit configured in php.ini, you will get the message, "An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports." This message is in file.module.

In this second case only, the upload wiget disappears and any other file already uploaded also disappears. If you save at this point, any other files uploaded will be removed from the node. I am seeing this in D7.22.

Liam Morland’s picture

However: the error message from my first attempt to upload a file was still on the screen. I don't think users need to continue to see the error message once a file is successfully uploaded. (see attached screenshot)

This is a duplicate of #1792032: File validation error message not removed after subsequent upload of valid file.

Liam Morland’s picture

After reading the fix to #1792032: File validation error message not removed after subsequent upload of valid file, I think this is the problem: In file_ajax_upload(), the first call to ajax_command_replace() replaces everything with the error message. The calls to ajax_command_replace() later in file_ajax_upload() include the rendered version of the form in the replacement, so the form still shows.

Liam Morland’s picture

Version: 8.x-dev » 7.x-dev
Status: Active » Needs review

Here is a D7 patch. I replaced the call to ajax_command_replace() with a call to ajax_command_prepend(), so the error message gets prepended without disturbing the form.

Liam Morland’s picture

FileSize
694 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,491 pass(es). View
Liam Morland’s picture

Version: 7.x-dev » 8.x-dev
FileSize
736 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,022 pass(es). View

D8 version.

Status: Needs review » Needs work

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

Liam Morland’s picture

Sorry, my D8 testing box is not working now. I just did what I thought was equivalent.

Liam Morland’s picture

Status: Needs work » Needs review
Issue tags: -tcdrupal2012

The failures do not appear to be related to anything changed by the patch. Trying again in case it was a problem with the testing process.

Liam Morland’s picture

Issue tags: -D8UX usability

I didn't mean to change the tagging.

Liam Morland’s picture

Issue tags: +D8UX usability, +tcdrupal2012

Fixing tags.

Liam Morland’s picture

Fixing tags.

Liam Morland’s picture

Liam Morland’s picture

Component: file system » file.module
Liam Morland’s picture

FileSize
736 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,481 pass(es). View

Reroll.

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

Liam Morland’s picture

Status: Needs work » Needs review
Liam Morland’s picture

Liam Morland’s picture

Liam Morland’s picture

Version: 8.x-dev » 7.x-dev
FileSize
694 bytes
FAILED: [[SimpleTest]]: [MySQL] 41,197 pass(es), 2 fail(s), and 2 exception(s). View

D8 code has been completely rewritten, removing the function that was patched, but the issue remains in D7.

Liam Morland’s picture

Liam Morland’s picture

Issue summary: View changes

edited some formatting

Liam Morland’s picture

Liam Morland’s picture

a.milkovsky’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I confirm the patch. It's better to prepend error instead of relapsing the whole file widget.
It should be changed.
Thank you, Liam.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: core_1489692_file_upload_ajax_message.patch, failed testing.

Status: Needs work » Needs review
Liam Morland’s picture

Status: Needs review » Reviewed & tested by the community

Restoring previous status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: core_1489692_file_upload_ajax_message.patch, failed testing.

a.milkovsky’s picture

omg.
@Liam, maybe you need to re-roll it with last dev version?

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
694 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_1489692_file_upload_ajax_message.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a reroll, but I don't think that is the problem. The errors seem to be unrelated to the patch and it passes some of the time.

Liam Morland’s picture

Status: Needs review » Reviewed & tested by the community

Restoring previous status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: core_1489692_file_upload_ajax_message.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: core_1489692_file_upload_ajax_message.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: core_1489692_file_upload_ajax_message.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: core_1489692_file_upload_ajax_message.patch, failed testing.

Status: Needs work » Needs review
Liam Morland’s picture

Status: Needs review » Reviewed & tested by the community
Roya-Rateshtari’s picture

Hi,

This patch worked for me.

Thanks

David_Rothstein’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +needs backport to D7

Are we really sure this doesn't affect Drupal 8? I see what looks like equivalent code in core/modules/file/src/Controller/FileWidgetAjaxController.php:

    if (empty($request_form_build_id) || $form_build_id !== $request_form_build_id) {
      // Invalid request.
      drupal_set_message(t('An unrecoverable error occurred. The uploaded file likely exceeded the maximum file size (@size) that this server supports.', array('@size' => format_size(file_upload_max_size()))), 'error');
      $response = new AjaxResponse();
      $status_messages = array('#theme' => 'status_messages');
      return $response->addCommand(new ReplaceCommand(NULL, drupal_render($status_messages)));
    }

Also, if it's true that this one case should be ajax_command_prepend() but the other two calls later in the function should remain ajax_command_replace(), this is screaming for a code comment explaining why :) I would at least think that if this one needs to be ajax_command_prepend() then the one after it should be too...

Liam Morland’s picture

I don't know if it effects D8 or not. It looked to me like the code had been rewritten, so I moved this to just being a D7 issue.

I only noticed the problem in the place that I fixed it. It seems reasonable that they should all be the same. I just haven't run into the problem with the other code.

Status: Needs review » Needs work

The last submitted patch, 32: core_1489692_file_upload_ajax_message.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
988 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Re-roll of D8 patch.

Status: Needs review » Needs work

The last submitted patch, 51: core-file_upload_ajax_message-1489692-51-D8.patch, failed testing.

Status: Needs work » Needs review
Liam Morland’s picture

It looks like the old testing system is saying "Queued for testing", but it has passed the new tests, so it should be OK.

Liam Morland’s picture

#@47: ReplaceCommand() no longer appears in FileWidgetAjaxController.php.

Status: Needs review » Needs work

The last submitted patch, 51: core-file_upload_ajax_message-1489692-51-D8.patch, failed testing.

Liam Morland’s picture

How can I see which test assertion failed?

Liam Morland’s picture

Status: Needs work » Closed (cannot reproduce)

I am no longer able to reproduce this problem on either D8 or D7. Thanks for whomever fixed it in some other issue.

David_Rothstein’s picture

Title: Incorrect handling of file upload limit exceeded » Incorrect handling of file upload limit exceeded - file widget disappears
Status: Closed (cannot reproduce) » Needs work

I can still reproduce this in both Drupal 7 and Drupal 8. Just add a file field to the Basic Page content type, make it accept .gz files for upload, then create a basic page and try to upload some gigantic .gz file that's bigger than the server limit. The error message is displayed, but the file widget disappears from the screen.

Liam Morland’s picture

Are there more than one upload limit, such as one in Drupal and one in PHP? Maybe that is why the problem appeared to vanish for me.

David_Rothstein’s picture

I believe I tested the scenario where the only upload limit was the PHP one.

Liam Morland’s picture

David, that sounds familiar. It could be that the problem happens when the PHP limit is reached, but not the Drupal limit.

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.

sudishth’s picture

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: core-file_upload_ajax_message-1489692-D8.patch, failed testing.

The last submitted patch, 64: core-file_upload_ajax_message-1489692-D8.patch, failed testing.

josmera01’s picture

Liam Morland’s picture

Status: Needs work » Needs review
juanramonperez’s picture

Status: Needs review » Reviewed & tested by the community

#68 It works for me.

Thanks!

YesCT’s picture

Issue tags: +DevDaysMilan

@juanramonperez Thanks for manually testing this!

I'm (at devdays) and going to do a code review.

I also sent the previous patch to test against 8.2.x also. :)

YesCT’s picture

@josmera01 thanks for changing that test.

Uploading a test only patch, to verify the test will fail without the fix.

(I have the thought that we might want to try in a test to upload a file over the limit, and then assert the error is appropriate, and that the widget is there (and the previously uploaded files), and that another file can be uploaded after the error, and then the error disappears... need to look into it more and think more)

YesCT’s picture

Issue summary: View changes

@Liam Morland Thanks so much for the detail (in #3) about the php limit vs the drupal file size limit. Adding that to the summary to keep that information easy to find.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 72: core-file_upload_ajax_message-1489692-68-testonly.patch, failed testing.

The last submitted patch, 72: core-file_upload_ajax_message-1489692-68-testonly.patch, failed testing.

YesCT’s picture

Issue summary: View changes

I think this is needs work for a test that asserts the correct behavior (and the absence of the wrong behavior).

Gábor Hojtsy’s picture

Issue tags: +Media Initiative
Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix for use of current tag. Sorry for using the wrong one.

Gábor Hojtsy’s picture

Issue tags: -D8UX usability +Usability

Also fix usability tag.

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.

keesje’s picture

Patch in #68 confirmed as working. Is it possible to get this fix in, and open a new issue for adding proper tests?

Martijn Houtman’s picture

Fixing #68 in Drupal 7 also solves the problem there.