CSS Aggregation should not rewrite # url

Created on 27 October 2022, about 2 years ago
Updated 1 August 2023, over 1 year ago

Problem/Motivation

I was working now with css clip-paths and it all works well, untill it goes to production. In the combined css `clip-path: url('#clip-cloud')` is then rewritten to `clip-path: url('/themes/my-theme/css/#clip-cloud')`, which breaks the svg reference.

Steps to reproduce

Use any theme css file with a svg id refference: `clip-path: url('#clip-cloud')`

Proposed resolution

I would say, best would be, if I could mark some parts of a css file to not be rewritten... Alternatively, `url(#whatever)` should NOT be rewritten. It's probably safe, but might lead in cases i don't know to an error.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Fixed

Version

10.1

Component
CSS 

Last updated about 6 hours ago

Created by

🇩🇪Germany Schoenef Unna

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States brobert7

    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.

  • 🇬🇧United Kingdom DanLew Birmingham

    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?

  • 🇩🇪Germany danielehrenhofer Frankfurt

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

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

  • Status changed to Needs review over 1 year ago
  • last update over 1 year ago
    29,446 pass
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

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

  • 🇩🇪Germany mkalkbrenner 🇩🇪

    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.

  • Status changed to Needs review over 1 year ago
  • 🇮🇳India heykarthikwithu Bengaluru 🌍

    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 :)

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    Can we add the test case to the patch.

  • Status changed to Needs review over 1 year ago
  • last update over 1 year ago
    29,451 pass
  • 🇮🇳India heykarthikwithu Bengaluru 🌍

    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 :)

  • last update over 1 year ago
    29,882 pass
  • Status changed to RTBC over 1 year ago
  • 🇺🇸United States smustgrave

    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!

  • last update over 1 year ago
    29,455 pass
  • Status changed to Needs review over 1 year ago
  • 🇬🇧United Kingdom catch

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

  • 🇮🇳India heykarthikwithu Bengaluru 🌍

    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.

  • 🇬🇧United Kingdom catch

    @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.

  • last update over 1 year ago
    29,456 pass
  • 🇮🇳India heykarthikwithu Bengaluru 🌍

    @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.

  • Status changed to RTBC over 1 year ago
  • 🇬🇧United Kingdom catch

    Yeah that looks good!

    • catch committed 74fcd731 on 10.1.x
      Issue #3317745 by heykarthikwithu, mkalkbrenner, catch, smustgrave,...
  • Status changed to Fixed over 1 year ago
    • catch committed 5f07e3ce on 11.x
      Issue #3317745 by heykarthikwithu, mkalkbrenner, catch, smustgrave,...
  • 🇬🇧United Kingdom catch

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

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

Production build 0.71.5 2024