Files referenced in CSS aggregates should not have CDN URLs: already root-relative, and they cannot be safely made forever cacheable anyway

For now, assuming option 1: Leave it as it is, since it does not and cannot cause problems., since it's the least amount of work, and because it brings a final release within closer reach.

In the mean time, #2745115: Port CDNCssUrlTestCase in tests/cdn.test removed the now unnecessary test coverage.

Adding one extra bit of information to the IS from #2745115-2: Port CDNCssUrlTestCase in tests/cdn.test, that makes this issue much easier to understand.

I first decorated CssCollectionGrouper's group() method. Unfortunately, that doesn't work, because the hash (and CSS aggregate filename) is generated based solely on CSS filenames, not on any other metadata.

So, what I actually need to override/decorate, is CssCollectionOptimizer::generateHash. Which is unfortunately protected, unlike CssCollectionGrouper::group().

The therefore less elegant solution albeit working one is the one attached.

Unfortunately, this will likely cease to work correctly as soon as #1014086: Stampedes and cold cache performance issues with css/js aggregation lands, or as soon as any other module is overriding the CSS collection optimizer.

So, likely a better solution is to override the asset dumper. Although that too could be impacted by #1014086: Stampedes and cold cache performance issues with css/js aggregation, but at least it'll work fine in collaboration with other contrib modules, like advagg. Except… advagg itself already overrides all of core's asset handling services. So the CDN module would have to implement a hook for AdvAgg too.

That'd look somewhat like the attached patch (not letting that one be tested, because it's only a PoC, it's incomplete). The problem there is that we duplicate 99% of the logic in AssetDumper… just to be able to modify the generated filename. This is terrible for maintainability: I'd have to monitor core's AssetDumper for changes, and make sure I copy those over…

So, in conclusion, no matter what I do, I'm in a bad place. Because the CDN module is unable to just modify the generated filenames. It is able to do the necessary cache invalidation (invalidating the library_info) cache tag. But not modify the generated filename.

So, what we need, is a simpler way to modify the generated filename for CSS aggregates.

Fortunately… it's looking like #1014086: Stampedes and cold cache performance issues with css/js aggregation will add exactly that: if that ends up adding AssetDumperUriInterface like it does today:

-class AssetDumper implements AssetDumperInterface {
+class AssetDumper implements AssetDumperUriInterface {
 
   /**
    * {@inheritdoc}
@@ -17,12 +17,19 @@ class AssetDumper implements AssetDumperInterface {
    * browsers to download new CSS when the CSS changes.
    */
   public function dump($data, $file_extension) {
+    $path = 'public://' . $file_extension;
     // Prefix filename to prevent blocking by firewalls which reject files
     // starting with "ad*".
     $filename = $file_extension . '_' . Crypt::hashBase64($data) . '.' . $file_extension;
-    // Create the css/ or js/ path within the files folder.
-    $path = 'public://' . $file_extension;
     $uri = $path . '/' . $filename;
+    return $this->dumpToUri($data, $file_extension, $uri);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function dumpToUri($data, $file_extension, $uri) {
+    $path = 'public://' . $file_extension;
     // Create the CSS or JS file.
     file_prepare_directory($path, FILE_CREATE_DIRECTORY);

… then I can just decorate that service and decorate only the dump() method. It'd then still need to duplicate some code, but far less of it:

namespace Drupal\cdn\Asset;

class AssetDumper implements AssetDumperInterface {
  public function dump($data, $file_extension) {
    $path = 'public://' . $file_extension;
    // Prefix filename to prevent blocking by firewalls which reject files
    // starting with "ad*". Also include the CDN settings hash to ensure a
    // different aggregate is served when CDN settings change.
    $filename = $file_extension . '_' . Crypt::hashBase64($data) . '.' . $this->settings->getHash() . '.' . $file_extension;
    $uri = $path . '/' . $filename;
    return $this->dumpToUri($data, $file_extension, $uri);
  }
}

That'd be far more maintainable: chances of that ever changing are very very low.

So, postponing this on #1014086: Stampedes and cold cache performance issues with css/js aggregation.

Clarifying title & issue summary.

Which made me realize that there are still two possible solutions:

3. Allow file_create_url() to receive a context. This would allow the CDN module to not alter the file URL if it's referenced from a CSS file. This requires a core patch. Note that this also works when the "Forever cacheable files" setting is enabled: files referenced in the CSS file then use root-relative URLs.
   For example, even if //cdn-example.com/cdn/farfuture/tG2ezMbStSbnCryLDs2r7b10f-VKv7I8S3hK2lPnmEM/1474536635/sites/default/files/css/css_UcklJvyLvvpIDbvEa9ARtoihbJeEE0_YBOaolKktIAc.css?odwh4j has a signed URL, all referenced URLs look like /core/themes/bartik/images/tabs-border.png, which means they are resolved to //cdn-example.com/core/themes/bartik/images/tabs-border.png, which works fine. It'd only break (like in D7) if those URLs were using relative URLs like image.png or ../image.png, which is no longer the case in Drupal 8.
   One could argue that those referenced URLs then are not "Forever cacheable files", but note that we wouldn't be aware of modifications to those referenced files anyway, because the aggregates are cached. Which means that we'd be serving e.g. the same background image forever even if it changed in the mean time. Therefore it is actually more accurate to not have those files be served from a CDN (which also means the Drupal 7 version of the CDN module gets this wrong). Therefore this is probably the better option.
4. Modify the CSS aggregate filename to include a hash of the CDN settings, this would ensure that a different aggregate is served depending on CDN settings. This requires a core patch, and it turns out #1014086: Stampedes and cold cache performance issues with css/js aggregation is already introducing this in an indirect way.

(Copied verbatim from the updated issue summary.)

In fact, thinking about it more, we do not need #1014086: Stampedes and cold cache performance issues with css/js aggregation, we only need #3!

We need to not modify URLs inside CSS aggregates because otherwise we'll end up serving those referenced files with Far Future expiration headers, which is wrong.
Then there also is no more need to generate a new aggregate whenever CDN settings change… because there's no point in changing them! We want them to always be the same:

1. when the CDN module is not installed
2. when the CDN module is installed, but not enabled
3. when the CDN module is installed, and enabled
4. when the CDN module is installed, and enabled, with "Forever cacheable files" enabled
5. when the CDN module is installed, and enabled, with "Forever cacheable files" disabled

Patch that implements #8's conclusion.

I talked it through with catch, who's working on #1014086: Stampedes and cold cache performance issues with css/js aggregation. He's +1 on this approach. His main remark is that the code here could be simplified: rather than decorating that service which sets a global variable, we could rely on a route match. The downside of that is that it will probably not work for contrib modules. So the approach in this patch is probably the most reliable one.

The only downside to this approach is that not all assets will have Far Future expiration headers. But correctness should come first, and this is definitely better for correctness.

As a bonus, this is better for simplicity/maintainability/performance: we won't have to regenerate CSS aggregates anymore upon enabling the CDN module, nor when changing settings. And even better: having CSS aggregation enabled will no longer be a requirement of using the Forever cacheable files setting!

Updating title accordingly.

This will still need a test: \Drupal\Tests\cdn\Functional\CdnIntegrationTest::testCss(), which tests:

• CSS without aggregation: image URL inside CSS is relative
• CSS with aggregation: image URL inside CSS is root-relative, regardless of the CDN settings (status=on/off, farfuture status=on/off)

In the mean time, this last reroll removes additional things that can be removed thanks to the changes in #10.

+++ b/src/Asset/CssOptimizer.php
@@ -0,0 +1,64 @@
+      return $this->decoratedCssOptimizer->optimize($contents);

s/optimize/clean/

And here's the missing test coverage.

Now it's getting on my nerves. This is adding debug output.

The output for #23 is:

//cdn.example.com/cdn/farfuture/kDGu2YHuE-rsJKbBgfpetFUIGbzOCGrRbx3sGJ2g_eA/1474882364/checkout/sites/simpletest/63982914/files/css/css_cgo6f0PLK42v3DyvcLbdBq9LNwC1Fs8urLurn19LQPc.css?oe3uic

That's wrong. It looks like this slipped through the cracks of the test coverage of #2708787: Port Far Future expiration to 8.x-3.x. It should be:

//cdn.example.com/checkout/cdn/farfuture/kDGu2YHuE-rsJKbBgfpetFUIGbzOCGrRbx3sGJ2g_eA/1474882364/sites/simpletest/63982914/files/css/css_cgo6f0PLK42v3DyvcLbdBq9LNwC1Fs8urLurn19LQPc.css?oe3uic

i.e. the "base path" is in the wrong location. Because the code introduced by #2708787: Port Far Future expiration to 8.x-3.x is overly simplistic: it just prefixes the root-relative URL rather than splitting it up as necessary:

return '//' . $cdn_domain . '/cdn/farfuture/' . $calculated_token . '/' . $mtime . $root_relative_url;

should be something like

return '//' . $cdn_domain . BASE_DIRECTORY . '/cdn/farfuture/' . $calculated_token . '/' . $mtime . $root_relative_url_WITHOUT_BASE_DIRECTORY;

Opened #2806241: Far Future expiration functionality doesn't work when Drupal is not installed in the root of a domain for that and fixed it there.

Reuploading #20.

There we are! :)