[#17978] - Check for JUri::base in each srcset url before converting
- Fixed in Code Base
- 20 Sep 2017
- Medium
- Build: staging
- # 17978
- Diff
- ryandemmer:patch-1
User tests: Successful: Unsuccessful:
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
joomla-cms-bot
- | Category | ⇒ | Front End Plugins |
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
@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);
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"
@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));
I have tested this item
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.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17978.
| Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
wilsonge
- | 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:
?
|
||
@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.
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