Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

system/sef plugin background image parser regexp bug #11412

Closed
wants to merge 1 commit into from
Closed

system/sef plugin background image parser regexp bug #11412

wants to merge 1 commit into from

Conversation

brianteeman
Copy link
Contributor

Pull Request for Issue #7267 .

Bug

The problem is that using images in the style attributes for background images and the image urls with a full url:

<div style="background-image: url(&#039;http://test.org/test.png&#039;)"></div>

The regexp is in plugins/system/sef/sef.php which used to find image urls in style attributes to fix them to the current site's relative path:

Test:

<div style="background-image: url(&#039;http://test.org/test.png&#039;)"></div>

Result:

<div style="background-image: url("/my/site/path/&#039;http://test.org/test.png&#039;")"></div>

Desired result: - (Regexp do not match this url.)

<div style="background-image: url(&#039;http://test.org/test.png&#039;)"></div>

@andrepereiradasilva
Copy link
Contributor

brian isn't this the same as #11266 ?

@brianteeman
Copy link
Contributor Author

No its different as this addresses the use case of &#39 but probably should be added to that PR by @ggppdk and then I can close this

@ggppdk
Copy link
Contributor

ggppdk commented Aug 3, 2016

Actually we need to exclude:
Single quotes

&#039;    &#39;

Double quotes

&#034;   &#34;

Also my initial suggestion does not work right for all cases

Thus yes this should be excluded

<div style="background-image: url(&#039;http://test.org/test.png&#039;)"></div>

But this should be included

<div style="background-image: url(&#039;images/test.png&#039;)"></div>

The check needs to go earilier at regexp at quotes checking:

[\'\"]

should be

([\'\"]|\&\#0?34;|\&\#0?39;)

@brianteeman
I ll test that it works for desired cases and commit to other PR ?

  • also another reason for adding this to the other PR, is that the other PR will output the original type of quotes:
'
"
&#039;
&#39;
&#034;
&#34;

@brianteeman
Copy link
Contributor Author

When you update your PR I will close this - thanks

On 3 August 2016 at 14:46, Georgios Papadakis [email protected]
wrote:

Actually we need to exclude:
Single quotes

' '

Double quotes

" "

Also my initial suggestion does not work right for all cases

Thus yes this should be excluded

But this should be included

The check needs to go earilier at regexp at quotes checking:

['"]

should be

(['"]|&#0?34;|&#0?39;)

@brianteeman https://github.com/brianteeman
I ll test that it works for desired cases and commit to other PR ?

  • also another reason for adding this to the other PR, is that the
    other PR will output the original type of quotes:

'
"
'
'
"
"


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11412 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8cZHSY1GMSQUyz34VzgRBCc4QkxMks5qcJu6gaJpZM4JboBn
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@ggppdk
Copy link
Contributor

ggppdk commented Aug 3, 2016

@brianteeman

i have updated the other PR to add cases like:

style='background-image:url(&#34;...&#34;);'
style="background-image:url(&#39;...&#39;);"
style='background-image:url(&#034;...&#034;);'
style="background-image:url(&#039;...&#039;);"

So you can close this PR, i tried to make some easier and more complete testing instructions too

@brianteeman
Copy link
Contributor Author

Thanks very much - closing in favour of #11266

@brianteeman brianteeman closed this Aug 3, 2016
@brianteeman brianteeman deleted the regex branch August 4, 2016 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants