Problems with shipped files and Drupal 6 file_create_url() behavior

IMCE module has the same problem.

Pressflow bug reported: https://bugs.launchpad.net/pressflow/+bug/597718

Also Javascript Aggregator has the same problem with Pressflow.

It looks for relative paths, but CDN module patches have made that Pressflow builds absolute paths.

https://bugs.launchpad.net/pressflow/+bug/605867

subscribing

I also ran into the IMCE issue. Fortunately our site is not in production yet, I'll look for a fix.

Just for related info, I created a patch for taxonomy_image.module that allows it to work with both Pressflow's version of file_create_url() and stock D6. However, that doesn't lessen the fact that this patch introduces a departure from the stock API.

Patch at #877982: Fix pressflow incompatibility and standardize image object URL creation..

(This comment is cross-posted to http://drupal.org/node/3358288 and https://bugs.launchpad.net/pressflow/+bug/597718.)

Brian's analysis at https://bugs.launchpad.net/pressflow/+bug/597718/comments/8 is correct.

file_create_url() in Drupal 6 is broken. It was only used for creating URLs for files in the files directory (i.e. the files_directory_path() directory). But even that it did not do well, since it allows for 2 kinds of calls:
1) with just test.jpg
2) with files/test.jpg
Both would result in http://example.com/files/test.jpg being returned.

You can easily see this when looking at the code, it first strips the files directory and then adds it again:

/**
 * Create the download path to a file.
 *
 * @param $path A string containing the path of the file to generate URL for.
 * @return A string containing a URL that can be used to download the file.
 */
function file_create_url($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())), '\\/');
  }
  switch (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC)) {
    case FILE_DOWNLOADS_PUBLIC:
      return $GLOBALS['base_url'] .'/'. file_directory_path() .'/'. str_replace('\\', '/', $path);
    case FILE_DOWNLOADS_PRIVATE:
      return url('system/files/'. $path, array('absolute' => TRUE));
  }
}

My patch changes that: it makes it so that file_create_url() can be used for *any* file URL. That's necessary to create a single point where all file URLs can be altered, those in the files directory (i.e. "created files") and those that ship with Drupal and modules/themes (i.e. "shipped files"). I.e. to make it possible to alter file URLs so that we can use a CDN.
But, it did not take properly into account the fact that many modules assume that the files directory is automatically added into the URL.

So, to avoid patching Drupal modules to make them use the improved file_create_url() (and thus require them to include file_directory_path() in the file paths they're passing to it), I've revised the way that shipped files are being detected: it now uses regular expressions instead of a simple string comparison. I didn't do any performance testing, but we should be fine.

Basically it involves changing this line:
if (strpos($path, file_directory_path() . '/') !== 0) {
to:
if (preg_match("#(/?)(misc|modules|sites|themes)/(.*)#i", $path)) {

Committed (but first committed the wrong patch, hence 2 commits)!

http://drupal.org/cvs?commit=410718
http://drupal.org/cvs?commit=410722

Thanks jcmarco, committed: http://drupal.org/cvs?commit=416710

You're right. That was in fact a copy/paste error. My apologies. Committed your entire suggested change, as posted in #7: http://drupal.org/cvs?commit=432432

the current version still breaks for

file_get_path(''); 

which is used in the IMCE file browser.

Without the core patch, it returns

$GLOBALS['base_url'] . '/' . file_directory_path() . '/' . str_replace('\\', '/', $path);

which is "http://mysite/sites/default/files/".

After the core path, in file_create_url(), the empty path after alter will become "http://my-cdn-site/", and not equal to the old path, so it will run this part:

  // 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;
  }

and return "http://my-cdn-site/".

That breaks IMCE's file browsing functionality.

which is "http://mysite/" .

I'm providing a patch (against 6.x-2.1). It detects if the path is emty ('' or '/'), if so, the module must just want to get the file directory path, so I return it.

Might not be the best way, but fixes the conflicts with the IMCE module. Hope this could be committed.

actually the latest version of IMCE doesn't not user file_create_url('') to get the file directory any more, it calls file_directory_path() directly now. So this issue might not be so important.

However, if there's any other modules still use file_create_url('') it could be a problem.

I highly doubt any other module used this hack. If so, they should simply also switch to calling file_directory_path() directly.