CDN integration: allow file URLs to be rewritten by hook_file_url_alter()

@Moshe, Dries, m3avrck & chx: thanks!

@Dries:

- I think the custom_file_url_rewrite($path) should be mentioned in the PHPdoc, not just in the inline comments. The inline documentation is really great but it is not as accessible as PHPdoc. This is pretty minor.

Done.

- It would be nice, but not required, to have an example custom_file_url_rewrite($path). It's pretty straight-forward so this isn't a must have.

Done. Added to system.api.php. The example function is the 95% use case.

- Can't we do + $is_in_files_directory = (strpos($path, file_directory_path() .'/') === 0); after the CDN check? $is_in_files_directory is only used once.

Done.

- This is worth a CHANGELOG.txt entry.

Wow :)

- Could benefit from a test but it is not clear how easy that would be to write.

If it's just testing file_create_url(), it'd be fairly easy. But I'm not sure how I'm supposed to add a custom function only for one specific test. If somebody could point me to the right documentation, I'll write it.

@m3avrck: Adding support for such a variable makes the possibilities too limited. Or the end result becomes too messy. With this simple function, it's still possible to support your simple use case. See the updated patch for your simple use case.

@geerlingguy: your use case is very easy to support: just change a couple of lines

@chx:

This issue is very, very cool. However we will have two rewrites, one for files , one for paths. I am wondering whether we want to integrate. Please feel free to yell "dont bloat this issue" at me I am aware that I might want to do too much here but this really nags me.

In my opinion, it's as simple as this: don't make it harder than it can be. Some people will want to rewrite file URLs but not Drupal paths. Other people will want to do the opposite, and even other people will want to do both. Also, file URLs may be served from other domains after rewriting and that will never be the case for Drupal paths.
It's simply conceptually easier to separate them. At least for me, that would be the case. I don't see how I could clearly write file URL rewriting logic if it were part of the same function as Drupal path rewriting.

@all:
Patch rerolled, it now passes. Instead of checking if the passed in path in file_create_url() begins with file_directory_path(), I'm now checking if it begins with one of misc, modules, profiles, sites or themes. This is ugly, but only mildly more so than the previous check. I'm open to suggestions of course.

One more thing. Unfortunately, thanks to m3avrck's comment, I stumbled upon one tricky, missing feature. The rewriting of URLs in aggregated CSS files. m3avrck rewrote the URLs in CSS (url(path/to/file)) to be served from the CDN.
From one point of view, this is plain wrong: the CSS files on the Drupal web server should point to files on the Drupal web server. Otherwise, you can no longer fall back easily anymore.
From another point of view, this is right: it allows you to easily serve images referenced from CSS files from the CDN/static file server.
I've always assumed that the files on the Drupal web server should be the fallback, i.e. that they are the "original" files and that any changes to serve files from a CDN/static file server are made dynamically (in the file URL) or through file processing. That is why my synchronization daemon also processes CSS files to update the URLs before syncing.
Maybe we could make it pass in context, so that the author of the custom_file_url_rewrite() can decide what is appropriate for his site setup.

@ugerhard: heh, oops, I had to submit my comment 10 times before d.o would accept it (probably my university network was to blame), and I guess I forgot to set it again. Thanks!

@moshe: My apologies for my bad explanation. You're absolutely right. In 99% of the cases, what I was talking about is unnecessary (and so is that part of m3avrck's patch). It is only necessary when the base path of your Drupal site and of your CDN are *different*.
E.g.:
1) http://drupal.org/files/css/aggregated.css references http://drupal.org/misc/bar.png through the relative url /misc/bar.png.
2a) http://drupal.org/files/css/aggregated.css is synced to http://cdn.drupal.org/files/css/aggregated.css. The relative URL /misc/bar.png is still valid, but it now points to http://cdn.drupal.org/misc/bar.png
2b) http://drupal.org/files/css/aggregated.css is synced to http://cdn.drupal.org/different/base/path/files/css/aggregated.css. The relative URL /misc/bar.png is now invalid, because it points to http://cdn.drupal.org/misc/bar.png instead of http://cdn.drupal.org/different/base/path/misc/bar.png.

@Gerhard: changing back to needs review so the test bot can do its thing :)

Tests have been written.

There is one gotcha: FileURLRewritingTest subclasses FileDownloadTest. This results in 2 things:

1. in the setUp() method, we can no longer call parent::setUp(); instead we must call DrupalWebTestCase::setUp()
2. the testPrivateFileTransfer() method is inherited and this is desired, because it ensures private file transfers are still working when the custom_file_url_rewrite() function is present, which doesn't rewrite URLs when private downloads are enabled (this is the 95% use case)

Now, another issue is the cruft present in Drupal core's current (and past) file_create_url():

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())), '\\/');
  }
  // ...
}

That piece of code implies that you could pass in sites/default/files/foo.png just as well as foo.png and get the same result. This conversion really should only happen for private downloads. Hence the patch changes it to that behavior.
Passing in sites/default/files/foo.png (and not just foo.png) is actually also what Drupal core does in the Upload and Blog API modules. The Upload module for example, stores paths like sites/default/files/foo.png in the database and routes that through file_create_url().
The only places where Drupal core deviates from this is ironically enough in the File tests themselves. I consider this a bug, and I hope you do too. A function sets requirements on the data its parameters contains. I suggest that the $path parameter file_create_url() accepts should be "a string containing the Drupal path (i.e. path relative to the Drupal root directory) of the file to generate the URL for". That way, the path custom_file_url_rewrite() receives is always one that exists. (Note: custom_file_url_rewrite() assumed this already, I had forgotten about this piece of cruft since none of the Drupal core modules actually uses the "path relative to the files directory" alternative.)

Sorry for yet another long post. I hope it gets the message across.

The test bot is broken. The patch applies just fine.

@moshe: I'd prefer not. For the reasons cwgordon7 mentioned and because I think it'd lead to more confusion than the clarity/consistency that can be achieved by turning "special functions" into hooks.

So I'd prefer to see the concept of this patch being approved and therefor committed, and *then* discuss the implications of having potentially multiple implementations of hooks that are not suited very well for multiple implementations.

aaron, I think the stream wrappers patch is *very* far from being RTBC? It's not even entirely sure that it will make it into core. It will surely need benchmarking as well because it is so pervasive.

Also, the changes I made that clash with that patch are all in a single function: file_create_url(). Most changes are about ceasing to use hardcoded paths to files, but to generate proper file URLs instead. That remains a necessity.

So I would very much prefer to see my patch go in first.

Worst case scenario: file_create_url() change again. It remains necessary to keep custom_file_url_rewrite() though. When your patch would get committed, Drupal *could* use stream wrappers, but that doesn't mean it *should* (or could in all situations) use that to sync files to a CDN. For Origin Pull CDNs (CDNs that automatically fetch files from the origin server, the most commonly used CDN type), it also remains necessary to be able to alter the file URL. For Origin Pull CDN file URLs, it's also imposible to stat() them. I'm not sure the stream wrappers patch takes that into account.
In the end, we're talking about a single small function in your patch that would be affected. It doesn't seem sensible to me to postpone a simple, small, almost RTBC patch containing things that are necessary anyway, for a fairly complex, large, not-even-sure-to-make-it-to-RTBC patch.

@Dries & moshe: Thanks for the reviews! A new patch will be rolled in the next few days. Can't say when yet.

@aaron: in not a single way does this patch require CDN support. I have no clue why you are suggesting that. You can combine static file servers with CDNs. You can do anything you want. In fact, my test case, http://driverpacks.net, mixes a static file server with a CDN and assigns it based on the visitor's properties (his country actually, based on his IP). As far as I can tell, no more flexibility than that can be required.

The only thing this patch does, is enable the ability to rewrite file URLs. That's it.

I fully agree that all CDN-handling stuff should live in contrib. That's what this patch makes possible in the first place.

Obviously, this patch *does* support multiple CDNs. The fact that you're saying it does not supports this, suggests that you did not even take a look at the patch. The patch even includes a sample custom_file_url_rewrite() implementation in system.api.php in which exactly that is being demonstrated!

I fully agree that things do get tricky when using hooks, that's what I'll have to tackle next.

However, stream wrappers cannot make that more easy. Stream wrappers is just a way to access remote file systems. The difficulty Drupal (or the site administrator) has to deal with, is *not* how to get files on a remote file system (which can be done through a syncing daemon, through your stream wrapper system …), but the ability to decide from which server each file should be served (or CDN, but a CDN can be considered just another server).

@jmstacey: You're trying to move a boatload of new code into core, whereas I'm only making file URLs alterable. That's useful in itself, regardless of using a CDN or not. Some people like to run a light-weight Apache instance or lighttpd for serving static files, and this makes that easy to do, too.

In my opinion, it doesn't *have* to be up to Drupal to move files to a CDN. Origin Pull CDNs also don't require Drupal core to do anything. In my case, I'm using a daemon (open source, GPL, will get a homepage soon) to synchronize files. This allows for much more flexibility and speed than letting Drupal sync the files ever could. Because Drupal cannot perform simultaneous uploads, because PHP doesn't allow for that. My daemon also allows files to be preprocessed, e.g. to compress the CSS and JS using YUI Compressor (which gives a better compression ratio than Drupal's compression algorithm), to optimize images (PNG files can easily become 10% smaller), video transcoding, and so on. That sort of preprocessing is unimaginable to do from within PHP, as it'd take ages, because nothing can happen in parallel. In fact, I talked to the Media Mover module author at DrupalCon DC and he was very interested because it is PHP that limits the transcoding speed (due to lack of parallelism).
In the case of the daemon, a input_file -> CDN URL mapping is stored in an SQLite DB. All files' CDN URLs are looked up — it takes about 0.2-0.4 ms to look up a single URL. You have limitless flexibility, as you can even decide to sync the same files to multiple servers (e.g. in the case you have a static file server near the location of an event your organization is hosting (e.g. DrupalCon Paris), to minimize latency for the visitors there) and pick a server dynamically based on the user's characteristics.

Drupal is awesome and should eat its own dog food, but there are limits. It of course depends on your requirements. But after all, if you think about the big picture, a stand-alone, multi-threaded daemon to synchronize files makes more sense than a PHP CMS syncing files, doesn't it? :)

Please don't get me wrong, I'm sure many site owners will love stream wrappers. I'll even use it myself for my smaller web sites. But it's not always the appropriate solution.

So yes, I also think we're on the same page, just with slightly different points of view. Now that we agree that our patches are in fact complementary (I should have used that word in my first reaction), lets' stop derailing this issue :) Ping me on IRC if you've got more questions/thoughts :)

Also, thanks for the excellent idea on never giving uid 1 files from the CDN. I'll definitely add that in my sample code. :)

Rerolled, with comment fixes and a couple of minor code style fixes. Instead of hook_url_alter(), I've named it hook_file_url_alter(), because it is really only meant for file URLs.

Note that in the approach aaron has taken — and with which I agree — it is impossible for two modules to alter the same file URL. As soon as one module alters the file URL, it is assumed to be the final one. This simple rule prevents all sorts of weird problems.

I'll reroll this patch tomorrow. I should've rerolled it right away after I posted my latest version.

@neclimdul: why then does imagecache pass absolute URLs? The doxygen said:

- * @param $path A string containing the path of the file to generate URL for.

and now says:

+ * @param $path
+ *   A string containing the Drupal path (i.e. path relative to the Drupal
+ *   root directory) of the file to generate the URL for.

Clearly, it was never intended to accept absolute URLs. So imagecache is at fault, IMO.

@drewish: that has been mentioned already, but this is complementary, really

@Jody Lynn: no, I don't want to start working on that massive patch and it's outside of my scope

Turns out it wasn't imagecache to blame, but my change in theme_image() was incorrect. It didn't accept absolute URLs anymore.

Patch rerolled.

I'd prefer to keep them separate, as I already outlined in #11:

In my opinion, it's as simple as this: don't make it harder than it can be. Some people will want to rewrite file URLs but not Drupal paths. Other people will want to do the opposite, and even other people will want to do both. Also, file URLs may be served from other domains after rewriting and that will never be the case for Drupal paths.
It's simply conceptually easier to separate them. At least for me, that would be the case. I don't see how I could clearly write file URL rewriting logic if it were part of the same function as Drupal path rewriting.

#227232: Support PHP stream wrappers went in, hence I have to reroll this patch. The previous patch worked just fine, but didn't apply anymore due to stream wrappers going in.

This patch previously replaced all base_path() calls with file_create_url() calls. That is no longer necessary, because the file stream wrappers patch already does this now.

A change over the previous version is that instead of getting a $path parameter, you now get a $uri parameter in hook_file_url_alter(). You can then also decide based on the scheme if you want to rewrite the URL. This is necessary for e.g. uploaded files, which use the public:// scheme.

Thanks to Mollom for letting me re-roll this patch on company time :)

This title is now more appropriate, since file_create_url() is now already being used for all file URLs, thanks to #227232: Support PHP stream wrappers.

And I forgot to port the changes to system.api.php. Fixed that in this new iteration.

I just removed that check (after talking with neclimdul). That automatically allows you to alter the scheme, i.e. to switch to a different stream wrapper.

Forgot to remove a little piece of now obsolete documentation.

It *needs* to be in its own module, otherwise the hook_file_url_alter() implementation will cause other tests to fail. aaron also didn't see this immediately, but he realized it in #40.

I'm distinguishing between "created" and "shipped" files now because it's otherwise not very clear why no scheme would be set. And previously, base_path() plus some string concatenations would be used to create the URL to created files and hence there was no need for such vocabulary. Now that everything is being routed through file_create_url(), it makes sense to add that explanation IMO.

Changed the first line of file_create_url()'s doxygen.

Ah, well, a variable_set()/variable_get() to control the alter is possible, yet very ugly/unclear. However, I understand that the number of test modules should be kept to a minimum, so from that perspective your suggestion does make sense.
Changed that in this new iteration: removed the file_url_test.module and moved the hook_file_url_alter() implementation into file_test.module.

And I indeed misinterpreted what you said about "shipped" vs "created". Glad that's sorted out :)

Yay! :)

I found a small bug in the system.api.php documentation that I forgot to remove. See the attached patch.

Updated the D6->D7 upgrade guide as well.

Something like this?

Wrong status.