? ? Pending

User tests: Successful: Unsuccessful:

avatar ryandemmer
ryandemmer
18 Sep 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

Votes

# of Users Experiencing Issue
2/2
Average Importance Score
4.50

avatar joomla-cms-bot joomla-cms-bot - change - 18 Sep 2017
Category Front End Plugins
avatar ryandemmer ryandemmer - open - 18 Sep 2017
avatar ryandemmer ryandemmer - change - 18 Sep 2017
Status New Pending
avatar ryandemmer ryandemmer - change - 18 Sep 2017
Labels Added: ?
avatar ufuk-avcu
ufuk-avcu - comment - 19 Sep 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

avatar steffans
steffans - comment - 19 Sep 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);
avatar ryandemmer
ryandemmer - comment - 19 Sep 2017

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

avatar admiralsmaster
admiralsmaster - comment - 19 Sep 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"

avatar steffans
steffans - comment - 20 Sep 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));
avatar ufuk-avcu ufuk-avcu - test_item - 20 Sep 2017 - Tested successfully
avatar ufuk-avcu
ufuk-avcu - comment - 20 Sep 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.

avatar admiralsmaster admiralsmaster - test_item - 20 Sep 2017 - Tested successfully
avatar admiralsmaster
admiralsmaster - comment - 20 Sep 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.

avatar franz-wohlkoenig franz-wohlkoenig - change - 20 Sep 2017
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 20 Sep 2017

RTC after two successful tests.

avatar wilsonge wilsonge - change - 20 Sep 2017
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-09-20 18:52:34
Closed_By wilsonge
Labels Added: ?
avatar wilsonge wilsonge - close - 20 Sep 2017
avatar wilsonge wilsonge - merge - 20 Sep 2017
avatar OlegKi
OlegKi - comment - 7 Oct 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.

Add a Comment

Login with GitHub to post a comment