Drupal alters svg fill paths with base url -> broken svg

Does this broken svg issue happen when advagg is disabled but core aggregation is enabled?

It is a core issue, but it cam be fixed in advagg. We'll need to fix in advagg, then 7.x so it can be tested then see if 8.x needs the fix and then fix it there so it can be committed in 7.x. Long story short it will be fixed in advagg before it's fixed in core.

Moving this back to AdvAgg; will then move to core once it's fixed here. Will also ping the CDN module as this will likely be an issue there as well.

Code in AdvAgg: _advagg_build_css_path()

Related issues
#1514182: IE @font-face CSS hack URL broken by CDN module's overridden CSS aggregation
#1961340: CSS aggregation breaks file URL rewriting because it abuses it

Committed this patch; I edited the regex so that URL's that start with # do not get altered.

Can I get the full context in how this svg is used. I was thinking it was inside a css file.

This is the test code with the assumption that url(#a) is inside a css file.

$contents = '<path xmlns="http://www.w3.org/2000/svg" fill="url(#a)" d="M1256.4 386h1500v21h-1500v-21z"/>';
$css_base_url = 'BASE-PATH';

module_load_include('inc', 'advagg', 'advagg');
_advagg_build_css_path(NULL, $css_base_url . '/');

// Anchor all paths in the CSS with its base URL, ignoring external,
// absolute paths, and urls that start with # (SVG).
$contents = preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+|\#+)([^\'")]+)[\'"]?\s*\)/i', '_advagg_build_css_path', $contents);
echo 'New code: ' . htmlentities($contents, TRUE) . "<br>\n";
$contents = preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+)([^\'")]+)[\'"]?\s*\)/i', '_advagg_build_css_path', $contents);
echo 'Old code: ' . htmlentities($contents, TRUE) . "<br>\n";

Output:

New code: <path xmlns="http://www.w3.org/2000/svg" fill="url(#a)" d="M1256.4 386h1500v21h-1500v-21z"/>
Old code: <path xmlns="http://www.w3.org/2000/svg" fill="url(//localhost/d7/BASE-PATH/#a)" d="M1256.4 386h1500v21h-1500v-21z"/>

Single/double or no quotes doesn't affect the regex matching from my testing. url('data: gets ignored because the regex skips all strings inside of url that start with {scheme}: ([a-z]+:), slashes (\/+) and now the number sign (\#+).

I still can't repo the bug since I committed the patch in #11. If you're worried about posting the full CSS code here that contains the SVG, you can use my contact info to point me toward the file so I can fix the bug.

Moving this to core.
Following patch should fix SVG issues in D8 css.

7.x patch

7.x & 8.0.x pass; moving to D8 for core inclusion.

@GijsRoge
Try applying the D7 patch in #18; that might fix it for you.

found it!

%23 is also #. Will need to adjust the regex to account for this and write a test as well.

/* line 53, /Applications/MAMP/htdocs/elipse/htdocs/sites/all/themes/elipse/scss/svg/icons.data.svg */
.icon-gradient, .frontbanner:after {
  background-image: url('data:image/svg+xml;charset=US-ASCII,%3C%3Fxml%20version%3D%221.0%22%20encoding%3D%22utf-8%22%3F%3E%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20width%3D%221500%22%20height%3D%2221%22%20viewBox%3D%22793.4%200%201500%2021%22%20enable-background%3D%22new%20793.4%200%201500%2021%22%3E%3Cg%20id%3D%22Layer_1_1_%22%3E%3ClinearGradient%20id%3D%22SVGID_1_%22%20gradientUnits%3D%22userSpaceOnUse%22%20x1%3D%225037.169%22%20y1%3D%2219.555%22%20x2%3D%225038.169%22%20y2%3D%2219.555%22%20gradientTransform%3D%22matrix(1500%200%200%20-538%20-7554960%2010531)%22%3E%3Cstop%20offset%3D%220%22%20stop-color%3D%22%23F5F8F8%22%2F%3E%3Cstop%20offset%3D%22.355%22%20stop-color%3D%22%23FAFBFB%22%20stop-opacity%3D%22.98%22%2F%3E%3Cstop%20offset%3D%22.833%22%20stop-color%3D%22%23FCFCFC%22%20stop-opacity%3D%220%22%2F%3E%3Cstop%20offset%3D%221%22%20stop-color%3D%22%23fff%22%20stop-opacity%3D%220%22%2F%3E%3C%2FlinearGradient%3E%3Cpath%20fill%3D%22url(%23SVGID_1_)%22%20d%3D%22M793.4%200h1500v21h-1500V0z%22%2F%3E%3C%2Fg%3E%3C%2Fsvg%3E');
  background-repeat: no-repeat; }

Committed to AdvAgg. moving back to core.

Can confirm this is a bug. Here is the CSS on an SVG element:

   url: (#diagonal-up);

Without core aggregation of CSS enabled, it works fine. When I enable it, it tacks a URL on to the beginning #diagonal-up, breaking it.

The patch in #17 worked for me.

Posting a re-roll of patch from #17.

We will need to update this patch to include the check for %23 that was noted in #21.

New patch that checks for %23 also.

Reviewed the patch and tested it, appears to work. Great work parkecag!

This needs test coverage, to prevent this from regressing again in the future.

The patch in #2362643-18: Drupal alters svg fill paths with base url -> broken svg didn't work for me, the updated D8 regex into D7 did, will try to create an updated patch asap.

Why did you change the version? This needs to be fixed in Drupal 8 first.

Excuse me, didn't do that on purpose. Thought it was for my reply only.

I've attached a patch for Drupal 7 based on the fix in #2362643-30: Drupal alters svg fill paths with base url -> broken svg

#30 patch works great for Drupal 8.2.3. Would love to push and get this in core, as this is pretty major for specific use cases, where svg is used.

@Wim Leers, I would love to create missing tests here. But I need more info how they should be properly created. Have not much experience with writing core tests really, but want to learn. Thanks :)

@Pls: Thanks for taking this on! Here's some guidance for you:

1. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -71,8 +71,9 @@ protected function processFile($css_asset) {
   -    // 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', array($this, 'rewriteFileURI'), $contents);
   +    // Anchor all paths in the CSS with its base URL, ignoring external,
   +    // absolute, and SVG (# or %23) paths.
   +    return preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+|\#|\%23+)([^\'")]+)[\'"]?\s*\)/i', array($this, 'rewriteFileURI'), $contents);
   
   @@ -176,9 +177,10 @@ protected function loadNestedFile($matches) {
   -    // Alter all internal url() paths. Leave external paths alone. We don't need
   -    // to normalize absolute paths here because that will be done later.
   -    return preg_replace('/url\(\s*([\'"]?)(?![a-z]+:|\/+)([^\'")]+)([\'"]?)\s*\)/i', 'url(\1' . $directory . '\2\3)', $file);
   +    // Alter all internal _url() paths. Leave external paths alone. We don't need
   +    // to normalize absolute paths here because that will be done later. Also
   +    // ignore SVG paths (# or %23).
   +    return preg_replace('/url\(\s*([\'"]?)(?![a-z]+:|\/+|\#|\%23+)([^\'")]+)([\'"]?)\s*\)/i', 'url(\1' . $directory . '\2\3)', $file);
   

   These changes can be tested by expanding the test coverage in \Drupal\Tests\Core\Asset\CssOptimizerUnitTest.

   Plenty of examples to look at.

2. +++ b/core/modules/color/color.module
   @@ -473,8 +473,9 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   -      // Prefix all paths within this CSS file, ignoring absolute paths.
   -      $style = preg_replace_callback('/url\([\'"]?(?![a-z]+:|\/+)([^\'")]+)[\'"]?\)/i', array($css_optimizer, 'rewriteFileURI'), $style);
   +      // Prefix all paths within this CSS file, ignoring external, absolute, and
   +      // SVG paths (# or %23).
   +      $style = preg_replace_callback('/url\([\'"]?(?![a-z]+:|\/+|\#|\%23+)([^\'")]+)[\'"]?\)/i', array($css_optimizer, 'rewriteFileURI'), $style);
   

   Testing this one is going to be a bit more difficult.

   But I think you should be able to expand the test coverage in \Drupal\Tests\color\Functional\ColorTest::_testColor() :)

@Wim Leers: Thanks for your pointers, really helped me starting up, appreciate your help!

Added patch with test dummy data added for CssOptimizerUnitTest. Let me know what you think.

Haven't figured out how to write tests for Color module. It looks like it compares whole stylesheet with test data from first look. Do you guys have any ideas or suggestions how to implement this better? :)

#41 looks great :)

Haven't figured out how to write tests for Color module. It looks like it compares whole stylesheet with test data from first look. Do you guys have any ideas or suggestions how to implement this better? :)

No, not without refactoring all of its test coverage. I'd just continue the existing way of testing: use $this->assertTrue(strpos(… or $this->assertPattern(….

Same patch as #41, but applies to 8.3. Still needs additional tests.

#47 works perfectly for me
:+1::skin-tone-2:

#47 is working great for me. What do we need to get this fix in core?

#47 fixed my issue!

Closed #2994490: CssOptimizer doesn't handle SVG clipPath urls properly as a duplicate issue.

Closed #2784107: SVG CSS marker-start: url(#id) properties break when aggregated as a duplicate issue.

Testing with Drupal 8.6.0 - #47 still works for me. How do we get this rolled into core?

Just checked, #47 perfectly applies to current 8.7.x.
According to the latest comments, mark it is rtbc.

1. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -60,8 +60,9 @@ protected function processFile($css_asset) {
   +    // absolute, and SVG (# or %23) paths.
   

   Is it an SVG path? I think we should use the language from CSS documentation. These appear to be fragment URLs. See https://drafts.csswg.org/css-values-3/#local-urls

   Going down that path I think we also have a few more bugs here because we should not prefix the idents inherit, initial and unset - let's open a follow-up for that.

2. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -60,8 +60,9 @@ protected function processFile($css_asset) {
   +    return preg_replace_callback('/url\(\s*[\'"]?(?![a-z]+:|\/+|\#|\%23+)([^\'")]+)[\'"]?\s*\)/i', [$this, 'rewriteFileURI'], $contents);
   
   @@ -166,8 +167,9 @@ protected function loadNestedFile($matches) {
   +    return preg_replace('/url\(\s*([\'"]?)(?![a-z]+:|\/+|\#|\%23+)([^\'")]+)([\'"]?)\s*\)/i', 'url(\1' . $directory . '\2\3)', $file);
   
   +++ b/core/modules/color/color.module
   @@ -488,8 +488,9 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   +      $style = preg_replace_callback('/url\([\'"]?(?![a-z]+:|\/+|\#|\%23+)([^\'")]+)[\'"]?\)/i', [$css_optimizer, 'rewriteFileURI'], $style);
   

   Why are we doing %23 as well? This is not tested. We need to add test coverage if this is necessary.

tested #47 successfully.
Thanks for the patch !

Why are we doing %23 as well? This is not tested. We need to add test coverage if this is necessary.

This is to catch any paths using the HTML encoded version of "#someID". It is tested in the .url .inline-svg definition with:

....url(%23SVGID_1_)....

It's a bit hard to see but it's in there ;)

This patch fixes #60.1. Also adding a test only patch.

Opened #3077821: [PP-1] Do not prefix the idents inherit, initial and unset during CSS optimisation as a follow up as per #60

The patch #63 works great. Thanks.

Can confirm this patch works to fix this issue.

Reviewing RTBC for Bug Smash Initiative, thus adding tag.

The first thing I notice is that the issue summary is an explanation of the problem and nothing about the proposed resolution. Of course, it is kind of implied, that we don't want to have broken svg. Still it would be helpful to include how one is going to fix the problem in the IS.

Reading the patch there are two references to this issue, neither with the usual format of 'See https...'. I wonder if pointing to this issue is necessary. Why not just expand the comment to say what it is doing to keep the url correct? Keep in mind, this is an area I know little about so I could be way off the track here.

1. +++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
   @@ -61,6 +61,8 @@ public function providerTestOptimize() {
   +      //   (https://www.drupal.org/project/drupal/issues/2362643)
   

   Should this be 'See https://www.drupal.org/project/drupal/issues/2362643' or an expanded comment?

2. +++ b/core/tests/Drupal/Tests/Core/Asset/css_test_files/import1.css
   @@ -18,3 +18,33 @@ ul, select {
   +/* Test urls with # or %23 character - https://www.drupal.org/node/2362643 */
   

   Again, same as above

Applying IS template

Thanks, the patch of #72 works great, even on 8.9.11.

There are some failures in the CI run for #72.

Fixing CI failures

Would it be possible to backport this patch to be compatible with D8? I have a number of sites with this issue and haven't yet made the leap to get the sites upgraded to D9 >< - even though I know it's supposed to be a small leap!

Works great all the comments are adressed

Thanks, still works great for me too!

Back in #40 Wim asked for tests for the color module changes, but they seemed to have slipped through, i.e. we're missing coverage for those changes.

Needs work for that

Patch re-roll via 9.3.x-dev, also works with 9.2.8.

PR created from #82

#82 works for me :)

Although this appears to fix references such as url(#someSVG);, I'm finding it is breaking inline SVGs by removing spaces.

Pre-patch:

data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='-4 -4 8 8'%3e%3ccircle r='2' fill='%23fff'/%3e%3c/svg%3e

Post-patch:

data:image/svg+xml,%3csvgxmlns='http://www.w3.org/2000/svg'viewBox='-4-488'%3e%3ccircler='2'fill='%23fff'/%3e%3c/svg%3e

nvm, was YUI compression on adv/agg: #3069151: Compatibility problem between YUI compressor and @supports CSS property

I can also confirm the latest #82 patch is working on latest 9.3.12 with both core and advagg aggregation enabled.

I was encountering the clip-path issue described in #2994490: CssOptimizer doesn't handle SVG clipPath urls properly, and the patch in #82 fixes that for me.

I was facing the issue with clip-path with the core version 9.4.5 and #82 fixes that for me.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The patch for #81 fails due to Color being removed from Core. The attached patch is a rewrite of #81 with the color.module patch removed. It works against the 10.0.10 branch; not tested against 10.1.x

hi, #96 doesn't seems apply on 10.1.x

Patch for 10.1.x that failed in #96.

Awesome, thank you! Patch is working on 10.1.x using php 8.1

I updated the patch to be compatible with Core 11.2.2+. Patch applied cleanly on my install.