Repository navigation
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
@dhrupo Thanks for the PR! Lets remove the query parameter additions entirely, see https://core.trac.wordpress.org/ticket/66639#comment:6 |
wp_audio_shortcode(), wp_video_shortcode(), and the media modal's prepareSrc() appended an auto-incrementing `&_=N` argument to every media source URL, working around MediaElement.js conflicts when several players in the DOM share one src. Mutating every URL breaks signed URLs from CDNs, and the conflict this worked around no longer applies now that the Flash fallback has been removed. Drop the argument entirely, and with it the prepareSrc() helper. Fixes #66639.
853d61a to
53a6770
Compare
|
Thanks for the direction @adamsilverstein — revised to remove the query parameter additions entirely:
Re-tested the original #26779 scenario live on the 7.2-beta4 nightly: two PHPUnit media shortcode tests pass (12 tests, 42 assertions); the one expected string updated in |
| audio.show(); | ||
| } | ||
| this.media = MediaDetails.prepareSrc( audio.get(0) ); | ||
| this.media = audio.get(0); |
There was a problem hiding this comment.
I guess audio files were also affected!
There was a problem hiding this comment.
Yes — same story on the audio side. prepareSrc() was also mutating the audio sources in the media modal (this file was one of its three callers), and wp_audio_shortcode() appended the _= argument too. The removal covers both, so signed audio URLs stay intact as well. The latest commit (53a6770) is the full removal you asked for — no query parameter additions left anywhere.
|
Thanks @dhrupo - adding a couple of additional reviewers here that are familiar with media in case I missed something. |
|
Hey @dhrupo, thanks for the quick update! Before committing I wanted to check whether anything in the plugin directory calls I had Claude search the plugin and theme directories for callers:
Can you add the no-op back? I'll update the proposed commit message to match once that is in. |
Third-party plugins call wp.media.view.MediaDetails.prepareSrc() directly and pass the return value to MediaElement.js, so removing the method throws a TypeError for them. Return the element unchanged instead; the cache-busting behavior stays removed. See #66639.
|
Good catch, thanks for checking Veloria — added back in c1d3c59: |
Trac ticket: https://core.trac.wordpress.org/ticket/66639
Summary
wp_audio_shortcode(),wp_video_shortcode(), and the media modal'sMediaDetails.prepareSrc()append an auto-incrementing&_=Nargument to every media source URL. The workaround dates from the WP 3.9 MediaElement.js integration and, as researched on the ticket, no longer applies now that the Flash fallback has been removed. Mutating every URL unconditionally breaks sources whose URL must stay intact, most notably signed URLs from CDNs.Fix
Per the maintainer's review, remove the query-parameter additions entirely:
add_query_arg( '_', $instance, ... )fromwp_audio_shortcode()andwp_video_shortcode().MediaDetails.instances/prepareSrc()from the media modal and update its three callers (audio details, video details, attachment details) to pass the element straight to MediaElement.js.Testing
Environment: WordPress 7.2-beta4-65231-src nightly, local Docker environment, Chrome. Reproduced the original scenario from #26779 live: two
[video]and two[audio]shortcodes, each pair sharing one self-hosted src,preload="metadata", on one page....small-video.mp4?_=1and?_=2....small-video.mp4, no&_=N), and all four players still play — each advanced ~1.7s in a 1.8s play window driven through the MediaElement.js API.prepareSrc()callers) still render and play, with no console errors.vendor/bin/phpunit --filter "test_wp_audio_shortcode|test_video_shortcode_body|test_wp_video_shortcode" tests/phpunit/tests/media.php→ OK (12 tests, 42 assertions). The one expected-string change is intest_video_shortcode_body().Use of AI Tools
AI assistance: Kimi (Moonshot AI) drafted the patch and the scripted reproduction; I reviewed every change, reproduced the issue on trunk, and ran and verified all results myself.
Proposed commit message
Claude reworked this commit message to lead with the problem: