Example inside an svg that is imported in css as data-url:
<path fill="url(#SVGID_1_)" d="M1256.4,386h1500v21h-1500V386z"/>

Notice the "url(#SVGID_1_)". This will be converted to
url(http://domain.com/path/path/#SVGID_1_1)

Resulting in a broke svg.

(added an svg that has this problem)

CommentFileSizeAuthor
#47 2362643-47-fix-svg.patch11.97 KBmbrc
#41 2362643-41-fix-svg.patch12.1 KBPls
#37 2362643-37-fix-svg-D7.patch2.27 KBericclaeren
#30 interdiff-2362643-28-30.txt2.28 KBparkecag
#30 2362643-30-fix-svg.patch2.66 KBparkecag
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,840 pass(es). View
#28 2362643-28-fix-svg.patch2.62 KBparkecag
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,688 pass(es). View
#23 advagg-2362643-21-fix-svg.patch7.96 KBmikeytown2
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch advagg-2362643-21-fix-svg_0.patch. Unable to apply patch. See the log in the details link for more information. View
#18 drupal-2362643-18-fix-css-SVG-url-D7.patch2.23 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 41,117 pass(es). View
#17 drupal-2362643-17-fix-css-SVG-url-D8.patch2.46 KBmikeytown2
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,203 pass(es). View
#11 advagg-2362643-10-fix-url-svg.patch1.7 KBmikeytown2
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch advagg-2362643-10-fix-url-svg.patch. Unable to apply patch. See the log in the details link for more information. View
#2 gradient.svg_.zip862 bytesGijsRoge
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

GijsRoge’s picture

Issue summary: View changes
GijsRoge’s picture

Issue summary: View changes
FileSize
862 bytes
mikeytown2’s picture

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

GijsRoge’s picture

Yes, I noticed yesterday it doesn't have anything to do with advagg. (Icouldn't update my post anymore because of the anti-spam protection)

I'll move it.

GijsRoge’s picture

Title: Fix improperly set type on svg path » Drupal alter's svg fill path's with base url -> broken svg
Project: Advanced CSS/JS Aggregation » Drupal core
Version: 7.x-2.x-dev » 9.x-dev
Component: Code » ajax system
mikeytown2’s picture

Version: 9.x-dev » 7.x-dev
Component: ajax system » CSS

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.

GijsRoge’s picture

Alright, thats good news. Thanks!

mikeytown2’s picture

Project: Drupal core » Advanced Aggregator
Version: 7.x-dev »
Component: CSS » Code

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

mikeytown2’s picture

Project: Advanced Aggregator » Advanced CSS/JS Aggregation
Version: » 7.x-2.x-dev

  • mikeytown2 committed 5f5b5dd on
    Issue #2362643 by mikeytown2: Do not parse SVG urls in CSS for base path...
mikeytown2’s picture

Status: Active » Fixed
FileSize
1.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch advagg-2362643-10-fix-url-svg.patch. Unable to apply patch. See the log in the details link for more information. View

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

GijsRoge’s picture

Brilliant, thanks!

Issue is still present after applying the patch, rebuilding aggregates and clearing cache

output
<path xmlns="http://www.w3.org/2000/svg" fill="url(/sites/all/themes/elipse/css/#a)" d="M1256.4 386h1500v21h-1500v-21z"/>
Should be
<path xmlns="http://www.w3.org/2000/svg" fill="url(#a)" d="M1256.4 386h1500v21h-1500v-21z"/>

GijsRoge’s picture

Status: Fixed » Needs work
mikeytown2’s picture

Status: Needs work » Postponed (maintainer needs more info)

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"/>
GijsRoge’s picture

Its used inside a css file within data-url(''); (so perhaps you could just ignore everything inside a data url as they do not have to contain relative paths) wich is a more elegant way of fixing this problem perhaps.

And your new code output seems to be correct.

This is the result of the applied patch http://take.ms/5R0TC (seems to be correct, so its weird that it works for you but not for me)
I have the exact same use case as you described in the post above.

Is it possible that this fill="url('#SVGID_1_')" (notice the single quotes inside) causes the regex to fail?

mikeytown2’s picture

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.

mikeytown2’s picture

Project: Advanced CSS/JS Aggregation » Drupal core
Version: 7.x-2.x-dev » 8.0.x-dev
Component: Code » CSS
Status: Postponed (maintainer needs more info) » Needs review
FileSize
2.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,203 pass(es). View

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

mikeytown2’s picture

Version: 8.0.x-dev » 7.x-dev
FileSize
2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 41,117 pass(es). View

7.x patch

mikeytown2’s picture

Version: 7.x-dev » 8.0.x-dev

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.

GijsRoge’s picture

@Mikeytown2 Issue still exists after patch from post #18

Link to the full css file -> https://dl.dropboxusercontent.com/u/15198233/main.css ( line: 859 )

mikeytown2’s picture

Project: Drupal core » Advanced CSS/JS Aggregation
Version: 8.0.x-dev » 7.x-2.x-dev
Component: CSS » Code

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; }

  • mikeytown2 committed c84270d on 7.x-2.x
    Issue #2362643 by mikeytown2: Test SVG & fix for %23 in url().
    
mikeytown2’s picture

Project: Advanced CSS/JS Aggregation » Drupal core
Version: 7.x-2.x-dev » 8.0.x-dev
Component: Code » CSS
Status: Needs review » Needs work
FileSize
7.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch advagg-2362643-21-fix-svg_0.patch. Unable to apply patch. See the log in the details link for more information. View

Committed to AdvAgg. moving back to core.

Chernous_dn’s picture

Status: Needs work » Needs review

The last submitted patch, 11: advagg-2362643-10-fix-url-svg.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: advagg-2362643-21-fix-svg.patch, failed testing.

kevinquillen’s picture

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.

parkecag’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,688 pass(es). View

Posting a re-roll of patch from #17.

mdrummond’s picture

Status: Needs review » Needs work

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

parkecag’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,840 pass(es). View
2.28 KB

New patch that checks for %23 also.

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

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

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ericclaeren’s picture

Version: 8.1.x-dev » 7.x-dev

The patch in #2362643-18: Drupal alter's svg fill path's 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.

Wim Leers’s picture

Version: 7.x-dev » 8.1.x-dev

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

ericclaeren’s picture

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 alter's svg fill path's with base url -> broken svg

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Pls’s picture

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

Wim Leers’s picture

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

Pls’s picture

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

Pls’s picture

Status: Needs work » Needs review

The last submitted patch, 37: 2362643-37-fix-svg-D7.patch, failed testing.

Wim Leers’s picture

#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(….

Wim Leers’s picture

Status: Needs review » Needs work

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mbrc’s picture

FileSize
11.97 KB

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kala4ek’s picture

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.