Friends ...
likely file_stream_wrapper_uri_normalize() has got a typo at the else statement:

function file_stream_wrapper_uri_normalize($uri) {
  $scheme = file_uri_scheme($uri);

  if ($scheme && file_stream_wrapper_valid_scheme($scheme)) {
    $target = file_uri_target($uri);

    if ($target !== FALSE) {
      $uri = $scheme . '://' . $target;
    }
  }
  else {
    // The default scheme is file://
    $url = 'file://' . $uri;
  }
  return $uri;
}

Likely the correct assignment should be: $uri = 'file://' . $uri;

On the other hand, if such a typo is fixed, seemingly file_scan_directory() has to be modified. Please, see the attached patch.

Maybe, other caller functions could exhibit side-effects too. I have found no more side-effects but there could be ...

Please, additional review of caller functions is welcome ...

Files: 
CommentFileSizeAuthor
#17 file-scheme-1049050-17.patch1.08 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 52,214 pass(es).
[ View ]
#16 file-scheme.1049050.16.patch1.06 KBTwoD
PASSED: [[SimpleTest]]: [MySQL] 40,299 pass(es).
[ View ]
#6 file-scheme.patch1.22 KBmfb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-scheme_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 file-scheme.patch8.62 KBmfb
FAILED: [[SimpleTest]]: [MySQL] 36 pass(es), 85 fail(s), and 92 exception(es).
[ View ]
file_stream_wrapper_uri_normalize_110202.patch1.12 KBjpsoto
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Needs review» Needs work

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

jpsoto’s picture

Priority:Major» Critical

Yes ... above failed test demonstrates Drupal7 installation will fail if the *typo* is fixed.

Locally, I have just tried a fresh installation with the same results.

I am going to promote this issue from major to critical.

Indeed, if the *typo* is not fixed there will be no critical effects but ... ihmo this is a (future) harmful bug waiting for us under cover.

What about this *typo*?

webchick’s picture

Priority:Critical» Major
Issue tags:+Needs tests

Unless Drupal is rendered unusable this isn't a critical bug.

And we obviously need tests for this. What a stupid bug. :)

mfb’s picture

Status:Needs work» Needs review
StatusFileSize
new8.62 KB
FAILED: [[SimpleTest]]: [MySQL] 36 pass(es), 85 fail(s), and 92 exception(es).
[ View ]

Wow this is really broken. It looks like someone wanted to start using file:// syntax for paths, but this never actually happened. A major overhaul would be required re: how files are included (each file is an absolute file:// URI and so no longer needs DRUPAL_ROOT prepended), and how paths are used (e.g. a path to a theme is now an absolute file:// URI, so can no longer be used as-is in URLs).

As an alternative,

else {
  // The default scheme is file://
  $url = 'file://' . $uri;
}

could just be removed from file_stream_wrapper_uri_normalize() since these lines don't do anything. This function is called thousands of times in certain places, such as the admin/config page, so should be optimized as much as possible.

The last submitted patch, file-scheme.patch, failed testing.

mfb’s picture

StatusFileSize
new1.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-scheme_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Still more changes required for this to work. Also, if paths are stored in the database (registry, system table, etc.) using file:// URIs, then if the site is moved around on the server, there are fatal errors due to failed opening required files. So there would have to be some means to regenerate all the broken file URIs. The alternative is to give up and remove this logic to help optimize file_stream_wrapper_uri_normalize(), which gets called thousands of times when scanning the module and theme directories.

Damien Tournoud’s picture

Status:Needs review» Needs work

I agree that this code should just be removed. It does more harm then good to add an explicit file:// (it's the default) because some part of PHP (and presumably some parts of Drupal too) do not understand the stream-wrapper schemes.

- *   URI may be a bare filepath (without a scheme) and in that case the default
- *   scheme (file://) will be used. If this value is omitted, Drupal's default
- *   files scheme will be used, usually "public://".
+ *   URI may be a bare filepath (without a scheme). If this value is omitted,
+ *   Drupal's default files scheme will be used, usually "public://".

This looks bogus to me. Without a scheme, direct file access is the default (ie. file://). This is true with or without the hunk in file_stream_wrapper_uri_normalize().

Damien Tournoud’s picture

Title:file_stream_wrapper_uri_normalize(): Likely an harmful typo» file_stream_wrapper_uri_normalize(): remove dead code
Priority:Major» Normal
Issue tags:-Needs tests

I'm removing the test tag. As shown here, this part of the code is well tested.

mfb’s picture

My thought was that even mentioning file:// scheme could cause confusion unless it's better supported. Some (many?) functions in the File API do not support the file:// scheme. For example, $source cannot use file:// scheme because this scheme is not supported by drupal_realpath().

That said, a $destination using file:// scheme does work, assuming you give it an absolute path, e.g. file:///path/to/drupal/sites/default/files/foo.txt or file://localhost/path/to/drupal/sites/default/files/foo.txt. The $destination is not returned with file:// scheme unless you give it a file:// URI to begin with.

jpsoto’s picture

I agree, this code should be removed. If it is *fixed* many side-effect bugs will arise.

Please, note my patch included to remove the call of file_stream_wrapper_uri_normalize() from file_scan_directory(). I discovered this *typo* while reviewing the function file_scan_directory(). It will be also dead code because file_scan_directory() manages only files.

mfb’s picture

@jpsoto file_scan_directory() is designed to scan a directory provided by any stream wrapper, not just local files. I actually don't know why stream wrapper URIs need to be normalized (the docs mention "trim erroneous leading slashes," how would those get there in the first place, and how does it know they need to be removed?), so that makes me hesitant to remove this line.

TwoD’s picture

IRuslan’s picture

Seems error is not fixed yet.
Why it's impossible just to remove that lines, because it have no any sense?

pwolanin’s picture

Status:Needs work» Needs review

#6: file-scheme.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, file-scheme.patch, failed testing.

TwoD’s picture

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 40,299 pass(es).
[ View ]

Re-rolled for 7.x.

David_Rothstein’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7
StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 52,214 pass(es).
[ View ]

Seems like a no-brainer, but needs to go in Drupal 8 first, right?

TwoD’s picture

Oh, of course. Forgot to check if that code was still around in D8...

Damien Tournoud’s picture

Status:Needs review» Reviewed & tested by the community

RTBC for Drupal 7 and 8. Just as a clarification, this is about removing dead code, and updating the comments to match the actual behavior of the code.

Supporting file:// throughout core is not on the table.

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

dcam’s picture

Status:Patch (to be ported)» Needs review

#16: file-scheme.1049050.16.patch queued for re-testing.

dcam’s picture

Status:Needs review» Reviewed & tested by the community

Already checked as RTBC for D7 in #19.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

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