This function has not been updated for the new stream wrapper URIs in file.inc.. Thatfor it was broken and tried to give a 'normal' file system path as destination which ended up causing file_unmanaged_copy to complain it could not copy the file there (=> #573288: File.inc function documentation: clarify that several functions _must_ be fed a stream wrapper URI)- relevant code in file.inc:

  // Assert that the destination contains a valid stream.
  $destination_scheme = file_uri_scheme($destination);
  if (!$destination_scheme || !file_stream_wrapper_valid_scheme($destination_scheme)) {
    drupal_set_message(t('The specified file %file could not be copied, because the destination %destination is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper.', array('%file' => $original_source, '%destination' => $destination)), 'error');
  }

The first patch adds a testcase for fetching a remote file, the other one repairs system_retrieve_file() after which the tests pass. Hope i haven't missed anything, comments appreciated.

CommentFileSizeAuthor
#15 system-module-repair-system_retrieve_file-with-testcase-v4.patch5.67 KBeMPee584
#12 system-module-repair-system_retrieve_file-with-testcase-v3.patch5.61 KBeMPee584
#4 system-module-repair-system_retrieve_file-with-testcase-v2.patch5.61 KBeMPee584
#1 system-module-repair-system_retrieve_file-with-testcase.patch5.33 KBeMPee584
system-module-repair-system_retrieve_file.patch2.98 KBeMPee584
system-module-retrieve-file-testcase.patch2.35 KBeMPee584
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented
Issue tags: +Needs tests, +API clean-up
FileSize
system-module-repair-system_retrieve_file-with-testcase.patch5.33 KB

both patches combined => should pass the tests..

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ modules/system/system.module
@@ -3061,33 +3061,38 @@ function system_image_toolkits() {
  * @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 fileis stored to that location.  NOTE: Relative to drupal "files" directory"
+ *   Stream wrapper URI telling Drupal where the file should be placed. If a
+ *   directory path is provided the file is saved into that directory with its
+ *   original name or, if given aswell, a different filename.
+ *   If this value is omitted, Drupal's default files scheme will be used,
+ *   usually "public://".

Can we remove the word "Drupal" from here?

The last one can be replaced with "site's default files scheme".

+++ modules/system/system.module
@@ -3061,33 +3061,38 @@ function system_image_toolkits() {
+ * @param $managed boolean
+ *   If this is set to TRUE the file API hooks will be invoked and
+ *   the file is registered in the database.

Does not wrap at 80 chars.

+++ modules/system/system.module
@@ -3061,33 +3061,38 @@ function system_image_toolkits() {
+ *   - FILE_EXISTS_RENAME - Append _{incrementing number} until the filename is
+ *       unique.

Improper indentation: "unique" should be located right below "FILE_...".

We also use colons (:) to separate option values from their descriptions (not hyphens).

+++ modules/system/system.module
@@ -3061,33 +3061,38 @@ function system_image_toolkits() {
+  $path = !isset($destination) ? file_build_uri(basename($parsed_url['path'])) : (is_dir(drupal_realpath($destination)) ? str_replace('///', '//', "$destination/") . basename($parsed_url['path']) : $destination);

Holy cow. Can we write that more readable by assigning the path differently in an if/else statement? :)

+++ modules/system/system.test
@@ -1227,3 +1227,47 @@ class TokenReplaceTestCase extends DrupalWebTestCase {
+ * Test http file downloading capability.

HTTP always all-uppercase.

+++ modules/system/system.test
@@ -1227,3 +1227,47 @@ class TokenReplaceTestCase extends DrupalWebTestCase {
+class RetrieveFileTestCase extends DrupalWebTestCase {

You're sure there isn't a private files test already?

+++ modules/system/system.test
@@ -1227,3 +1227,47 @@ class TokenReplaceTestCase extends DrupalWebTestCase {
+  /**
+    * Invokes system_retrieve_file() in several scenarios.
+    */

Wrong indentation of PHPDoc comment here.

This review is powered by Dreditor.

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented
Status: Needs work » Needs review
FileSize
system-module-repair-system_retrieve_file-with-testcase-v2.patch5.61 KB

Thx for reviewing sun and sorry it took me so long. Synced my D7 up to date, checked the patch (and test) still works as intended and followed your recommendations. Regarding your question, yes tests for the file interface do exist, but the included tests really check something different. Maybe the three tests (check for correct path, then for file existence, then for correct file size) could be collapsed into only two, but..
?

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented
Title: system_retrieve_file() was broken -> fix » system_retrieve_file() fails in file_unmanaged_copy() (invocation with path instead of URI)

What a baaad issue title this has X)
Also might be a good idea to post what happens if you try to use system_retrieve_file wihout this fix:
The specified file temporary://file47942o could not be copied, because the destination sites/default/private/temp is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper.

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented

...btw what does drupal_http_request do when it gets fed .f.e an MP3 stream URL?.. mmh gonna try this..

eMPee584 requested that failed test be re-tested.

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented
Priority: Normal » Critical

bumping this to critical - system module shouldn't really ship with code broken by default?

dcor’s picture

dcor CreditAttribution: dcor commented
Status: Needs work » Needs review

Just a use case example - when uploading a new logo file or favicon icon to a theme... you get the following error message...

"The specified file temporary://logo_0.jpg could not be copied, because the destination logo.jpg is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper."

So I'd say eMPee584 is right in bumping this to a critical issue.

I tried the attached patch which patched fine.. but didn't fix the issue any. Here's the error message again after applying the patch (not being a developer I hope that I'm right in assuming this is the right issue to post this in!)

The specified file temporary://logo_1.jpg could not be copied, because the destination logo.jpg is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper.

So am I implying that this is a bigger issue? I haven't a clue.

Related issue - just found the actual issue for my problem but I'll leave this post here for your review just in case it is relevant: #605892: Cannot upload a logo

dcor’s picture

dcor CreditAttribution: dcor commented
Status: Needs review » Needs work
eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented

..so apparently the same problem exists in the system_theme_settings() and there's a patch for it aswell - nicce ;)

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented
FileSize
system-module-repair-system_retrieve_file-with-testcase-v3.patch5.61 KB

rerolled..
am i wrong or is system_retrieve_file() supposed to be the primary function to allow modules the fetching of remote files?

sun.core’s picture

sun.core CreditAttribution: sun.core commented

#12: system-module-repair-system_retrieve_file-with-testcase-v3.patch queued for re-testing.

Dries’s picture

Dries CreditAttribution: Dries commented

This looks good. Couple of smaller things:

+++ modules/system/system.module
@@ -3050,33 +3050,49 @@ function system_image_toolkits() {
+ *   original name or, if given aswell, a different filename.

'aswell' is not a valid word in English.

+++ modules/system/system.module
@@ -3050,33 +3050,49 @@ function system_image_toolkits() {
+function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $replace = FILE_EXISTS_RENAME) {

I find it odd that an API function uses drupal_set_message() but that is probably best left for another issue.

+++ modules/system/system.test
@@ -1619,3 +1619,47 @@ class FloodFunctionalTest extends DrupalWebTestCase {
+      'name' => 'HTTP File Retrieval',

Should read 'HTTP file retrieval'.

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented
FileSize
system-module-repair-system_retrieve_file-with-testcase-v4.patch5.67 KB

ggrr if browser crashes one more #%! time i'm gRRrr---- ..

I find it odd that an API function uses drupal_set_message() but that is probably best left for another issue.

Well at least it's not within a core include (where it actually might belong?)...
Anyways, polished the text a further bit, g2g hopefully..

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Lovely! Thanks, eMPee584 for your patience! :)

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented

Cool cool. Forward and BEYOND! ;D

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

This was not documented in the upgrade guide. Added at http://drupal.org/update/modules/6/7#system_retrieve_file

eMPee584’s picture

eMPee584 CreditAttribution: eMPee584 commented

THX Gabor for filling the void!..

marco71’s picture

marco71 CreditAttribution: marco71 commented

He sun are you a Javaman? lol