URL encoding not correctly handled by farfuture functionality

Thanks for the crystal-clear bug report!

\Drupal\Tests\cdn\Unit\File\FileUrlGeneratorTest::testGenerate() is testing "special" file names already. But \Drupal\Tests\cdn\Functional\CdnIntegrationTest::testFarfuture() isn't yet. So we should be able to expand that test to trigger a failure.

I was wrong in #1, \Drupal\Tests\cdn\Unit\File\FileUrlGeneratorTest::testGenerate() only tests non-far-future cases. It's \Drupal\Tests\cdn\Unit\File\FileUrlGeneratorTest::testGenerateFarfuture() that we should expand to test a "special" file name, because that contains the bug described in the last paragraph of the IS.

But on top of that, we also should expand \Drupal\Tests\cdn\Functional\CdnIntegrationTest::testFarfuture(), but we should convert it to a proper integration test.

First, let's fix the imperfect simulation of how PublicStream behaves. We have to mock that, because PublicStream can only ever be actually available in a kernel or functional test, whereas this is a unit test (which we want, because it's much faster).

Voila, this shows that our unit tests weren't reflecting reality. That's of course the danger of unit tests. But in this case, they're worth the trouble. This is (or should be) the only flaw in them.

Now that our unit tests do reflect reality, we can expand it with the missing unit test coverage.

This should fail.

This is the failure:

--- Expected
+++ Actual
@@ @@
-//cdn.example.com/cdn/farfuture/mklcqDAZef74DPzCW53N_mqaPMip9uNAKsluggC1sJk/1511362523/sites/default/files/llama%20%28aVD8XdYb%29.jpg
+//cdn.example.com/sites/default/files/llama%20%28aVD8XdYb%29.jpg

We expected the file to be served using a farfuture URL, but that was not the case. This is exactly the bug that was reported. So now we have a regression test 👍

And this then finally updates the integration test coverage. Instead of testing with public://druplicon.png and core/misc/drupal.js, this changes that all to public://druplicon ❤️.png.

This should cause another failure. And this means the regression test coverage is now complete.

Oops, I re-uploaded the #9 patch in #12. The interdiff was right though.

Great, those failures were expected! Now let's fix it.

The root cause lies in using relative URLs as paths, but relative URLs may look like paths, but they are of course URL-encoded. And that's what I overlooked here. I got this right when I ported farfuture functionality from D6 to D7 in #974350: Far Future setting for Origin Pull mode, but I got it wrong in the D8 port. My bad.

Green, yay!

And now we can actually clean up the logic a little bit (because with the few added lines, it looks weird). Slightly out of scope, but doing this in a separate issue also seems silly. Also fixing the 2 coding standards violations that I accidentally introduced earlier.

Also, this definitely is a major bug. Nothing actually breaks, which is why it's not critical, and which is why it went unnoticed until now!

Can you please apply the patch, and confirm that it solves the problem? Thank you!

Self-review.

1. +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php
   @@ -138,19 +139,29 @@ class FileUrlGeneratorTest extends UnitTestCase {
   +    // Generate file for testing non-shipped file.
   

   s/non-shipped/managed/

2. +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php
   @@ -138,19 +139,29 @@ class FileUrlGeneratorTest extends UnitTestCase {
   +    file_put_contents($llama_jpg_filepath, $this->randomMachineName());
   

   We should delete this file.

3. +++ b/tests/src/Unit/File/FileUrlGeneratorTest.php
   @@ -138,19 +139,29 @@ class FileUrlGeneratorTest extends UnitTestCase {
        $drupal_js_mtime = filemtime($this->root . '/core/misc/drupal.js');
        $drupal_js_security_token = Crypt::hmacBase64($drupal_js_mtime . '/core/misc/drupal.js', static::$privateKey . Settings::getHashSalt());
   

   This code is unnecessary, we should delete it.

Credited Steven Jones.

Great, thank you for testing!

Turns out the fix here was incomplete 😑 #3069516: Farfuture functionality + RFC3986-reserved characters = wrong URLs adds the finishing touch.