Add additional test coverage for CssOptimizer

The last submitted patch, 2: 3353808-2.patch, failed testing. View results →

The test failure confirms the suspected regression 😔

This is probably it.

Apparently not - I don't have Drupal installed in a subdirectory locally, so getting the test to fail the right way wasn't happening. Maybe we need an additional call to ::transformRelative()?

The last submitted patch, 5: 3353808.patch, failed testing. View results →

Alternative patch for #2 to verify that this fails on 10.0 too.

+++ b/core/tests/Drupal/FunctionalTests/Asset/RootRelativeUrlsInAggregatesTest.php
@@ -0,0 +1,50 @@
+    $css = $this->drupalGet($this->baseUrl . $href);

I think this is why this is a false alarm.

This is wrongly requesting a root-relative URL in $href prefixed by the base URL, which includes the hostname AND the subdirectory in which Drupal is installed.

Which is just plain wrong! 🙈

Unfortunately, \Drupal\Tests\UiHelperTrait::drupalGet() does not know how to follow links: it cannot handle root-relative URLs 😳 So we need this:

-    $css = $this->drupalGet($this->baseUrl . $href);
+    $parts = parse_url($this->baseUrl);
+    $scheme_and_host_of_base_url = sprintf("%s://%s", $parts['scheme'], $parts['host']);
+    $css = $this->drupalGet($scheme_and_host_of_base_url . $href);

Let's find out…

Correct patch.

Alternatively:

    $this->getSession()->visit($href);
    $css = $this->getSession()->getPage()->getContent();

… if #10 is green, then I'm going to close this. Sorry for the noise 🙈

I now wonder how this worked fine for about a decade 😅

The last submitted patch, 8: 3353808-test-only-FAIL-8.patch, failed testing. View results →

I spent more time than I care to admit trying to make the test pass in so many different ways (every way I could think of to generate a file URL), didn't occur to me that the test might be broken!

However in the process of that, I did wonder if we should be doing this instead when building the asset URI. If we wanted to, we can remove the file URL generator from CssCollectionOptimizerLazy and should also make the js version work the same way.

I also think we could keep the test coverage here (once the test is correct :P).

#10 is green, but happier issue title rather than closing, at least for now.

I now wonder how this worked fine for about a decade 😅

That's basically been a recurring thought for me when trying to get a complex site up and running on DigitalOcean's App Platform with multiple containers of the site code, some web accessible and some not with different addresses - I ran into many problems with the asset system and how it rewrites URLs in aggregated CSS that are largely off topic here (see kind of hacky and temporary decorated CssOptimizer on GitHub) but just saying yup, same.

I spent more time than I care to admit trying to make the test pass in so many different ways (every way I could think of to generate a file URL), didn't occur to me that the test might be broken!

Same thing here 😅🫣

However in the process of that, I did wonder if we should be doing this instead when building the asset URI.

Yes, we should! Query strings and fragments in url(…) references in CSS files is one of the things that has been causing bug reports against the CDN module for over a decade:

1. #1514182: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation →
2. #1978170: CSS aggregation override breaks querystrings of referenced files →
3. #3128116: Negated file extensions in file URLs with querystrings or fragments are not recognized correctly →

And I think \Drupal\Tests\cdn\Unit\File\FileUrlGeneratorTest::urlProvider() now provides a pretty solid/comprehensive set of test cases: https://git.drupalcode.org/project/cdn/-/blob/4.0.1/tests/src/Unit/File/.... We'd want to change that to not be CDN-simulated of course, but other than that, if we'd test a CSS file containing every edge case listed there, we'd be better off for sure.

The urldecode is extremely annoying though, so we might want a follow-up for that bit.

Hah, that is that other edge case which has seen many bug reports over the years, but I can only quickly find this one: #1719568: CDN URLs are not properly encoded in some edge cases → .

That too is covered by that same test case I just linked 😊

@catch Would you be interested in me porting that test coverage to core?

Opened 📌 Tidy up URL generation for asset aggregates Needs work for the asset URL generation clean-up.

@Wim that sounds good, it might help one of the many related issues on #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it → .

Can't think of anything else here.

+++ b/core/tests/Drupal/FunctionalTests/Asset/RootRelativeUrlsInAggregatesTest.php
@@ -0,0 +1,52 @@
+   * Regression?

This comment won't mean much to us in the future, let's add something a bit more descriptive.