Fixed filter failure on unescaped urls #123
Conversation
|
@fprochazka Alright I am satisfied, could you check it out? |
| $self = $this; | ||
|
|
||
| return preg_replace_callback($regexp, function ($matches) use ($self, $file) | ||
| $regexp = '/url\(([\'"])?(.*)([\'"])?\)/i'; |
There was a problem hiding this comment.
this will fail when the url contains a newline character, or when there are two url statements on the same line :-/
There was a problem hiding this comment.
I think it would be better to fix the original regex
There was a problem hiding this comment.
I am not that great with regexps and the original one looks like monstrosity, I like to do things the easy way, in this case I take anything in url() and work with in in PHP instead of doing everything in regexp.
|
It seems like you're solving three different issues at once, which is a bit confusing for me to review :-/ |
|
What do you mean by three different issues? |
| // is already absolute | ||
| if (preg_match('/^([a-z]+:\/)?\//', $url)) { | ||
| // Skip absolute and data uris | ||
| if (preg_match('/^([a-z]+:|\/)/i', $url)) { |
There was a problem hiding this comment.
this seems like a separate issue
There was a problem hiding this comment.
This is just extension, I removed data uri filtering from the regexp so I filter it here.
|
@fprochazka I've fixed the greedines but I can't remove the trim nor undo the modification of the condition. If you wan't I can split this code into several commits. |
| @@ -55,7 +55,7 @@ public function setBasePath($basePath) | |||
| public function absolutizeUrl($url, $quote, $cssFile) | |||
| { | |||
| // is already absolute | |||
| if (preg_match('/^([a-z]+:\/)?\//', $url)) { | |||
| if (preg_match('/^([a-z]+:|\/)/i', $url)) { | |||
There was a problem hiding this comment.
do you think you could add some more tests for this method, so it's obviouse what you were tryting to solve?
There was a problem hiding this comment.
What exactly would you want? Paths from all urls are passed into this match, if it is https? or data uri, it is returned as is, if not, the path is then parsed further.
There was a problem hiding this comment.
I would like some unit tests for this method only.
There was a problem hiding this comment.
I've added 2 assertions into testAbsolutizeAbsolutized, should be sufficient.
|
@fprochazka You were right, this pull is starting to get little big. Too bad there weren't more tests for this filter in the first place, this would have been much easier. |
|
@fprochazka ping, probably busy, right? |
|
@juniwalk I'm scared of the regexp changes, I'm gonna have to dive into it and verify it works as expected - which will take time :) |
|
@fprochazka I see, I hoped to cover this in the |
|
@fprochazka Hello, any news? I've spent last hour finding one issue only to discover that it is this issue with webloader again. |
|
Sorry, no time for this so far. |
|
I've redirected composer to my cloned copy so I use the changes I've made. It's okay for now but not ideal. |
|
@fprochazka Hello, still no time? Last commit was 7 months ago, looks like this repo is being "abandoned" :/ |
Related #97