A new all files except CSS+JS option that's available right in the CDN UI, and that would ship as the new default configuration of the CDN module would make sense:

  1. it's more valuable to load CSS & from origin, because the connection to origin is already open and they are the blocking resources
  2. all other files are not blocking the page from being rendered, and are usually larger (in terms of number of bytes to transfer) and would therefore benefit more from a CDN
  3. this then happens to also work around the symptom of the #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS Drupal core bug

See #2811615-5: [upstream] [Core bug, fixed since 9.5] AJAX commands that need additional JS to be loaded will fail when JS is loaded from CDN + #2811615-6: [upstream] [Core bug, fixed since 9.5] AJAX commands that need additional JS to be loaded will fail when JS is loaded from CDN.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

FileSize
13.21 KB
7.57 KB

#2 was just cosmetic changes; it did not yet have any effect. Now it does.

Wim Leers’s picture

FileSize
18.67 KB
5.43 KB

And now with an upgrade path, plus test coverage.

Wim Leers’s picture

Self-review.

  1. +++ b/cdn.install
    @@ -0,0 +1,31 @@
    + * Install and update functions for the CDN module.
    

    s/Install and update/Update/

  2. +++ b/cdn.install
    @@ -0,0 +1,31 @@
    + * Update the default configuration if it's not modified.
    

    s/configuration/settings/

  3. +++ b/cdn.module
    @@ -19,7 +19,7 @@ function cdn_help($route_name, RouteMatchInterface $route_match) {
    -        $output .= '<dd>' . t('By letting a CDN serve files instead of your webserver(s), it is possible to significantly reduce the load on your server load and its costs. Note that it may just as well end up costing more — it depends on pricing of the CDN and your server(s).') . '</dd>';
    +        $output .= '<dd>' . t('By letting a CDN serve files instead of your webserver(s), it is possible to significantly reduce the load on your server and its costs. Note that it may just as well end up costing more — it depends on pricing of the CDN and your server(s).') . '</dd>';
    

    IDK how this snuck in here. Unrleated change.

  4. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
    @@ -164,9 +165,17 @@ class CdnSettingsForm extends ConfigFormBase {
    +      // Plus one particular common preset: 'media files', which means all files
    

    Outdated, should now say 'nocssjs'.

  5. +++ b/src/CdnSettings.php
    @@ -117,7 +126,10 @@ class CdnSettings {
    +        assert(!empty($nested_mapping['conditions']), 'The nested mapping ' . $i . ' includes no conditions, which is not allowed for complex mappings.');
    

    This is validation that should have already existed. Should be done in a separate issue.

  6. +++ b/src/CdnSettings.php
    @@ -117,7 +126,10 @@ class CdnSettings {
    +        assert(!empty($nested_mapping['conditions']), 'The nested mapping ' . $i . ' includes no conditions, which is not allowed for complex mappings.');
    +        assert(!isset($nested_mapping['conditions']['not']), 'The nested mapping ' . $i . ' includes blacklist conditions, which is not allowed for complex mappings: the fallback_domain already serves this purpose.');
    

    Both of these should put the assertion logic in single quotes.

  7. +++ b/tests/src/Unit/CdnSettingsTest.php
    @@ -25,7 +25,7 @@ class CdnSettingsTest extends UnitTestCase {
    -      'simple, no conditions' => [
    +      'simple, on, no conditions' => [
    
    @@ -37,7 +37,7 @@ class CdnSettingsTest extends UnitTestCase {
    -      'simple, oone empty condition' => [
    +      'simple, on, one empty condition' => [
    
    @@ -69,7 +69,27 @@ class CdnSettingsTest extends UnitTestCase {
    -      'auto-balanced, on, no fallback' => [
    ...
    +      'auto-balanced, on' => [
    

    Out of scope, should be fixed in a separate issue.

  8. +++ b/tests/src/Unit/CdnSettingsTest.php
    @@ -198,6 +218,81 @@ class CdnSettingsTest extends UnitTestCase {
    +  /**
    +   * @covers ::getLookupTable
    +   * @expectedException \AssertionError
    +   * @expectedExceptionMessage The nested mapping 0 includes no conditions, which is not allowed for complex mappings.
    +   */
    +  public function testComplexMappingWithoutConditions() {
    +    $this->createCdnSettings([
    +      'status' => TRUE,
    +      'mapping' => [
    +        'type' => 'complex',
    +        'fallback_domain' => 'cdn.example.com',
    +        'domains' => [
    +          0 => [
    +            'type' => 'simple',
    +            'domain' => 'foo.example.com',
    +          ],
    +        ],
    +      ],
    +    ])->getLookupTable();
    +  }
    

    Again out of scope.

  9. +++ b/tests/src/Unit/CdnSettingsTest.php
    @@ -198,6 +218,81 @@ class CdnSettingsTest extends UnitTestCase {
    +  public function testComplexDomainWithBlacklist() {
    

    "blacklist" should be replaced with "negative conditions" everywhere?

anavarre’s picture

Registering a massive +1 here. Sane defaults mean less reasons for people to run into issues when they don't know what they're doing.

Wim Leers’s picture

Related issues: +#2828150: Clean-up + one piece of missing validation
FileSize
18.73 KB
4.77 KB

Addressed everything in #5, except the removal of out-of-scope changes. Opened #2828150: Clean-up + one piece of missing validation for that, and now getting that committed first.

Wim Leers’s picture

FileSize
15.82 KB
Wim Leers’s picture

FileSize
15.9 KB
701 bytes
+++ b/tests/fixtures/update/drupal-8.cdn-cdn_update_8001.php
@@ -0,0 +1,58 @@
+// Install the rest configuration.

s/rest configuration/default CDN settings/

Fixed.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready. Thanks, Aurelien, for your feedback! It's much appreciated :) And it helps convince me that this is the right call.

Wim Leers’s picture

Title: Add a new default option to the CDN UI: "all files except CSS+JS", and make this the new default of the CDN module » 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

  • Wim Leers committed 9c438c5 on 8.x-3.x
    Issue #2827998 by Wim Leers: Add a new default option to the CDN UI: "...
Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

Giving issue credit to Aurelien.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.