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

Check for JUri::base in each srcset url before converting #17978

Merged
merged 6 commits into from Sep 20, 2017

Conversation

@ryandemmer
Copy link
Contributor

@ryandemmer ryandemmer commented Sep 18, 2017

With reference to - #15300 (comment), this PR updates the srcset URL conversion to check for the base url before performing the replacement.

Summary of Changes

As pointed out by @wronax - #15300 (comment) - if the srcset URL already contained the site base url, then the conversion would add the base url again. This change checks for the base url on each of the srcset urls before performing the replacement, if necessary.

Testing Instructions

As before, create a new article using the following HTML code, or similar. You will need 3 images of different resolutions, or just three different images of any resolution.

<img src="images/image1.jpg" srcset="images/image3.jpg 2x,images/image2.jpg 1.5x,images/image1.jpg 1x" alt="" />

Expected result

The image displays correctly in the browser as all the srcset values are converted.

Actual result

With the current sef.php the images will not display correctly in a modern browser. You can check the code using the browser console to see that the srcset values have not all been converted.

Documentation Changes Required

None

With reference to - #15300 (comment), this PR updates the srcset URL conversion to check for the base url before performing the replacement.

## Summary of Changes

As pointed out by @wronax - #15300 (comment) - if the srcset URL already contained the site base url, then the conversion would add the base url again. This change checks for the base url on each of the srcset urls before performing the replacement, if necessary.

## Testing Instructions

As before, create a new article using the following HTML code, or similar. You will need 3 images of different resolutions, or just three different images of any resolution.

```html
<img src="images/image1.jpg" srcset="images/image3.jpg 2x,images/image2.jpg 1.5x,images/image1.jpg 1x" alt="" />
```

## Expected result

The image displays correctly in the browser as all the srcset values are converted.

## Actual result

With the current sef.php the images will not display correctly in a modern browser. You can check the code using the browser console to see that the srcset values have not all been converted.

## Documentation Changes Required

None
Ryan Demmer added 2 commits Sep 18, 2017
Ryan Demmer
The regex is updated to allow each srcset url to contain spaces in the url, and to not contain a device pixel ratio.
The if condition has been updated to check for the position of the base uri in the srcset url, rather than just for the existence of the base uri in the string. This is because it is possible that the base uri string could be repeated somewhere in the url, so the check is only done to see if the base uri is at the beginning of the srcset url.
@ufuk-avcu
Copy link

@ufuk-avcu ufuk-avcu commented Sep 19, 2017

I tested this with Joomla 3.8.0 Stable and Yootheme Pro 1.9.6. The problem is now a little bit different:

Output:
<img src="/templates/yootheme/cache/layla-a5dcdd1c.jpg" srcset="/templates/yootheme/cache/layla-644e1610.jpg 72w,/ /templates/yootheme/cache/layla-32b06753.jpg 180w,/ /templates/yootheme/cache/layla-a5dcdd1c.jpg 90w" sizes="(min-width: 90px) 90px, 100vw" class="el-image uk-border-circle uk-box-shadow-large" alt="">

Now we have this: ,/ /

Original Problem: #17919

@steffans
Copy link

@steffans steffans commented Sep 19, 2017

@ryandemmer I just checked this issue and there is no need to check for the base url as proposed in the pull request. It actually happens because the regex which checks for an absolute path is wrong in v3.8.0:

$data[] = preg_replace('#(?!/|' . $protocols . '|\#|\')([^\s]+)\s+(.*)#', $base . '$1 $2', $url);

It needs to start with a ^ to match the start of the string, see proper regex below:

$data[] = preg_replace('#^(?!/|' . $protocols . '|\#|\')([^\s]+)\s+(.*)#', $base . '$1 $2', $url);
@ryandemmer
Copy link
Contributor Author

@ryandemmer ryandemmer commented Sep 19, 2017

@steffans I have made the regular expression change as recommended, removing the uri check.

@admiralsmaster
Copy link

@admiralsmaster admiralsmaster commented Sep 19, 2017

I test your latest changed line and I get a ",/ " too.

srcset="/images/thumbnails2/images/stories/abc/2011/xyz/xyz.jpg 1.5x,/ /images/thumbnails2/images/stories/abc/2011/xyz/xyz.jpg 2x"

@steffans
Copy link

@steffans steffans commented Sep 20, 2017

@ryandemmer With your change you have to make sure to trim the whitespace from the $url see:

$data[] = preg_replace('#^(?!/|' . $protocols . '|\#|\')(.+)#', $base . '$1', trim($url));
@ufuk-avcu
Copy link

@ufuk-avcu ufuk-avcu commented Sep 20, 2017

I have tested this item successfully on f599bf2

Tested it with Joomla 3.8.0 Stable and Yootheme Pro 1.9.7 and now it's ok.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17978.

@admiralsmaster
Copy link

@admiralsmaster admiralsmaster commented Sep 20, 2017

I have tested this item successfully on f599bf2


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17978.

@ghost
Copy link

@ghost ghost commented Sep 20, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Sep 20, 2017
@zero-24 zero-24 added this to the Joomla 3.8.1 milestone Sep 20, 2017
@wilsonge wilsonge merged commit ccebfb7 into joomla:staging Sep 20, 2017
5 checks passed
5 checks passed
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@joomla-cms-bot joomla-cms-bot removed the RTC label Sep 20, 2017
@ryandemmer ryandemmer deleted the ryandemmer:patch-1 branch Sep 25, 2017
@OlegKi
Copy link

@OlegKi OlegKi commented Oct 7, 2017

@ryandemmer: trimming of URLs produce another problem in case of usage srcset like "images/image1.jpg, images/image2.jpg 1.5x, images/image3.jpg 2x" instead of "images/image3.jpg 2x,images/image2.jpg 1.5x,images/image1.jpg 1x". I described the problem in details in the issue.

After applying the patch "images/image1.jpg, images/image2.jpg 1.5x, images/image3.jpg 2x" will be modified to "images/image1.jpg,images/image2.jpg 1.5x,images/image3.jpg 2x" and the URL of 1x DPR ("1x" is skipped correspond to specification) will be merged with the second one. As the result, the web browser will try to load picture from wrong URL images/image1.jpg,images/image2.jpg.

I remind, that URLs could contains comma character. The only restriction of URL used in srcset is that comma could not be the last character of the URL. Thus the syntax "images/image1.jpg,images/image2.jpg 1.5x,images/image3.jpg 2x" have to be interpreted as two URLs instead of three URLs. By the way, Cloudinary CDN uses commas inside of URLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.