Support additional stream wrappers

Thanks for creating this issue! And thank you especially for providing a complete argumentation, as well as a patch!

My key concern is that this is complicating the CDN module and the CDN UI module for a 1% use case. However, I see that you've already ensured that this only complicates the UI if there are actually additional stream wrappers installed.

1. The current patch breaks uploaded files that use the public:// stream wrapper. Hence the failures.

2. +++ b/cdn.routing.yml
   @@ -1,7 +1,8 @@
   +    scheme: .+
   

   We can make this more strict: [a-z]+

3. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -74,7 +103,6 @@ class CdnSettingsForm extends ConfigFormBase {
   -      '#attributes' => ['class' => ['container-inline']],
   

   Accidental change?

4. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -143,6 +171,25 @@ class CdnSettingsForm extends ConfigFormBase {
   +    $wrappers = array_keys($this->streamWrapperManager
   +      ->getWrappers(StreamWrapperInterface::WRITE_VISIBLE));
   +    $localWrappers = array_keys($this->streamWrapperManager
   +      ->getWrappers(StreamWrapperInterface::LOCAL));
   +    $wrappers = array_diff($wrappers, $localWrappers);
   +    $existingWrappers = $config->get('additional_wrappers');
   

   This could use some clarification. I personally definitely don't quite understand this yet :)

   Also, why WRITE_VISIBLE, why not just VISIBLE?

5. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -143,6 +171,25 @@ class CdnSettingsForm extends ConfigFormBase {
   +    $form['wrappers']['additional_wrappers'] = [
   
   +++ b/config/install/cdn.settings.yml
   @@ -69,3 +69,4 @@ mapping:
   +additional_wrappers: []
   
   +++ b/config/schema/cdn.schema.yml
   @@ -20,3 +20,8 @@ cdn.settings:
   +    additional_wrappers:
   +      label: 'Additional stream wrappers for CDN'
   

   Let's change this to stream_wrappers.

   By making this list all CDN-enabled stream wrappers, we make the configuration more clear: no more "additional", and instead simply make it a declaration of all CDN-enabled stream wrappers. Much easier to understand.

6. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -143,6 +171,25 @@ class CdnSettingsForm extends ConfigFormBase {
   +      '#description' => $this->t('Additional stream wrappers to rewrite for CDN.')
   

   Let's change this to list all eligible stream wrappers, including public. Let's just make public checked by default, and impossible to uncheck.

7. +++ b/src/File/FileUrlGenerator.php
   @@ -111,78 +111,62 @@ class FileUrlGenerator {
   +      $scheme = '_shipped';
   

   Let's use :shipped:, because it's impossible to use colons in actual scheme names.

8. The URL signature of the far-future expiration service changes, however sites upgrading should just generate the "new" syntax. This will result in a period of CDN refresh as the edge servers would need to fetch previously-cached files in a new location.

   I'm afraid this is an oversimplification… Drupal's Page Cache and Dynamic Page Cache will contain cached responses. But those we could invalidate. What's worse, is that Varnish, company proxies and even Google's cache will contain cached responses too. And they'll be using the "old far future URLs". Which will no longer work. So this will definitely break things.

   To work around this, I'd suggest using the existing cdn.farfuture.download route for shipped + public:// files. We'll need to introduce a separate route for other stream wrappers. Less elegant, but necessary to not break BC.

There's no interdiff for #4 or #6. Here's one. For the next time, please see https://www.drupal.org/documentation/git/interdiff :)

It's pretty much essential in order for me to give feedback on changes since the last review!

And of course I then fail to post the correct interdiff :) Here's the right one!

First round of review. The more simpler/more obvious things.

1. +++ b/cdn.install
   @@ -29,3 +29,14 @@ function cdn_update_8001() {
   +  if (is_null($cdn_settings->get('stream_wrappers'))) {
   

   There's no need for this if-test. Because new installations will have this new key, and only existing installations will run this update hook.

   Therefore you can simplify this. See e.g. \statistics_update_8002().

2. +++ b/cdn.install
   @@ -29,3 +29,14 @@ function cdn_update_8001() {
   +    $cdn_settings->set('stream_wrappers', []);
   
   +++ b/config/install/cdn.settings.yml
   @@ -69,4 +69,6 @@ mapping:
   +stream_wrappers: []
   

   This should not set [] as the default, but ['public'].

3. +++ b/cdn.routing.yml
   @@ -1,8 +1,10 @@
   -cdn.farfuture.download:
   -  path: '/cdn/farfuture/{security_token}/{mtime}/{scheme}'
   +# The /cdn/farfuture route has been deprecated and is rewritten
   +# in CdnFarfuturePathProcessor.
   +cdn.farfuture_scheme.download:
   +  path: '/cdn/ff/{security_token}/{mtime}/{scheme}'
   

   If you're going to rewrite the incoming path (CLEVER!), then why even rename the route?

   The new route name makes less sense IMO.

4. +++ b/cdn.routing.yml
   @@ -1,8 +1,10 @@
   +    scheme: '(:\w+:)|([a-zA-Z0-9+.-]+)'
   

   I'm not sure what the portion before the pipe is doing? Can you add a comment here?

5. +++ b/cdn.services.yml
   @@ -1,7 +1,7 @@
        class: Drupal\cdn\CdnSettings
   -    arguments: ['@config.factory']
   +    arguments: ['@config.factory', '@stream_wrapper_manager']
   

   I don't like that this class is now no longer just a "configuration interpreter". I'm curious to see why you chose to do this.

6. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -171,25 +171,27 @@ class CdnSettingsForm extends ConfigFormBase {
   -      '#access' => $wrappers || $existingWrappers,
   

   I think we should still deny access if no custom stream wrappers were installed. I.e. if only public is shown, then let's not expose this at all.

   To quote myself from #3:

   However, I see that you've already ensured that this only complicates the UI if there are actually additional stream wrappers installed.

   That's no longer true in #6.

7. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -171,25 +171,27 @@ class CdnSettingsForm extends ConfigFormBase {
   +      '#description' => $this->t('Stream wrappers to rewrite for CDN. "Local" stream wrappers are always enabled.')
   
   +++ b/config/install/cdn.settings.yml
   @@ -69,4 +69,6 @@ mapping:
   +# All LOCAL_NORMAL wrappers are enabled by rule,
   +# additional wrappers can be added.
   

   This is not true. private:// is also LOCAL_NORMAL, yet the CDN module is not enabled for it.

8. +++ b/src/CdnSettings.php
   @@ -79,13 +89,15 @@ class CdnSettings {
   +  public function streamWrappers() {
   +    $configured = $this->rawSettings->get('stream_wrappers');
   +    // LOCAL_NORMAL stream wrappers are always served.
   +    return array_merge(array_keys($this->streamWrapperManager
   +      ->getWrappers(StreamWrapperInterface::LOCAL_NORMAL)), $configured);
   

   Ahh, this is injecting the stream wrapper manager service so that the return value here can be dynamically computed!

   If you set the default value to ['public'], that'd be a non-issue.

9. +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php
   @@ -67,10 +67,11 @@ class FileUrlGeneratorTest extends UnitTestCase {
   -        'farfuture' => [
   -          'status' => FALSE,
   -        ],
          ],
   +      'farfuture' => [
   +        'status' => FALSE,
   +      ],
   

   Oh wait, what?! This was in the wrong place before!? Great catch. I guess it was passing simply because the expected key did not exist in the expected place then.

10. +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php
    @@ -182,8 +184,8 @@ class FileUrlGeneratorTest extends UnitTestCase {
    -    $stream_wrapper_manager->getWrappers(StreamWrapperInterface::LOCAL)
    -      ->willReturn(['public' => TRUE, 'private' => TRUE]);
    +    $stream_wrapper_manager->getWrappers(StreamWrapperInterface::LOCAL_NORMAL)
    +      ->willReturn(['public' => TRUE]);
    

    Changing this is a lie? private:// is LOCAL_NORMAL too! See \Drupal\Core\StreamWrapper\PrivateStream::getType().

    Making it actually declarative, and defaulting to ['public'] solves this.

11. If I go to update.php, I'm seeing error messages. See attached screenshot. They do disappear after applying the updates. But that's of course less than ideal. Let's remember to fix that, hopefully the changes made because of my reviews will cause them to disappear without extra effort :) (Though I'm not at all sure that will be the case, I just want to avoid that you spend lots of time on this in vain!)

1. +++ b/src/PathProcessor/CdnFarfuturePathProcessor.php
   @@ -19,19 +23,30 @@ class CdnFarfuturePathProcessor implements InboundPathProcessorInterface {
   -    if (strpos($path, '/cdn/farfuture/') !== 0) {
   +    if (!preg_match('/^\/cdn\/(ff|farfuture)\/.*/', $path, $matches)) {
   ...
   +    // Backwards compatibility for non-scheme aware farfuture paths.
   +    if ($matches[1] == 'farfuture') {
   

   Hah, clever!

   However, you've replaced the strpos() in the critical path with a preg_match() in the critical path.

   Remember, inbound path processors run on every request! So this is not ideal.

   I suggest you do the following:

   function processInbound() {
     if (old strpos()-based check)
       $path = rewriteBcPath($path)
     }
   
     if (new strpos()-based check) {
     // rest of the code
   

   That replaces it by two strpos() checks, which is simpler to understand, simpler to maintain and should be faster.

2. +++ b/src/CdnFarfutureController.php
   @@ -61,15 +85,18 @@ class CdnFarfutureController implements ContainerInjectionInterface {
   -  public function download(Request $request, $security_token, $mtime) {
   ...
   +  public function download(Request $request, $security_token, $mtime, $scheme) {
   
   @@ -105,7 +132,10 @@ class CdnFarfutureController implements ContainerInjectionInterface {
   -    $response = new BinaryFileResponse(substr($root_relative_file_url, 1), 200, $farfuture_headers, TRUE, NULL, FALSE, FALSE);
   ...
   +    $response = new BinaryFileResponse($uri, 200, $farfuture_headers, TRUE, NULL, FALSE, FALSE);
   

   Hrm… does this actually even make sense?

   What if you're using Flysystem to store files on S3? Then this would cause the file to be downloaded from S3 to Drupal, and then be served via Symfony's BinaryFileResponse, which will then send it again.

   In such scenarios, it'd be far better to just point your origin pull CDN at your S3 domain, and then use those URLs instead…

   This calls into question the entire point of this issue I'm afraid…

   Did you consider this? What are your thoughts around this?

Turns out we had a similar discussion/issue at #1863310: CDN module should know how to deal with custom stream wrappers.

Or, I have a module that generates CloudFront URLs with signed cookies/query strings to leverage CDN+authz. In that case, you need Drupal to act effectively as an HTTP proxy in front of the storage providing middleware for the CDN.

Drupal acting as a HTTP proxy will not work well: Drupal was not designed for that.

So while this does mean piping the file through Drupal, we're doing so way less often than without the CDN.

True for anonymously available content, but not for signed requests…

Does that help make the case a little better?

Yes and no. The anon use case might make sense, but the auth one definitely does not. You mention both. Can you elaborate a bit on your concrete use case?

Pinged @bradjones1 on Twitter: https://twitter.com/wimleers/status/863081189690757120.

Damn. This time I definitely dropped the ball. I'm very sorry for the long delay, Brad!

I first interpreted CloudFront URLs with signed cookies/query strings to leverage CDN+authz. as meaning "authentication between end user and the external file storage", but it sounds like it's really "authentication between Drupal and the external file storage". Meaning that all those files are safe to serve to anonymous users. That changes things of course.

The more I read about this, the more I realized this is primarily about supporting https://www.drupal.org/project/flysystem. So I started looking at Flysystem's source code. That's where the WRITE_VISIBLE came from: from \Drupal\flysystem\FlysystemBridge::getType(). What I see makes sense.

I don't think CDN is the right module to put *all* this logic, but supporting additional stream wrappers and exposing some of the settings and other internal bits to reading by other configuration makes it rather trivial to generate CDN URLs

This makes sense.

Conclusion

You convinced me. Let's do this.

I have a few hard requirements, to ensure the CDN module remains both stable and low-maintenance (because I'll be the one supporting this new feature in the future!):

1. this new vertical tab in the CDN UI must ONLY be visible if additional stream wrappers besides public and private are installed (this used to be the case in the patch, but is currently broken)
2. that vertical tab must list public as a checkbox, but it must always be checked and always disabled (already the case)
3. that vertical tab must indicate which stream wrappers are remote and which are local (not yet the case, but trivial to add)
4. test coverage checking that this behaves as expected when installing additional stream wrappers that either qualify (VISIBLE) or not
5. test coverage proving that existing Far Future URLs will not break (already done!)

I think these are reasonable — and I'll help you get this patch to that point :)

Next steps

The current patch has 258 insertions, 81 deletions as its diffstat. But quite a bit of this is due to refactoring. I'll already do some refactoring to make the patch simpler to review. Stay tuned.

Created #2891162: Refactor: extract logic from FileUrlGenerator::generate() into a new ::getCdnDomain() method to allow this patch to become simpler.

Things that stand out in the current patch:

1. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -74,7 +103,6 @@ class CdnSettingsForm extends ConfigFormBase {
   -      '#attributes' => ['class' => ['container-inline']],
   

   Why this change?

2. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -143,6 +171,27 @@ class CdnSettingsForm extends ConfigFormBase {
   +    $form['wrappers'] = [
   

   This should be hidden unless there are non-default wrappers installed that are eligible for configuration here.

3. +++ b/config/install/cdn.settings.yml
   @@ -69,3 +69,6 @@ mapping:
   +# All LOCAL_NORMAL wrappers are enabled by rule,
   +# additional wrappers can be added.
   +stream_wrappers: []
   

   That's not actually true, only public is enabled by default.

   I think this should be:

   stream_wrappers:
     - public
   

4. +++ b/src/CdnFarfutureController.php
   @@ -15,6 +17,13 @@ use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
   +  protected $root;
   
   @@ -105,7 +132,10 @@ class CdnFarfutureController implements ContainerInjectionInterface {
   -    $response = new BinaryFileResponse(substr($root_relative_file_url, 1), 200, $farfuture_headers, TRUE, NULL, FALSE, FALSE);
   +    if ($scheme == FileUrlGenerator::RELATIVE) {
   +      $uri = $this->root . $uri;
   +    }
   

   Why is this app root suddenly needed? It's been working fine without that. So I'm confused. This seems an unrelated/out-of-scope change?

5. +++ b/src/CdnSettings.php
   @@ -78,6 +89,18 @@ class CdnSettings {
   +    $configured = $this->rawSettings->get('stream_wrappers');
   +    // LOCAL_NORMAL stream wrappers are always served.
   +    return array_merge(array_keys($this->streamWrapperManager
   +      ->getWrappers(StreamWrapperInterface::LOCAL_NORMAL)), $configured);
   

   Why is this returning streamwrapper instances, rather than just streamwrapper schemes?

6. +++ b/src/PathProcessor/CdnFarfuturePathProcessor.php
   @@ -19,19 +23,30 @@ class CdnFarfuturePathProcessor implements InboundPathProcessorInterface {
   +    if (!preg_match('/^\/cdn\/(ff|farfuture)\/.*/', $path, $matches)) {
   

   Should use strpos(), like I said before. This is in the critical path, so we can't regress performance here.

7. +++ b/src/PathProcessor/CdnFarfuturePathProcessor.php
   @@ -19,19 +23,30 @@ class CdnFarfuturePathProcessor implements InboundPathProcessorInterface {
   +    // Backwards compatibility for non-scheme aware farfuture paths.
   +    if ($matches[1] == 'farfuture') {
   ...
   +    else {
   

   This makes the code unnecessarily hard to read (and maintain).

   Rather than an if/else, it'd be better to have multiple methods.

   Also, it'd be great if any code that is there for BC would be in a separate method that can simply be deleted altogether (along with its invocation that will need to be deleted). That will make removing BC layers in the future much, much easier :)

I just realized something.

It looks like you've designed this to work with the Forever cacheable files ("Far Future") functionality enabled. That's fine. But it means that you're making one important assumption: that everybody has this enabled. If they don't have it enabled, there are severe consequences:

1. Without Forever cacheable files, the CDN domain must point at the external storage service' public URL … but this breaks for e.g. Swift, which you mentioned in #12 that it might not have publicly accessible URLs
2. With Forever cacheable files, you can use a single CDN domain for all files: Drupal's shipped files, public:// files, but also files stored in anything that Flysystem supports (thanks to Drupal just downloading the file from Flysystem and then serving it). But end users may not realize that this is the case, decide to turn of Forever cacheable files, and things would break horribly.

What are your thoughts on this? My only thought about how this could be prevented is that the Flysystem module could provide a config override that ensures that Forever cacheable files cannot be disabled.

❤️ — looking forward to your feedback! I'm planning to tag a new release soon, so let's get this done (either committed, or agree that it's not a good match after all).

#24:

because the URL the wrapper generates is something like /_flysystem/wrapper/file

I did not realize that. Interesting! That means the CDN module would also have an effect even without Forever cacheable files turned on.

I suppose it's possible that a different stream wrapper implementation would generate absolute URLs, however those I don't believe would be rewritten by CDN in the first place

This is definitely not just possible, but a reality. And yes, in those cases, the CDN module would not rewrite those URLs, and that's exactly my point in #21: if you'd enable the CDN module for a stream wrapper with its own absolute HTTP URLs (which I think is the majority use case, Flysystem is the exception AFAICT), then it really has no effect at all, which is the misleading thing I'm concerned about!

It does raise the question of whether that makes checking the box for CDN treatment a false choice, but this whole thing is an advanced operation so in that case I think it's safe to assume the developer knows what he's doing.

That is indeed a "false choice" (a choice/setting without consequences). I don't think you can necessarily make this assumption. There were plenty of support requests about confusing things in the CDN module and its UI in the past, and that's something I intentionally fixed in the D8 port: https://wimleers.com/article/simplicity-maintainability-cdn-module-drupal-8.

#26:

it's actually more likely than I implied earlier that the URL generated by, say, a flysystem plugin would be external. E.g., S3.

Thanks for double-checking that. So then my concern definitely still stands.

However the point remains, if the URL that CDN sees is external, it wouldn't be rewritten.

I've explained why this is still a problem: it doesn't make sense for the user's mental model: it'd be a setting without any effect.

#32:

Related ticket that will need created if/when this merges: Allow CDN host selection to be mapped by stream wrapper.

Do you mean different CDN domain per Drupal site/domain/host in a multi-site setup?

Sorry for taking longer than I'd hoped!

Unfortunately, that's also largely because there was quite a lot of work left to be done.

1. No interdiffs for #36 + #38 + #40
2. I spent more than an hour getting https://www.drupal.org/project/flysystem to work, so I was able to see how this would impact the UI. I did not succeed. I did succeed today, by reading their README and making those weird settings.php additions.
   An easier way to test this is by installing the file_test module in core.
3. The patch in #40 is resulting in PHP warning notices, on a vanilla Drupal installation, when you go to update.php. Along the lines of:
   

   Warning: array_merge(): Argument #2 is not an array in Drupal\cdn\CdnSettings->streamWrappers() (line 101 of modules/cdn/src/CdnSettings.php).
   

4. 4 new coding standards violations reported by DrupalCI.

Fixes:

• Point 3 is because DbUpdateController.php actually no longer defines the MAINTENANCE_MODE constant, so
  

  function cdn_file_url_alter(&$uri) {
    // Don't alter file URLs when running update.php.
    if (defined('MAINTENANCE_MODE')) {
      return;
    }
  
  

  doesn't actually work. IMHO this is a bug in core, but for now the CDN module will have to detect this.

• cdn_update_8002() doesn't actually install the default settings that the updated cdn.settings.yml contains.
  I already pointed this out in #10.1 and #10.2, more than a year ago… Fixed now.
• I installed the file_test module, which registers the dummy, dummy-remote and dummy-readonly stream wrappers. This causes the Stream wrappers tab to show up! 👍
  Unfortunately, it automatically checks the dummy-remote stream wrapper's checkbox, despite that not actually being enabled in configuration:
  
  This is not acceptable.
• When the private file system is installed, that's also listed (fine), disabled (fine), but also checked (VERY BAD!):
  
• This caused me to re-scrutinize all the changes in CdnSettingsForm. I ended up digging into flysystemagain and in doing so found that
  

  I actually don't think that's necessarily true. E.g. for stream wrappers provided by a Flysystem implementation, the CDN domain just sits in front of Drupal like the other stream wrappers, because the URL the wrapper generates is something like /_flysystem/wrapper/file

  from #24 was not quite the whole story. \Drupal\flysystem\FlysystemBridge specifies WRITE_VISIBLE. That excludes the LOCAL bit, because Flysystem can include remote data. It just happens to be that the flysystem setup you're using uses \Drupal\flysystem\Flysystem\Local or \Drupal\flysystem\Flysystem\Ftp.
  Of course, the blame hardly lies with you, but rather with both the Drupal stream wrapper system for being so confusing and especially the flysystem Drupal module and PHP library.
  Fortunately, the "check external URL" heuristic we discussed at DrupalCon seems to do the trick. Still, I'd like to see this confirmed by somebody with e.g. flysystem + S3 buckets.

• Upon submit, the public:// stream wrapper is always being removed 😨
• #10.5 was never addressed. This was necessary back then because magic determined which stream wrappers the CDN module would process, but since #24 that magic is gone. And with it,
  the reason to have the stream wrapper manager service injected into CdnSettings.

I'm honestly a bit disappointed. You didn't actually test this patch as thoroughly as I previously had the impression. But I know that A) contributing is hard, B) contributing when stream wrappers is triply so! So I understand. Please review my reroll!

Finally, another patch review, for the files I touched above:

1. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -143,6 +173,49 @@ class CdnSettingsForm extends ConfigFormBase {
   +    $visibleWrappers = array_keys($this->streamWrapperManager
   +      ->getWrappers(StreamWrapperInterface::VISIBLE));
   +    $localWrappers = array_keys($this->streamWrapperManager
   +      ->getWrappers(StreamWrapperInterface::LOCAL_NORMAL));
   +    $wrappers = array_unique(array_merge($localWrappers, $visibleWrappers));
   ...
   +      '#access' => count($wrappers) > count($localWrappers),
   

   This is brittle, and also not quite right, at least not anymore. Now that the magic is gone (no more automatic whitelisting of all LOCAL_NORMAL stream wrappers), this logic needs to be updated. It needs to show this new vertical tab whenever there are any visible stream wrappers besides the ones in core.

2. +++ b/cdn_ui/src/Form/CdnSettingsForm.php
   @@ -206,4 +282,11 @@ class CdnSettingsForm extends ConfigFormBase {
   +  protected function getBasePath() {
   +    return $this->getRequest()->getBasePath();
   +  }
   

   This is redundant.

3. +++ b/config/install/cdn.settings.yml
   @@ -69,3 +69,6 @@ mapping:
   +# Public is enabled by rule, additional wrappers can be added.
   

   Not by rule, but by default. Users who use just the module and not the UI are free to override this.

4. +++ b/config/schema/cdn.schema.yml
   @@ -20,3 +20,8 @@ cdn.settings:
   +      label: 'Stream wrappers for CDN'
   

   This is ambiguous. "CDN-enabled stream wrappers" is clearer.

5. +++ b/config/schema/cdn.schema.yml
   @@ -20,3 +20,8 @@ cdn.settings:
   +        type: string
   

   Let's be more specific.

And that's just the UI + config + config schema + update path portions. I have not yet started reviewing or testing the functional changes in FileUrlGenerator.

Now the UI looks like this (with file_test + flysystem with two FTP adapters):

d.o removed one file…

Reverted the phpcs.xml changes, which are most definitely unwanted.

These changes theoretically belong in a separate issue:

• +++ b/cdn.module
  @@ -34,7 +34,8 @@ function cdn_help($route_name, RouteMatchInterface $route_match) {
  -  if (defined('MAINTENANCE_MODE')) {
  +  // @todo Drupal 8 no longer defines the MAINTENANCE_MODE constant when running update.php; once that is fixed, we can stop using $_SERVER here.
  +  if (stripos($_SERVER['PHP_SELF'], 'update.php') !== FALSE) {
  

• +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php
  @@ -67,10 +68,11 @@ class FileUrlGeneratorTest extends UnitTestCase {
  -        'farfuture' => [
  -          'status' => FALSE,
  -        ],
         ],
  +      'farfuture' => [
  +        'status' => FALSE,
  +      ],
  

High-level review of the remainder:

1. +++ b/cdn.routing.yml
   @@ -1,7 +1,17 @@
   +# The /cdn/farfuture route has been deprecated and is rewritten
   +# in CdnFarfuturePathProcessor.
    cdn.farfuture.download:
      path: '/cdn/farfuture/{security_token}/{mtime}'
      defaults:
        _controller: cdn.controller.farfuture:download
   +    _disable_route_normalizer: TRUE
   +  requirements:
   +    _access: 'TRUE'
   +    mtime: \d+
   +cdn.farfuture_scheme.download:
   +  path: '/cdn/ff/{security_token}/{mtime}/{scheme}'
   +  defaults:
   +    _controller: cdn.controller.farfuture:downloadByScheme
        # Ensure the redirect module does not redirect to add a language prefix.
        # @see \Drupal\redirect\EventSubscriber\RouteNormalizerRequestSubscriber
        # @todo Update this comment when https://www.drupal.org/project/drupal/issues/2641118 lands.
   @@ -9,3 +19,4 @@ cdn.farfuture.download:
   
   @@ -9,3 +19,4 @@ cdn.farfuture.download:
      requirements:
        _access: 'TRUE'
        mtime: \d+
   +    scheme: '(:\w+:)|([a-zA-Z0-9+.-]+)'
   

   Ideally these changes could be made simpler.

2. +++ b/config/schema/cdn.schema.yml
   index e1f7bea..0ef5fda 100644
   --- a/src/CdnFarfutureController.php
   

   Ideally this could be made simpler.

3. +++ b/src/CdnSettings.php
   @@ -175,4 +189,47 @@ class CdnSettings {
   +  public function getCdnDomain($uri) {
   

   Ideally we wouldn't need something like this.

4. +++ b/src/CdnSettings.php
   --- a/src/File/FileUrlGenerator.php
   +++ b/src/File/FileUrlGenerator.php
   

   The changes here are fairly complex. Tried to simplify.

5. +++ b/src/File/FileUrlGenerator.php
   --- a/src/PathProcessor/CdnFarfuturePathProcessor.php
   +++ b/src/PathProcessor/CdnFarfuturePathProcessor.php
   

   This looks good.

6. +++ b/src/PathProcessor/CdnFarfuturePathProcessor.php
   --- a/tests/src/Functional/CdnIntegrationTest.php
   +++ b/tests/src/Functional/CdnIntegrationTest.php
   

   This looks good.

7. +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php
   @@ -133,6 +135,12 @@ class FileUrlGeneratorTest extends UnitTestCase {
   +      // File is used here generically to test a stream wrapper that is not
   +      // shipped with Drupal, but is natively supported by PHP.
   +      // @see \Drupal\cdn\File\FileUrlGenerator::generate(), which uses
   +      // file_exists() and would require actually configuring the stream
   +      // wrapper in the context of the unit test.
   +      'stream_wrappers' => ['public', 'file'],
   

   This looks questionable. Should probably install the file_test module and use that module's stream wrappers?

   Also, overall, test coverage here seems superficial; it doesn't cover the test case of #18.4 for example AFAICT.

I'll try to post a detailed comment plus patch reroll later today.

Fixed the first portion of what I called out in #43 as belonging in a separate issue in #2969048: Core's DbUpdateController does not define MAINTENANCE_MODE, causes CDN module to run even for update.php, and added test coverage there.

While working on this yesterday, I started thinking that this was too brittle:

+++ b/cdn_ui/src/Form/CdnSettingsForm.php
@@ -143,10 +174,82 @@ class CdnSettingsForm extends ConfigFormBase {
+  protected function streamWrapperGeneratesExternalUrls($stream_wrapper_scheme, StreamWrapperInterface $stream_wrapper) {
+    // Generate URL to imaginary file 'cdn.test'. Most stream wrappers don't
+    // check file existence, just concatenate strings.
+    $stream_wrapper->setUri($stream_wrapper_scheme . '://cdn.test');
+    try {
+      $absolute_url = $stream_wrapper->getExternalUrl();
+      $base_url = $this->getRequest()->getSchemeAndHttpHost() . $this->getRequest()->getBasePath();
+      $relative_url = str_replace($base_url, '', $absolute_url);
+      return UrlHelper::isExternal($relative_url);
+    }
+    catch (\Exception $e) {
+      // In case of failure, assume this would have resulted in an external URL.
+      return TRUE;
+    }
+  }

@@ -169,6 +272,12 @@ class CdnSettingsForm extends ConfigFormBase {
+    $config->set('stream_wrappers', $stream_wrappers);

+++ b/config/install/cdn.settings.yml
@@ -69,3 +69,6 @@ mapping:
+stream_wrappers:
+  - public

What if instead, we could move this into validation constraints?

Well, we can, but it's new territory, despite Drupal 8 clearly being headed there.

So I decided to make the CDN module pave the path: #2969065: Use typed config validation constraints for validation of cdn.settings simple config.

That'd also make it much easier to write the necessary tests!

Also, we discussed this at DrupalCon: this is the sort of major new feature that warrants a new major version. Which means … we only need to care about an update path, we don't need to care about BC layers. Which means that all the update path complexity could go away!

Before this can go in, we'll also need to investigate how the CDN module can support uses cases such as https://www.drupal.org/project/cdn_cloudfront_private. Any insight into that would be most appreciated.

Committed #2969065: Use typed config validation constraints for validation of cdn.settings simple config :)

Rebased.

Pair-programmed with @bradjones1 at DrupalCon Seattle to fix the current test failures 🥳

Fixed the UI problems.

Only remaining thing: the validation constraint. Can follow the example set by #2969065: Use typed config validation constraints for validation of cdn.settings simple config :)

I was flying during comments #59 through #62, and sleeping for comment #63 :D

I promise I'll follow up here in the next few days.

• Tests have been written, so removing Needs tests :)
• Tried to convert \Drupal\cdn\Plugin\Validation\Constraint\CdnStreamWrapperConstraintValidator into a validator.constraint_validator-tagged service to avoid service location and instead use service injection, but … Drupal core does not (yet?) use \Symfony\Component\Validator\DependencyInjection\AddConstraintValidatorsPass in its container compilation process and hence that does not work. So leaving it as-is.
• Fixed CS violations I introduced in #58 :P
• Fixed a bunch of nits in #59–#63.
• LGTM! 😃👍🥳

Let's see if this passes all CS standards tests. If it does, I'll commit this!

2 remaining coding standards violations. Will fix those on commit. Will commit tomorrow, at a family gathering right now :)

Thanks so much for your willingness and patience to do things right, Brad! 🙏 👏

I wanted to commit this just before going to sleep … and then in a final review of the entire patch I found lots more nits. 😓 Past midnight now, so not finishing this now.

This is what I found & fixed so far. All nits! Nothing more. But I want to get those done prior to commit. Just bear with me a little longer!

More nits fixed:

1. copy/pasted comments that needed updating
2. made non-strict comparisons strict
3. fooBar -> foo_bar
4. @returns -> @return (was already a problem in HEAD, not this patch's fault)
5. deprecation handling

And finally, changed some bits wrt rawurldecode() and naming that I found confusing. It's important that it makes sense to me, because I'll be maintaining this for years to come :)

It's now more similar to the old code, both in variable naming and execution sequence.

I screwed up a @deprecated annotation in #69.

Also … we should update the CDN UI tour! We completely forgot about that :)

It's pretty clear at this point that this is a major new feature for the CDN module, so updating issue metadata to reflect that.

Finally, I'd like one final manual test from @bradjones1 to ensure that my changes didn't break anything for him :)

🥳 Nice! 😃

I noticed yet one more thing we hadn't done yet (man, the list of best practices is looooong isn't it?): an update path test. Done.

When this comes back green in ~5 minutes, I'll finally commit it 💪

🚢

Change record created: https://www.drupal.org/node/3051642

Despite tests passing here (in #73), in automated testing I'm seeing 4 coding standards violations: https://www.drupal.org/pift-ci-job/1274957:

The attached patch should fix that. Except that there's a bug in drupal/coder, that will cause this to fail until A) the fix (#3050166: Contrib project version is not correctly matched in @deprecated tag) ships in a release, B) d.o updates to that release.

The remaining failures are indeed caused by #3050166: Contrib project version is not correctly matched in @deprecated tag.

Talked to @deprecated experts:

@Gábor Hojtsy (he/him) @mglaman FYI: https://www.drupal.org/project/cdn/issues/3044414#comment-13086118 — one more contrib module using `phpstan`! In the comment I linked, you can find two errors I did not fix: the first seems wrong, the second I cannot fix. Thoughts?

alexpott   [2 days ago]
The calling your own deprecated code is interesting.

alexpott   [2 days ago]
Core’s new rules are that you don’t call deprecated code from runtime code - cause that’s not deprecated :slightly_smiling_face:

alexpott   [2 days ago]
I guess that this code is handling some legacy format and so needs to be called to support the deprecation

alexpott   [2 days ago]
… of that legacy format.

alexpott   [2 days ago]
Also I :heart: seeing all the Unicode:: replacement. polyfills ftw.

wimleers (he/him)   [2 days ago]
> I guess that this code is handling some legacy format and so needs to be called to support the deprecation
Exactly!

alexpott   [2 days ago]
So in core this code would not have an @deprecated block but it would call @trigger_error if it found something to process.

wimleers (he/him)   [2 days ago]
But it _is_ a deprecated code path.

wimleers (he/him)   [2 days ago]
I’m guessing you’re saying we _would not_ annotate it?

wimleers (he/him)   [2 days ago]
(in core)

wimleers (he/him)   [2 days ago]
Because we only care about _public API deprecations_?

wimleers (he/him)   [2 days ago]
This _is_ a private API.

alexpott   [2 days ago]
Yep something along those lines.

alexpott   [2 days ago]
This is a tricky case - often we wouldn’t extract the legacy processing into its own method because then you don’t get into this situation

wimleers (he/him)   [2 days ago]
Right, that usually makes sense in core.

wimleers (he/him)   [2 days ago]
I guess I’ll just replace the `@deprecated` with a `@todo`, that achieves the same, and probably makes more sense given this is internal

wimleers (he/him)   [2 days ago]
Thanks, @alexpott! (edited)

D'oh.

#76:

Awesome! I take it you're tagging 3.3 along with this?

#77 introduced a last delay, but here we finally are: https://www.drupal.org/project/cdn/releases/8.x-3.3 😃

We missed one detail here: #3065145: Follow-up for #2870435: Deprecated apache configuration provided in README.txt.

And one more detail: #3063498: Do not validate configuration while syncing (support installing from existing config.).