Display generic message and log detailed message when file upload fails due to PHP error

no patch

This seems to still be the case in 5.2.

Patch, based on above code:

--- includes/file.inc.orig        2007-05-31 01:48:58.000000000 -0400
+++ includes/file.inc  2007-09-26 01:50:43.000000000 -0400
@@ -192,6 +192,11 @@
     return $upload_cache[$source];
   }

+  if ($_FILES["files"]["error"][$source] && $_FILES["files"]["error"][$source] == UPLOAD_ERR_NO_TMP_DIR) {
+    drupal_set_message(t('The file %file could not be saved, because no server temp folder exists.', array('%file' => $_FILES["files"]["name"][$source])), 'error');
+    return FALSE;
+  }
+
   // If a file was uploaded, process it.
   if ($_FILES["files"]["name"][$source] && is_uploaded_file($_FILES["files"]["tmp_name"][$source])) {

No patch.

Due to the age of the last comment on this issue and due to the fact that D5 is no longer supported, I am closing this issue.

Going to open this issue until I can confirm it doesn't exist in modern Drupal.

I can test this, this weekend.

Patch still applies, and provides more detailed feedback when such errors occur.

I think due to the procedural nature of file_save_upload(), writing a test for these two errors would be quite difficult. If/when we ever switch to use the Symfony file validators, they provide complete coverage of these errors (as unit tests).

I've updated the issue summary to include a beta phase evaluation.

Back to RTBC. Not sure what's going on with the testbot today.

If we are doing this we should make this complete there's now UPLOAD_ERR_EXTENSION see http://php.net/manual/en/features.file-upload.errors.php

I agree that testing this is going to be unnecessarily tricky.

This adds UPLOAD_ERR_EXTENSION. I took the error text directly from the php manual.

Oh yes, I've been bitten by this one a couple of times already.

Committed/pushed to 8.0.x, thanks!

+ drupal_set_message(t('The file %file could not be saved because server is missing a temporary folder.', array('%file' => $file_info->getFilename())), 'error');
+ drupal_set_message(t('The file %file could not be saved because server is unable to write to disk.', array('%file' => $file_info->getFilename())), 'error');
+ drupal_set_message(t('The file %file could not be saved because a PHP extension stopped the file upload. PHP does not provide a way to ascertain which extension caused the file upload to stop; examining the list of loaded extensions with phpinfo() may help.', array('%file' => $file_info->getFilename())), 'error');

These technical details should not be shown to end users; 99% won't understand what it means and can't do anything to fix it. It might even be considered a small security flaw, since it exposes information about how the server is configured.

These could perhaps be watchdog messages instead, with the end-user-facing message changed from "unknown error" to "server error" or similar...

But first I think this should be rolled back because it causes more problems than it solves.

More accurate title, also. And we can probably backport at least some of this (e.g. watchdog messages).

That's a good point. Reverted the commit.

What about changing these errors to a trigger_error() so it shows up both on screen depending on error reporting levels and in watchdog. Then the more restricted message with dsm() sounds good.

Hm, I think trigger_error() makes sense, although while trying that I found that the triggered error usually won't be displayed even if it's configured to (since file uploads usually happen via Ajax and Drupal only displays errors to the screen during Ajax if they're fatal, which usually makes sense but in this case seems like a bug).

I also wasn't sure how to handle translation here - do we not run through t() since the primary destination is watchdog or do we translate since it might appear on the screen?

Here's a patch that tries trigger_error() anyway. I also fixed a grammar issue in a couple of the messages ("because server" => "because the server").

...I found that the triggered error usually won't be displayed even if it's configured to (since file uploads usually happen via Ajax and Drupal only displays errors to the screen during Ajax if they're fatal, which usually makes sense but in this case seems like a bug).

Eventually decided that this really seems like a bug - filed #2400477: Drupal never displays non-fatal error messages to the screen during Ajax requests

I think the patch in #25 is good to go, but should it perhaps wait on #2400477: Drupal never displays non-fatal error messages to the screen during Ajax requests?

Rerolled the patch,

Can you reroll patch #36 for Drupal 8.9 and 9.x, please?

Re-rolled for 8.9.x

1. +++ b/core/modules/file/file.module
   @@ -976,6 +977,18 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +      $error_to_log = t('The file %file could not be saved because the server is missing a temporary folder.', ['%file' => $file_info->getFilename()]);
   ...
   +      $error_to_log = t('The file %file could not be saved because the server is unable to write to disk.', ['%file' => $file_info->getFilename()]);
   ...
   +      $error_to_log = t('The file %file could not be saved because a PHP extension stopped the file upload. PHP does not provide a way to ascertain which extension caused the file upload to stop; examining the list of loaded extensions with phpinfo() may help.', ['%file' => $file_info->getFilename()]);
   
   @@ -985,11 +998,21 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +      $error_to_log = t('The file %file could not be saved. An unknown error has occurred.', array('%file' => $file_info->getFilename()));
   

   Log messages are not translated.

2. +++ b/core/modules/file/file.module
   @@ -985,11 +998,21 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +    $files[$i] = FALSE;
   

   This needs to return FALSE here.

3. +++ b/core/modules/file/file.module
   @@ -985,11 +998,21 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   -  // Begin building file entity.
   +
   

   This line shouldn't be removed.

Addressed #45.

This needs to be on 9.2.x and it needs a reroll. Needs IS update - at least have the proposed resolution and the remaining tasks to make it easier to find out what needs to be done here.

Reroll patch given for 9.2.x.

Fixing some phpcs issues. Please have a look.

+++ b/core/modules/file/file.module
@@ -943,6 +944,18 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
+      $error_to_log = 'The file %file could not be saved because the server is missing a temporary folder.';

+++ b/core/modules/file/file.module
@@ -943,6 +944,18 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
+      $error_to_log = 'The file ' . $file_info->getFilename() . ' could not be saved because the server is unable to write to disk.';

How to write the test when this triggers and error?

I also think this needs a screenshot of the screen output when one of the newly detected errors occurs. If you make a screenshot - put it in the IS. Thx.

Adding tests and implemented fixes for #50.

Ignore that, forgot to run commit-code-check.

More or less started over. This attempts to properly display a message to the user and log an useful, more detailed message. The test has been updated to check both messages.

Update title to what this is doing.

OK. Let's get the max upload size differently.

Read the code, all seems logical.
Looks like a very positive change to have.

I'm afraid I couldn't apply the patch to 9.3.x - any extra information could be useful to apply the patch here.
Else-wise it's line 17 on the Kernel test file which it's failing at.

The patch appears to be against 9.2.x, have rerolled against 9.3.x and added keys to the dataProvider to make it easier to see what (if anything) fails.

Excellent. The re-roll worked @mstrelan.
Looks great, applies cleanly.

Thanks for the updated patch. Here's some feedback:

1. +++ b/core/modules/file/file.module
   @@ -957,9 +970,17 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   -      \Drupal::messenger()->addError(t('The file %file could not be saved. An unknown error has occurred.', ['%file' => $original_file_name]));
   -      return FALSE;
   -
   +      $error_to_log = 'The file ' . $file_info->getFilename() . ' could not be saved. An unknown error has occurred.';
   

   Should use $original_file_name instead of $file_info->getFilename().

2. +++ b/core/modules/file/file.module
   @@ -957,9 +970,17 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +    // problems that end users should not know about (and cannot fix by
   +    // attempting the upload again).
   

   Nitpick: IMO the parentheses in the comment are unnecessary.

3. +++ b/core/modules/file/tests/src/Kernel/FileModuleTest.php
   @@ -17,21 +17,80 @@ class FileModuleTest extends KernelTestBase {
   +    $expected_message = new TranslatableMarkup("The file %file could not be saved. An unknown error has occurred.", ['%file' => $file_name]);
   

   Nitpick: Should be single quotes rather than double.

Addressed #63.1, 63.2 and 63.3, Please have a look.

This looks good to me but I can't RTBC.

The issue summary might need an update:

check for the following errors and show a useful error message

However, we have only changed the message that gets logged, the message that is presented to the user is still:

+++ b/core/modules/file/file.module
@@ -957,9 +970,17 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
+    \Drupal::messenger()->addError(t('The file %file could not be saved. An unknown error has occurred.', ['%file' => $original_file_name]));

.

1. For the message that is presented to the user, instead of "an unknown error", what about "a server error" and indicate that the details have been logged? (Suggested a long long time ago in #20.)

+++ b/core/modules/file/file.module
@@ -957,9 +970,17 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
+    \Drupal::messenger()->addError(t('The file %file could not be saved due to a server error. Error details have been logged.', ['%file' => $original_file_name]));

2. Also, I updated the issue summary about the user-facing error message. (Perhaps could still use an IS update about steps to reproduce?)

@kim.pepper
As per the comment in #20, we can skip the updating user facing message.

@Amber Himes Matz
I've updated the patch and test with the message The file %file could not be saved. A server error has occurred.
Do you think we need to get approval from UX team as well or we are good?

+++ b/core/modules/file/file.module
@@ -957,9 +970,17 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
+    \Drupal::messenger()->addError(t('The file %file could not be saved. A server error has occurred.', ['%file' => $original_file_name]));

Looks like here is where the generic user message is constructed. Thank you @mohit_aghera for the update.

Yes, though minor, I do think it's a good idea for the UX team to have a look. So I'll add a tag.

I'm going to try to reproduce one of the errors and add a screenshot of the user-facing error message for the benefit of the usability review.

Updated title, since this issue is specific to error messages that PHP returns, not errors about file field configuration. (See http://php.net/manual/features.file-upload.errors.php.)

Updated issue summary, per comments that indicated that $original_file_name should be used and not $file_info->getFilename(). Also added the missing cases to the IS. And updated point 2 about the user-facing message contents.

So, in my attempt to reproduce a PHP file upload error, I updated my local site's PHP ini with the following values:

upload_tmp_dir = "/monkey"
upload_max_filesize = 10

I added a File field to the Article content type with the defaults, and the help text does show a file size limit of 10 bytes, as set in my INI file, with a file type limit of ".txt".

I then tried to upload INSTALL.txt (94 bytes) to a new article. This is what happened:

1. No error was displayed by AJAX when I uploaded the file. The file name value remained in the field.
2. No error was displayed in the messages block and the file was successfully saved, albeit without a file upload attached. (The file upload error was essentially silent.)
3. The error message was logged as follows:

Type 	file
Date 	Friday, September 24, 2021 - 15:46
User 	admin
Location 	https://92944-fileuploaderrmsg.ddev.site/node/add/article?_wrapper_format=drupal_ajax&ajax_form=1&element_parents=field_file%2Fwidget%2F0
Referrer 	https://92944-fileuploaderrmsg.ddev.site/node/add/article
Message 	The file INSTALL.txt could not be saved because it exceeds 10 bytes, the maximum allowed size for uploads.
Severity 	Error

So, I am not convinced that the if (isset($error_to_log)) block on lines 973-982 in core/modules/file.module are running "unconditionally". As, what is supposed to happen is that a helpful technical message is logged AND a generic helpful-but-not-detailed-and-not-unknown error message is shown to the user.

I am also not sure if this is something to do with AJAX and we're in the wrong file. I'm also not sure if there is a server configuration or permissions issue that prevents this type of PHP error message from being displayed. I tried turning display_errors =1 in my ini file, and I tested the file upload with admin, content editor, and limited editor users.

The changes here are in a function that is now deprecated. It was deprecated in #3232248: Move _file_save_upload_single to a service and deprecate. Perhaps the work here should be in function file_save_upload()?

@quietone++ the work he should definitely only change file_save_upload(). It shouldn't touch the new FileUploadHandler service.

@quietone @alexpott
I've refactored the code and moved all the logic to "file_save_upload()" method.
I had to re-roll the patch as well because we renamed test cases classes. Added re-roll diff file as well.
I've added new test cases instead of modifying existing one to keep things simple.

@mohit_aghera, thanks for updating the patch. Unfortunately, the patch no longer applies but it was just the changes to the test so I went ahead and did some manual testing.

To generate a On the webserver I moved /tmp to /tmp.bkp and then uploaded a file. That resulted in the same message displayed to the user and in the logs.

UI

Logged

The message logged and the message displayed are supposed to be different. The user can't do anything about the server so they don't need to know the details. This agrees with what Amber Himes Matz said #72 "As, what is supposed to happen is that a helpful technical message is logged AND a generic helpful-but-not-detailed-and-not-unknown error message is shown to the user.'

Do we have agreement on the text of the generic message that is shown to the user? What is in the IS is not in the patch.

Rerolled against 9.3.x

@quietone don't hesitate to add "Needs reroll" tag next time)

@kostyashupenko, just a reminder to always and an interdiff or diff when rerolling a patch. And I did not add the tag because this needs more work than a reroll.

This needs work for #76 and a diff between the patches in #75 and #77.

Added reroll diff of patch #75 and #77.

Here is a patch for 9.4

I attempted to re-roll for 11.x and convert this to an MR but it proved difficult due to changes in #3375447: Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface. I wondered if this issue is still relevant but it is lacking steps to reproduce.

I looked at the diff in https://git.drupalcode.org/project/drupal/-/commit/6bd3ad841f346d650ace3... and I also can't tell what's left here.

::handleFileUpload() creates violations, these are then formatted into a messenger message here: https://git.drupalcode.org/project/drupal/-/commit/6bd3ad841f346d650ace3...

The issue summary is saying that instead of listing the errors, we should log them. The errors we list now are validation constraints, not the raw message, so I think that's fine.

However, it looks like whereas we used to cover UPLOAD_ERR_NO_TMP_DIR as an explicit case in FileUploadHandler::handleFileUpload() https://git.drupalcode.org/project/drupal/-/commit/6bd3ad841f346d650ace3... that wasn't handled in the new code, and I can't see anywhere else in core that it's handled. Symfony's FileValidator does handle it explicitly but I don't think we're using that.

Given all that, if the above is correct, which I'm not 100% sure it is, then I think what we're missing is logging the full range of errors we used to throw exceptions for, but now only show a handful of violation messages for.

I just committed #2838474: Remove dependency of "file_system" service on "logger" which I'm not sure directly affects this issue but might influence the eventual outcome even if it doesn't.

I believe it's because of how we move uploaded files.

We call \Drupal\Core\File\FileSystem::moveUploadedFile() which calls \move_uploaded_file(). We don't call \Symfony\Component\HttpFoundation\File\UploadedFile::move() which would throw those exceptions. From looking at the comments, due to a lack of stream wrapper support.

We could add those missing validations to \Drupal\file\Validation\Constraint\UploadedFileConstraintValidator::validate(), I'm just not sure if they are triggered before we try and move the file.

We are catching UPLOAD_ERR_NO_TMP_DIR in https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...

      default => $this->context->buildViolation($constraint->uploadErrorMessage, [
        '%file' => $value->getClientOriginalName(),
      ])->setCode((string) $value->getError())
        ->addViolation()

This means this issue is about adding additional logging to file_save_upload() which would reclassify it as a task, IMO.