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 ...

CommentFileSizeAuthor
#17 file-scheme-1049050-17.patch1.08 KBDavid_Rothstein
#16 file-scheme.1049050.16.patch1.06 KBTwoD
#6 file-scheme.patch1.22 KBmfb
#4 file-scheme.patch8.62 KBmfb
file_stream_wrapper_uri_normalize_110202.patch1.12 KBjpsoto
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

jpsoto’s picture

jpsoto CreditAttribution: jpsoto commented
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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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

mfb
they or he
CreditAttribution: mfb commented
Status: Needs work » Needs review
FileSize
file-scheme.patch8.62 KB

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

mfb
they or he
CreditAttribution: mfb commented
FileSize
file-scheme.patch1.22 KB

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

Damien Tournoud CreditAttribution: Damien Tournoud commented
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

Damien Tournoud CreditAttribution: Damien Tournoud commented
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

mfb
they or he
CreditAttribution: mfb commented

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

jpsoto CreditAttribution: jpsoto commented

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

mfb
they or he
CreditAttribution: mfb commented

@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

TwoD
Primary language Swedish
Location Sweden
CreditAttribution: TwoD commented

Marked #1728128: Wrong variable in file_stream_wrapper_uri_normalize as as duplicate.

IRuslan’s picture

IRuslan CreditAttribution: IRuslan commented

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

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
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

TwoD
Primary language Swedish
Location Sweden
CreditAttribution: TwoD commented
Status: Needs work » Needs review
FileSize
file-scheme.1049050.16.patch1.06 KB

Re-rolled for 7.x.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
FileSize
file-scheme-1049050-17.patch1.08 KB

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

TwoD’s picture

TwoD
Primary language Swedish
Location Sweden
CreditAttribution: TwoD commented

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

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
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

catch
he/him
Primary language English
CreditAttribution: catch commented
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

dcam CreditAttribution: dcam commented
Status: Patch (to be ported) » Needs review

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

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs review » Reviewed & tested by the community

Already checked as RTBC for D7 in #19.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/a9c27da

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

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

Issue tags: +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.