"Forever cacheable files" enabled + "CSS aggregation" disabled via settings.local.php = files referenced from CSS files 403'ing => don't alter CSS file URLs in this case

#2745109: Files referenced in CSS aggregates should not have CDN URLs: already root-relative, and they cannot be safely made forever cacheable anyway addressed the handling of files referenced by CSS files. It has test coverage specifically for the case when Far Future expiration is enabled. And hence I don't see how this is possible.

So it would seem you're on a dev version older than beta 1. Please upgrade.

If the problem then still persists, please provide steps to reproduce.

Oh wait, that screenshot proves you don't have CSS aggregation enabled. That means the CDN module is unable to rewrite file URLs in CSS files. Hence there's no way it can possibly work.

This leaves me with a few options:

1. put the burden on the site builder to know that the CDN module should only have Forever cacheable files enabled while CSS aggregation is enabled (the current behavior)
2. show an error message (on every request) whenever Forever cacheable files is enabled and CSS aggregation is disabled
3. automatically disable Forever cacheable files while CSS aggregation is disabled, and show a warning message informing the developer (on every request)

None of these is ideal.

What do you think?

So you prefer option 2, because option 3 seems overkill and option 1 is what got you to report this issue. That's fair.

I'm not yet entirely convinced myself that this all makes sense. Does this feel like sensible behavior to you?

I was referring to the solution: it's less than ideal, it's pretty "WTF"-inducing.

Related: #2827998: Add a new default option to the CDN UI: "all files except CSS+JS", and make this the new default of the CDN module, include upgrade path will change the default, so that CSS and JS files are no longer being served from a CDN anymore. That would reduce the chances of running into this problem.

Thanks for sharing your thinking about this! It's always better to think things through with multiple people.

I'm in favor of adding this. We must explain WTFs, even if they are rare to run into.

1. +++ b/src/CdnSettings.php
   @@ -50,6 +70,17 @@ class CdnSettings {
      /**
   +   * Sanity check for the far-future setting relative to CSS aggregation.
   +   */
   +  public function checkFarfutureSettings() {
   +    if ($this->farfutureIsEnabled()
   +      && !$this->configFactory->get('system.performance')->get('css.preprocess')
   +      && $this->currentUser->hasPermission('administer CDN configuration')) {
   +      drupal_set_message($this->t('CDN far-future expiration requires the use of CSS aggregation.'), 'warning');
   +    }
   +  }
   

   This logic needs to move to a helper method on HtmlResponseSubscriber. The CdnSettings class must not generate messages, it's only purpose is parsing/interpreting the cdn.settings config.

2. That logic should also check if the CDN module is enabled: $this->cdnSettings->isEnabled().
3. That logic should also check that CSS files are in fact being served by the CDN module. Otherwise we're showing a warning message for no reason. Sample code:
   

   $lookup_table = $this->cdnSettings->getLookupTable();
   if (!isset($lookup_table['css']) && !isset($lookup_table['*']) {
     // CSS files will never be altered by the CDN module.
   }
   

This is basically what I was asking for. Still needs cleaning up, but this is the structure we want.

While working on this, this made me realize that this is significantly increasing the overhead of the CDN module. It's more than twice as expensive as addDnsPrefetchLinkHeaders().

This is what that looks like BTW.

Here's a completely different approach: it detects if settings.php (or settings.local.php) disable CSS aggregation, and in that case, it will just not alter any CSS file URLs.

No need for warnings; everything continues to work just fine.

Thoughts?

Forgot the patch.

So this would not address the case of developers intentionally disabling CSS aggregation for a site. It's a bad practice to disable it anyway. And in that case, they should arguably be smart enough: if they have reasons to disable CSS aggregation, they should also be smart enough to update the CDN module's settings.

Finally, every developer should know to use settings.local.php, they should not manually disable it in the UI.

I think the patch in #15 strikes a nice balance between pragmatism and strictness. It just silently ignores CSS file URLs when CSS aggregation is off. It doesn't ever get in your way. It doesn't require you to keep two pieces of configuration in sync. It doesn't require us to start altering other forms with additional validation logic and messages.

Combined with the fact that the CDN module will by default not serve CSS files from a CDN (since #2827998: Add a new default option to the CDN UI: "all files except CSS+JS", and make this the new default of the CDN module, include upgrade path), I think this adequately addresses the majority use case. We can add a warning message in the future if the need arises. But I'd rather not, because it slows things down and gets in the way.

Looks like a very good compromise. Intentionally disabling CSS aggregation for a site means you either have issues to fix that should get you worried or you know what you're doing and you can find workarounds for the CDN module.

Also agreed that settings.local.php is the way to go for sustainable dev mode rather than clicking around. And if you can deal with sites.php, surely you can deal with settings.local.php anyway.

#17 is a compelling reason to go with the patch in #15 IMHO. Also, with #2827998: Add a new default option to the CDN UI: "all files except CSS+JS", and make this the new default of the CDN module, include upgrade path now committed, the need should erode as time goes by.

Excellent, thanks for sharing your perspective, @anavarre!