A site using repsonsive images (via the core "responsive_image" module) with multiplers is producing markup like this (via the core "media" module):

<source srcset="/sites/default/files/styles/ucbweb_media_width_400/public/stock-photos/CampanileWindow_1200_KeeganHouser.jpg?itok=tm8WkdEz 1x, /sites/default/files/styles/ucbweb_media_width_800/public/stock-photos/CampanileWindow_1200_KeeganHouser.jpg?itok=RvM2BU8N 2x" media="(min-width: 576px) and (max-width: 767px)" type="image/jpeg">

Notice how the "srcset" has two URLs with multipliers, joined by a comma.

After being processed by the Pathologic input filter, it looks like this:

<source srcset="http://ucbweb.lndo.site/sites/default/files/styles/ucbweb_media_width_400/public/stock-photos/CampanileWindow_1200_KeeganHouser.jpg?itok=tm8WkdEz%201x%2C%20/sites/default/files/styles/ucbweb_media_width_800/public/stock-photos/CampanileWindow_1200_KeeganHouser.jpg%3Fitok%3DRvM2BU8N%202x" media="(min-width: 576px) and (max-width: 767px)" type="image/jpeg">

It correctly put the full path on the first URL, but everything else after that got URL encoded, including the multipliers, spaces and comma - as if they were all part of the first URL.

The end result is a broken image!

If Pathologic is going to process "srcset", it should only modify the URLs, and not touch anything else in there.

Issue fork pathologic-3354345

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dsnopek created an issue. See original summary.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Issue summary: View changes

edwtorba made their first commit to this issue’s fork.

edwtorba’s picture

edwtorba commented
Version: 8.x-1.x-dev » 2.0.x-dev
Priority: Normal » Major

I'm confirming that this issue is still happening on version 2.

edwtorba changed the visibility of the branch 3354345-pathologic-breaks-srcset to hidden.

edwtorba opened merge request !14

dww’s picture

dww
we/he/they
commented
Status: Active » Needs work
Issue tags: +Needs tests

Great, thanks! In principle, this sounds like it needs to happen and is heading in the right direction. No time for a super close review, but I see there are no new tests in the MR. I'm trying not to commit anything else here that doesn't have test coverage, tagging accordingly.

Thanks again, -Derek

prudloff made their first commit to this issue’s fork.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review
Issue tags: -Needs tests

The MR was splitting on XXXw but srcset also supports other syntax like 2x. (See the spec here: https://html.spec.whatwg.org/multipage/images.html#srcset-attribute)

I added tests for the different variants of the syntax.

I am not a huge fan of having to do str_replace('="', '', $fixed_path) but avoiding it would require refactoring _pathologic_replace_individual().

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Thanks for fixing this, and for adding tests! I tried the GitLab test-only job, and although it failed, the output is pretty useless due to trying to view this stuff on the web. 😂 But I did a test-only run locally, and saw that the test indeed fails as expected:

There was 1 failure:

1) Drupal\Tests\pathologic\Kernel\PathologicTest::testPathologic
srcset attribute with pixel density descriptors. Clean URLs: Yes protocol style full.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<source srcset="http://localhost/foo 1x, http://localhost/foo/bar 2x" />'
+'<source srcset="http://localhost/foo%201x%2C%20foo/bar%202x" />'

I don't have 100% confidence in our existing test coverage, but it's encouraging that nothing broke with this change. 😅 There are a couple of existing 'srcset' cases in tests/src/Kernel/PathologicTest.php and those are still passing. 🎉

Reviewing the actually changes... uhhh, yeah. 😂 This code is so cryptic, I'll be the first to admit it's not easy to modify or maintain. I appreciate https://git.drupalcode.org/project/pathologic/-/merge_requests/14/diffs?... since IMHO that helped make the resulting code more readable. I'm okay with doing this srcset check directly in _pathologic_replace() so we don't have to mess with the rest of what became _pathologic_replace_individual().

However, I did push a few commits to do some superficial cleanup of the code in _pathologic_replace(). The previous versions had a somewhat confusing mix of camelCaseVariables and snake_case. To match the rest of the code in pathologic.module I made everything snake_case. Also simplified a few things that had been split multi-line when they easily fit under 80 chars as 1 line. Finally, I added a comment about the special handling of srcset, inverted that if (), and do an early return of _pathologic_replace_individual(). That way, what remains in _pathologic_replace() is only the srcset code, and it doesn't have to be indented an extra 2 spaces. 🤓

The final pipeline is happy on all versions of core. It's passing phpstan level 5. All the other validation jobs are passing. I'm going to merge this.

Thanks again!

  • dww committed 49d3af92 on 2.0.x authored by prudloff 
    fix: #3354345 Pathologic breaks srcset when used with multipliers

Co-...

dww closed merge request !14

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.