CSS Aggregation should not rewrite # url

I've verified that I'm seeing the same behavior - my clip-path: url(#something) gets rewritten to a relative link and breaks the SVG reference.

Adding { preprocess: false } to the stylesheet reference in the THEME.libraries.yml file DOES work, but { minified: true } DOES NOT.

I've just run into this on core 10.0.7.

Turning off the aggregation stops the problem, but is definitely a workaround, not a fix.

The original suggestion of testing the path to see if it starts with `#` seems like a sensible idea. I can't think of any reason for that not working? Can anyone smarter than me think of any reason that would fall over?

The problem also exists on core 10.1.x-dev.

The workaround described here, switching off aggregation, still solves the problem.

As a bug can we get a test case showing this problem?

I just can see the issue on our website that some "parts" of SVGs embedded in CSS are missing. The patch in #7 fixes this issue and the SVGs are "completely visible" again.

I can't tell if the fix suggested in the issue description here is correct for all use-cases, but it fixes our issue. So I shared the patch here as a beginning.

I would leave the addition of tests to someone who has better knowledge about SVG and who can judge if the suggested patch might cause other issues instead of just solving the clip-path issue.

Hi @smustgrave, in regards to

As a bug can we get a test case showing this problem?

Below is the test case shows the problem.

karthik.dk@drupal_contribute % git diff core
diff --git a/core/lib/Drupal/Core/Asset/CssOptimizer.php b/core/lib/Drupal/Core/Asset/CssOptimizer.php
index bc2aef7238..413106cf98 100644
--- a/core/lib/Drupal/Core/Asset/CssOptimizer.php
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -90,8 +90,8 @@ protected function processFile($css_asset) {
     // Store base path.
     $this->rewriteFileURIBasePath = $css_base_path . '/';

-    // Anchor all paths in the CSS with its base URL, ignoring external and absolute paths.
-    return preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+)([^\'")]+)[\'"]?\s*\)/i', [$this, 'rewriteFileURI'], $contents);
+    // Anchor all paths in the CSS with its base URL, ignoring external and absolute paths and paths starting with '#'.
+    return preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+|#|%23)([^\'")]+)[\'"]?\s*\)/i', [$this, 'rewriteFileURI'], $contents);
   }

   /**
diff --git a/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
index 2c49928416..347759527f 100644
--- a/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
+++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
@@ -222,6 +222,18 @@ public function providerTestOptimize() {
         ],
         file_get_contents($absolute_path . 'quotes.css.optimized.css'),
       ],
+      [
+        [
+          'group' => -100,
+          'type' => 'file',
+          'weight' => 0.013,
+          'media' => 'all',
+          'preprocess' => TRUE,
+          'data' => $path . 'import3.css',
+          'basename' => 'import3.css',
+        ],
+        file_get_contents($absolute_path . 'import3.css.optimized.css'),
+      ],
     ];
   }

karthik.dk@drupal_contribute % ddev exec phpunit -c core core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
PHP Warning:  file_get_contents(/var/www/html/core/tests/Drupal/Tests/Core/Asset/css_test_files/import3.css.optimized.css): Failed to open stream: No such file or directory in /var/www/html/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php on line 235

Warning: file_get_contents(/var/www/html/core/tests/Drupal/Tests/Core/Asset/css_test_files/import3.css.optimized.css): Failed to open stream: No such file or directory in /var/www/html/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php on line 235
PHPUnit 9.6.10 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Asset\CssOptimizerUnitTest
............F..                                                   15 / 15 (100%)

Time: 00:00.518, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\Core\Asset\CssOptimizerUnitTest::testOptimize with data set #12 (array(-100, 'file', 0.013, 'all', true, 'core/tests/Drupal/Tests/Core/...t3.css', 'import3.css'), false)
Group of file CSS assets optimized correctly.
Failed asserting that 'div{clip-path:url('#clip-cloud')}\n
' matches expected false.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php:255
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

FAILURES!
Tests: 15, Assertions: 17, Failures: 1.
Failed to execute command phpunit -c core core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php: exit status 1
karthik.dk@drupal_contribute %

Here's code within import3.css

div {
  clip-path: url('#clip-cloud')
}

Thanks :)

Can we add the test case to the patch.

Hi @smustgrave, in regards to

Can we add the test case to the patch.

Attached the patch file with test case & inter diff changes.

Log for reference:

karthik.dk@drupal_contribute % ddev exec phpunit -c core core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
PHPUnit 9.6.10 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Asset\CssOptimizerUnitTest
...............                                                   15 / 15 (100%)

Time: 00:00.521, Memory: 4.00 MB

OK (15 tests, 17 assertions)
karthik.dk@drupal_contribute %

Thanks :)

Thanks but you don't need to post the output of the test passing, that's shown by the patch.

Typically uploading a test-only patch would be good though.

But ran the test locally. and got
Failed asserting that two strings are equal.
Expected: 'div{clip-path:url('#clip-cloud');}\n
Actual: 'div{clip-path:url(generated-relative-url:core/tests/Drupal/Tests/Core/Asset/css_test_files/#clip-cloud);}\n

So the test appears to be covering

Good job!

Do we have or need coverage for when the URL is relative but contains a #?

Hi @catch I see, we don't need coverage for this scenario.

I have checked locally with below scenario

div {
  clip-path: url('#clip-cloud');
}
div {
  clip-path: url('/abc/#clip-cloud');
}

div{clip-path:url('#clip-cloud');}div{clip-path:url('/abc/#clip-cloud');}

I have noticed, bug described in the issue summary will not show up with the case of relative url, like clip-path: url('/abc/#clip-cloud');
So, we don't need coverage for this scenario.

@heykarthikwithu it's not the question of whether there's a bug now, it's adding test coverage to show that there isn't and preventing one being introduced in the future. I think that just adding an extra bit of CSS to the test file and assertion would be enough.

@catch,

I think that just adding an extra bit of CSS to the test file and assertion would be enough.

sure.. here's the patch.

Yeah that looks good!

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

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