- Patch #573300 by eMPee584: system_retrieve_file() fails in file_unmanaged_copy()

modules/system/system.module

@@ -3099,33 +3099,50 @@ function system_image_toolkits() {
  *   The URL of the file to grab.
  *
  * @param $destination
- *   Where the file should be saved, if a directory is provided, file is saved
- *   in that directory with its original name.  If a filename is provided,
- *   remote file is stored to that location.  NOTE: Relative to drupal "files" directory"
- *
- * @param $overwrite boolean
- *   Defaults to TRUE, will overwrite existing files of the same name.
+ *   Stream wrapper URI specifying where the file should be placed. If a
+ *   directory path is provided, the file is saved into that directory under
+ *   its original name. If the path contains a filename as well, that one will
+ *   be used instead.
+ *   If this value is omitted, the site's default files scheme will be used,
+ *   usually "public://".
+ *
+ * @param $managed boolean
+ *   If this is set to TRUE, the file API hooks will be invoked and the file is
+ *   registered in the database.
+ *
+ * @param $replace boolean
+ *   Replace behavior when the destination file already exists:
+ *   - FILE_EXISTS_REPLACE: Replace the existing file.
+ *   - FILE_EXISTS_RENAME: Append _{incrementing number} until the filename is
+ *     unique.
+ *   - FILE_EXISTS_ERROR: Do nothing and return FALSE.
  *
  * @return
- *   On success the address the files was saved to, FALSE on failure.
+ *   On success the location the file was saved to, FALSE on failure.
  */
-function system_retrieve_file($url, $destination = NULL, $overwrite = TRUE) {
-  if (!$destination) {
-    $destination = file_directory_path('temporary');
-  }
+function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $replace = FILE_EXISTS_RENAME) {
   $parsed_url = parse_url($url);
-  $local = is_dir(file_directory_path() . '/' . $destination) ? $destination . '/' . basename($parsed_url['path']) : $destination;
-
-  if (!$overwrite && file_exists($local)) {
-    drupal_set_message(t('@remote could not be saved. @local already exists', array('@remote' => $url, '@local' => $local)), 'error');
-    return FALSE;
+  if (!isset($destination)) {
+    $path = file_build_uri(basename($parsed_url['path']));
+  }
+  else {
+    if (is_dir(drupal_realpath($destination))) {
+      // Prevent URIs with triple slashes when glueing parts together.
+      $path = str_replace('///', '//', "$destination/") . basename($parsed_url['path']);
+    }
+    else {
+      $path = $destination;
+    }
   }
-
   $result = drupal_http_request($url);
-  if ($result->code != 200 || !file_save_data($result->data, $local)) {
-    drupal_set_message(t('@remote could not be saved.', array('@remote' => $url)), 'error');
+  if ($result->code != 200) {
+    drupal_set_message(t('HTTP error @errorcode occured when trying to fetch @remote.', array('@errorcode' => $result->code, '@remote' => $url)), 'error');
     return FALSE;
   }
+  $local = $managed ? file_save_data($result->data, $path, $replace) : file_unmanaged_save_data($result->data, $path, $replace);
+  if (!$local) {
+    drupal_set_message(t('@remote could not be saved to @path.', array('@remote' => $url, '@path' => $path)), 'error');
+  }
 
   return $local;
 }

modules/system/system.test

@@ -1599,6 +1599,50 @@ class FloodFunctionalTest extends DrupalWebTestCase {
   }
 }
 
+/**
+ * Test HTTP file downloading capability.
+ */
+class RetrieveFileTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'HTTP file retrieval',
+      'description' => 'Checks HTTP file fetching and error handling.',
+      'group' => 'System',
+    );
+  }
+
+  /**
+   * Invokes system_retrieve_file() in several scenarios.
+   */
+  function testFileRetrieving() {
+    // Test 404 handling by trying to fetch a randomly named file.
+    drupal_mkdir($sourcedir = 'public://' . $this->randomName());
+    $filename = $this->randomName();
+    $url = file_create_url($sourcedir . '/' . $filename);
+    $retrieved_file = system_retrieve_file($url);
+    $this->assertFalse($retrieved_file, t('Non-existent file not fetched.'));
+
+    // Actually create that file, download it via HTTP and test the returned path.
+    file_put_contents($sourcedir . '/' . $filename, 'testing');
+    $retrieved_file = system_retrieve_file($url);
+    $this->assertEqual($retrieved_file, 'public://' . $filename, t('Sane path for downloaded file returned (public:// scheme).'));
+    $this->assertTrue(is_file($retrieved_file), t('Downloaded file does exist (public:// scheme).'));
+    $this->assertEqual(filesize($retrieved_file), 7, t('File size of downloaded file is correct (public:// scheme).'));
+    file_unmanaged_delete($retrieved_file);
+
+    // Test downloading file to a different location.
+    drupal_mkdir($targetdir = 'temporary://' . $this->randomName());
+    $retrieved_file = system_retrieve_file($url, $targetdir);
+    $this->assertEqual($retrieved_file, "$targetdir/$filename", t('Sane path for downloaded file returned (temporary:// scheme).'));
+    $this->assertTrue(is_file($retrieved_file), t('Downloaded file does exist (temporary:// scheme).'));
+    $this->assertEqual(filesize($retrieved_file), 7, t('File size of downloaded file is correct (temporary:// scheme).'));
+    file_unmanaged_delete($retrieved_file);
+
+    file_unmanaged_delete_recursive($sourcedir);
+    file_unmanaged_delete_recursive($targetdir);
+  }
+}
+
 /**
  * Functional tests shutdown functions.
  */