Issue #3475892 by joseph.olstad, jenlampton, yesct: Improved handling of failed uploads

file_entity.pages.inc

@@ -629,6 +629,14 @@ function file_entity_add_upload_submit($form, &$form_state) {
   if ($save) {
     $file = file_load($form_state['storage']['upload']);
     if ($file) {
+      $file->type = $form_state['storage']['type'];
+      $file->display = TRUE;
+      // Save the form fields.
+      // Keep in mind that the values for the Field API fields must be in
+      // $form_state['values'] and not in ['storage']. This is true as long as
+      // the fields are on the last page of the multi step form.
+      entity_form_submit_build_entity('file', $file, $form, $form_state);
+
       if (file_uri_scheme($file->uri) != $form_state['storage']['scheme']) {
         $file_destination = $form_state['storage']['scheme'] . '://' . file_uri_target($file->uri);
         $file_destination = file_stream_wrapper_uri_normalize($file_destination);
@@ -638,23 +646,38 @@ function file_entity_add_upload_submit($form, &$form_state) {
         if ($moved_file = file_move($file, $file_destination, FILE_EXISTS_RENAME)) {
           // Only re-assign the file object if file_move() did not fail.
           $file = $moved_file;
+          $file_in_final_location = TRUE;
+        }
+        else {
+          $file_in_final_location = FALSE;
         }
       }
-      $file->type = $form_state['storage']['type'];
-      $file->display = TRUE;
-
-      // Change the file from temporary to permanent.
-      $file->status = FILE_STATUS_PERMANENT;
+      else {
+        $file_in_final_location = TRUE;
+      }
 
-      // Save the form fields.
-      // Keep in mind that the values for the Field API fields must be in
-      // $form_state['values'] and not in ['storage']. This is true as long as
-      // the fields are on the last page of the multi step form.
-      entity_form_submit_build_entity('file', $file, $form, $form_state);
+      $substitutions = array(
+        '@type' => file_entity_type_get_name($file),
+        '%name' => $file->filename,
+      );
 
-      file_save($file);
-      $form_state['file'] = $file;
-      drupal_set_message(t('@type %name was uploaded.', array('@type' => file_entity_type_get_name($file), '%name' => $file->filename)));
+      // Only save the file if any necessary move was successful.
+      if ($file_in_final_location) {
+        // Change the file from temporary to permanent.
+        $file->status = FILE_STATUS_PERMANENT;
+        file_save($file);
+        // Record that the module (in this case, the File module) is using the
+        // file. Without the call to file_usage_add, file_managed_file_validate()
+        // produces an error upon saving the form, saying that the uploaded file
+        // may not be referenced.
+        file_usage_add($file, 'file', 'file', $file->fid);
+        $form_state['file'] = $file;
+        drupal_set_message(t('@type %name was uploaded.', $substitutions));
+      }
+      else {
+        file_delete($file, TRUE);
+        drupal_set_message(t('@type %name was not saved.', $substitutions), 'error');
+      }
     }
     else {
       drupal_set_message(t('An error occurred and no file was uploaded.'), 'error');

file_entity.test

@@ -457,6 +457,50 @@ class FileEntityUploadWizardTestCase extends FileEntityTestHelper {
     $this->assertRaw(t('!type %name was uploaded.', array('!type' => 'Document', '%name' => $file->filename)), t('Document file uploaded.'));
   }
 
+
+  /**
+   * Test the file upload wizard when uploading a private file fails.
+   */
+  function testFileUploadWizardPrivateFailure() {
+    $test_file = $this->getTestFile('text');
+    // Record the last saved file ID.
+    $previous_last_fid = $this->getLastFileId();
+    // Enable the private file system.
+    variable_set('file_private_path', $this->private_files_directory);
+    // Step 1: Upload a basic document file.
+    $edit = array();
+    $edit['files[upload]'] = drupal_realpath($test_file->uri);
+    $this->drupalPost('file/add', $edit, t('Next'));
+    // After the file is uploaded but before the file is saved to its final
+    // location, update the private files directory to an non-writable location.
+    // The /root directory is the home directory of the root user, and should
+    // never be writable by the web user. As long as the directory either does
+    // not exist, or is not writable, this example should work.
+    $temp_private_location = '/root';
+    variable_set('file_private_path', $temp_private_location);
+    // Now the file is in a temporary location. Load its entity.
+    $new_fid = $this->getLastFileId();
+    $new_file = file_load($new_fid);
+    $this->assertTrue(file_exists($new_file->uri), 'File is temporarily uploaded.');
+    // Step 3: Scheme selection.
+    $edit = array();
+    $edit['scheme'] = 'private';
+    $this->drupalPost(NULL, $edit, t('Next'));
+    // Check that a message was printed indicating the file was not saved.
+    $this->assertRaw(t('@type %name was not saved.', array('@type' => 'Document', '%name' => $test_file->filename)), 'Failed private upload was deleted.');
+    // Check that the file is no longer on disk.
+    clearstatcache($new_file->uri);
+    $this->assertFalse(file_exists($new_file->uri), 'Temporary file on disk has been deleted.');
+    // Check that the file entity was deleted.
+    $last_fid = $this->getLastFileId();
+    $this->assertTrue($previous_last_fid == $last_fid, 'Latest file entity remains the same.');
+    $this->assertFalse($new_fid == $last_fid, 'New file entity was deleted when file fails to upload.');
+    // Reset the private file path so the testing framework doesn't try to
+    // clean up the non-writable directory.
+    variable_del('file_private_path');
+  }
+
+
   /**
    * Test the file upload wizard type step.
    */