Conversation
rhukster
added a commit
that referenced
this pull request
Sep 27, 2026
Member
|
Thanks @wakqasahmed, this is a really thorough one. Merged as is. On top I added a few tests for other things that leaked between repeated embeds of the same image and that your change fixes too: a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3567.
When the same image is embedded more than once in Markdown, every embed goes through the page's single shared medium object. Query params that aren't media actions (like
?foobar=asdasdor?preset=foo) are appended to that medium's querystring and never cleared, so the second and third embeds pick up everything the earlier ones added.I first tried clearing the querystring in
reset(), buturl()deliberately resets before adding the querystring, and there are existing tests (plus the older "Medium objects losing query string attributes" fix) that depend on it surviving a reset. So I left that alone. Instead, the Markdown excerpt now works on$medium->copy(), so each embed starts from the untouched original. For that copy to really be independent,Medium::__clone()now also clones the@2xalternatives (the querystring gets pushed onto them too, so otherwise the srcset still leaked), and it drops any cached thumbnail, which would otherwise still point at the original medium as its parent and bring the leak back forlightbox/linkembeds once the thumbnail had been built from Twig.One visible output change: an embed without image actions now always serves the original file, even if an earlier embed of the same image was cropped or resized. Before, it happened to be served from the
images/cache path, only because the shared medium still had the earlier embed's image open. I updated the second assertion intestProcessImageHtml, which relied on that behavior.This only covers the Markdown excerpt path. The public static
Helpers\Excerpts::processMediaActions()still operates on whatever medium its caller passes in, so callers of that helper aren't changed here.I added four tests to
ExcerptsTest: the exact case from the issue (same jpg embedded three times plus once with no query), the same with a@2xalternative to cover the srcset, the lightbox case with a thumbnail already cached from Twig, and an SVG to cover the non-ImageMedium path. They all fail on develop and pass with this change. I ran the Excerpts, Page/Medium, Page/Markdown, Markdown and Twig tests, plus the full unit suite, on PHP 8.4 with GD, and everything passes.