Merge lp:~wimleers/pressflow/hook_file_url_alter_bugfix into lp:pressflow

[Wim Leers]

Includes the bugfixes for the bugs reported in https://bugs.launchpad.net/pressflow/+bug/597718. Only the file_create_url() function in includes/file.inc is affected.

Big thanks go to jcmarco, whose insight has uncovered flaws in my initial patch.

Note: the exact same code for file_create_url() is used in the CDN module's (http://drupal.org/project/cdn) core patch, as of version 2.0 beta 6 (http://drupal.org/node/933908). No more problems have been reported.

[David Strauss (davidstrauss)]

This is a little silly:

> drupal_substr($path, 0, 1) == '/' || drupal_substr($path, 0, 2) == '//'

Anything that satisfies the latter condition inherently satisfies the former.

This confuses me:

> if (strpos($path, file_directory_path() . '/') === 0) {
> $path = trim(substr($path, strlen(file_directory_path())), '\\/');
> }

If we know $path begins with file_directory_path() plus a slash, why would I set $path to the first N characters of itself, where N is the length of file_directory_path()? It seems like that would set $path = trim(file_directory_path(), '\\/').

Finally, I don't see why the preg_match() should be case-insensitive, while other code (like the match I'm confused about above) is case-sensitive.

review: Needs Fixing

[David Strauss (davidstrauss)]

Never mind about the file_directory_path() thing I was confused about. I mis-remembered the parameters to substr() to be substr(<string>, <characters-to-take>) where the second parameter is actually where to begin. My other two concerns remain.

[Wim Leers (wimleers)]

> This is a little silly:
>
>> drupal_substr($path, 0, 1) == '/' || drupal_substr($path, 0, 2) == '//'
>
> Anything that satisfies the latter condition inherently satisfies the former.

It is. But it's for clarity: the code now corresponds to the comment:
> // Return path if it was altered by any module, or if it already is a
> // root-relative or a protocol-relative URL.

It's not a bug, it's just to make the code more legible/understandable/maintainable.

> Finally, I don't see why the preg_match() should be case-insensitive, while other > code (like the match I'm confused about above) is case-sensitive.

You're right, this is not necessary. But it can't do anything wrong either. It's a tiny performance hit, I suppose. Fixed: http://bazaar.launchpad.net/~wimleers/pressflow/hook_file_url_alter_bugfix/revision/97.

review: Needs Resubmitting

[Wim Leers (wimleers)]

Is there anything else I can do to move this forward?

[David Strauss (davidstrauss)]

> Is there anything else I can do to move this forward?

I just need to find the time to review it. I think you've done everything you can. Thanks.

[Aaron Moore (mcaden)]

This patch resolved this issue for me: http://drupal.org/node/997364

[David Strauss (davidstrauss)]

Pressflow 6 is now maintained on GitHub, so I'm marking this merge as rejected.

[goofrider (goofrider)]

@mcaden:

I have the same problem with pressflow 6 (some file url return as href="//sites/...." on my staging sites). This patch didn't work for me though. :(

Unmerged revisions

97. By Wim Leers <email address hidden> on 2010-10-22

Fixed: the regular expression to detect shipped files was case-insensitive, while this was not necessary.

96. By Wim Leers <email address hidden> on 2010-10-21

Fixed #597718: file.inc function file_create_url() not correct

=== modified file 'includes/file.inc'
--- includes/file.inc	2010-08-11 21:05:18 +0000
+++ includes/file.inc	2010-10-22 15:57:50 +0000
@@ -48,7 +48,7 @@
  *   A string containing the Drupal path (i.e. path relative to the Drupal
  *   root directory) of the file to generate the URL for.
  * @return
- *   A string containing a URL that can be used to download the file.
+ *   A string containing a URL that can be used to access the file.
  */
 function file_create_url($path) {
   // Clean up Windows paths.
@@ -56,21 +56,14 @@
 
   drupal_alter('file_url', $path);
 
-  // If any module has altered the path, then return the alteration.
-  if ($path != $old_path) {
+  // Return path if it was altered by any module, or if it already is a
+  // root-relative or a protocol-relative URL.
+  if ($path != $old_path || drupal_substr($path, 0, 1) == '/' || drupal_substr($path, 0, 2) == '//') {
     return $path;
   }
 
-  // Otherwise serve the file from Drupal's web server. This point will only
-  // be reached when either no custom_file_url_rewrite() function has been
-  // defined, or when that function returned FALSE, thereby indicating that it
-  // cannot (or doesn't wish to) rewrite the URL. This is typically because
-  // the file doesn't match some conditions to be served from a CDN or static
-  // file server, or because the file has not yet been synced to the CDN or
-  // static file server.
-
   // Shipped files.
-  if (strpos($path, file_directory_path() . '/') !== 0) {
+  if (preg_match("#^(/?)(misc|modules|sites|themes)/#", $path) && (strpos($path, file_directory_path() . '/') !== 0)) {
     return base_path() . $path;
   }
   // Created files.
@@ -83,7 +76,9 @@
         // rewritten to be served relatively to system/files (which is a menu
         // callback that streams the file) instead of relatively to the file
         // directory path.
-        $path = file_directory_strip($path);
+        if (strpos($path, file_directory_path() . '/') === 0) {
+          $path = trim(substr($path, strlen(file_directory_path())), '\\/');
+        }
         return url('system/files/' . $path, array('absolute' => TRUE));
     }
   }
@@ -1046,23 +1041,6 @@
 }
 
 /**
- * Remove a possible leading file directory path from the given path.
- *
- * @param $path
- *   Path to a file that may be in Drupal's files directory.
- * @return
- *   String with Drupal's files directory removed from it.
- */
-function file_directory_strip($path) {
-  // Strip file_directory_path from $path. We only include relative paths in
-  // URLs.
-  if (strpos($path, file_directory_path() . '/') === 0) {
-    $path = trim(substr($path, strlen(file_directory_path())), '\\/');
-  }
-  return $path;
-}
-
-/**
  * Determine the maximum file upload size by querying the PHP settings.
  *
  * @return