#64372 closed defect (bug) (fixed)
Additional URL args supplied via enqueued script/style handle cause malformed ver query param when null
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 7.0 | Priority: | low |
| Severity: | normal | Version: | 2.6 |
| Component: | Script Loader | Keywords: | has-patch has-unit-tests |
| Focuses: | Cc: |
Description (last modified by )
When working on fixing PHPStan issues (#64238) for WP_Scripts (r61358), I stumbled across a capability I was previously unaware of: you can pass additional query parameters to an enqueued script/style URL by adding them to the handle. For example:
<?php wp_enqueue_script( 'foo?bar=baz&baz=quux', 'https://example.com/test.js', array(), '1.0' );
This results in the following script being printed:
<script type="text/javascript" src="https://example.com/test.js?ver=1.0&bar=baz&baz=quux" id="foo-js"></script
This capability is not included in any unit test, and it may be a vestige of something no longer used. Naturally, a better way to do this now would seem to be to leverage the script_loader_src filter, or rather, to add the query parameters to the URL being registered in the first place. I can find one plugin (Map Navigator) which seems to be using this intentionally.
In any case, I found that this capability is broken when using null as a version:
<?php wp_enqueue_script( 'foo?bar=baz&baz=quux', 'https://example.com/test.js', array(), null );
This results in:
<script type="text/javascript" src="https://example.com/test.js?ver=bar=baz&baz=quux" id="foo-js"></script>
Note the query string: ver=bar=baz&baz=quux.
The "ver=" shouldn't have been included here.
Change History (15)
This ticket was mentioned in PR #10608 on WordPress/wordpress-develop by @westonruter.
6 months ago
#1
- Keywords has-patch has-unit-tests added
@westonruter commented on PR #10608:
6 months ago
#4
@peterwilsoncc I added your example code as a test case in 624ea26959281a29bbb0f4f475dc3b4b49521ee2.
I then added ba88780 to ensure this PR outputs the same markup, except for what is supposed to be fixed of course.
nichita231 commented on PR #10608:
3 months ago
#6
Hey @westonruter
I'm facing small problem with this fix
so, here's the link to google fonts
https://fonts.googleapis.com/css2?family=Figtree:wght@400;500;600;700&family=Montserrat:wght@700&display=swap
and when I'm trying towp_enqueue_style with current version your fix removes doubled url params ( family in our case )
what do you suggest?
I can use style_loader_src filter to use original URL in my case, but would like it to work out of the box
@westonruter commented on PR #10608:
3 months ago
#7
@nichita231 Thanks for the ping. Would you mind sharing the PHP code you're using just to be clear so I can reproduce exactly your setup? (I think I know, but I just want to be sure.)
nichita231 commented on PR #10608:
3 months ago
#8
@westonruter
here the code, thanks
wp_register_style('fonts-source-google', 'https://fonts.googleapis.com/css2?family=Figtree:wght@400;500;600;700&family=Montserrat:wght@700&display=swap', [], null);
wp_enqueue_style('fonts-source-google');
This ticket was mentioned in PR #11164 on WordPress/wordpress-develop by @westonruter.
3 months ago
#9
Previously, using 'add_query_arg()' to append versions or handle-specific query arguments would strip any existing duplicate query variables in the source URL (common in Google Fonts URLs).
This change refactors 'WP_Styles::_css_href()' and 'WP_Scripts::do_item()' to manually append these parameters to the URL string. This ensures all original query variables are preserved exactly as provided. It also improves fragment handling by ensuring query parameters are inserted before any '#' anchor while maintaining the anchor's presence.
Regression tests are added to verify preservation of duplicate query variables and fragments using 'WP_HTML_Tag_Processor' for precise attribute verification.
The URL encoding changes in tests/phpunit/tests/dependencies/scripts.php are reversions of what had previously been done in https://github.com/WordPress/wordpress-develop/commit/c4d8047c965eb14232052e5f1ca8959ed3f34b2e.
Follow-up to r61397.
Props @nichita231.
See #64372.
Trac ticket: https://core.trac.wordpress.org/ticket/64372
## Use of AI Tools
I iterated with Gemini CLI on adding tests to reproduce the issue, and then to iterated back and forth on changes. Then I had Gemini draft a commit message and commit.
<details><summary>Prompts</summary>
- Initial instruction regarding
add_query_arg()eliminating repeated query vars inWP_StylesandWP_Scripts, with a request for a better solution and unit tests. - Don't add a new test file. Just add to the existing files which were previously modified in c4d8047c965eb14232052e5f1ca8959ed3f34b2e. Add the new tests in the appropriate spot, probably right after the tests added in that commit to those two files
- You can supply --group=64372 to just one call to npm run test:php
- As opposed to using assertStringContainsString, it would be better to assert on the entire URL being the same. Let's provide 'https://example.com/' URLs for each script and style and add assetions to assertSame for the entire string
- Let's actually compare URLs. Extract the href/src from the tag via the WP_HTML_Tag_Processor and then compare the attribute value with the expected URL
- Let's also add assertTrue() checks to the next_tag calls
- ok, now you provided an abbreviated "$frag" variable. Let's avoid this abbreviation. use $fragment
- None of the tests provide any URLs with fragments
- Wouldn't it make sense to skip setting $fragment to an empy string if strstr() returns false, and then to only append $fragment onto $src if it is a string?
- for the tests, the phpdoc descriptions should all use 3rd singular person verbs to start (e.g. "Tests")
- I see there are failures now when I run
pm run test:php -- --group=scripts - I think you should rather change the URLs back to what they had been beforec c4d8047c965eb14232052e5f1ca8959ed3f34b2e. YOu can see that I had changed the URLs to deal with the encoding change
- That doesn't look relevant. I mean literally to undo the fixture changes I had made in c4d8047c965eb14232052e5f1ca8959ed3f34b2e.
- Your sed commands keep failing.
- ok, now based on all of the changes we've made together, and the changes which are staged in git, write a commit message and commit
</details>
@westonruter commented on PR #10608:
3 months ago
#11
@nichita231 It's very good that you reported this, as there are quite a few themes which would also have this issue: https://wpdirectory.net/search/01KJXS8RRHSDRJV7H0KT6VFZR0
And plugins too: https://wpdirectory.net/search/01KJXSAX6A4YB48GMA58JTDK0Z
Please test this fix: https://github.com/WordPress/wordpress-develop/pull/11164
nichita231 commented on PR #10608:
3 months ago
#12
hey @westonruter
works for me, thanks
@westonruter commented on PR #11164:
3 months ago
#13
Fix tested successfully: https://github.com/WordPress/wordpress-develop/pull/10608#issuecomment-4003370382
@westonruter commented on PR #10608:
3 months ago
#15
@nichita231 Thank you. I've committed this in r61927 (b8b74d5). I couldn't find your WordPress username, so I didn't include you in the props for the commit message. However, if you have one please let me know and I'll attach you to the commit so you get credit.
Trac ticket: https://core.trac.wordpress.org/ticket/64372
This pull request was based off of https://github.com/WordPress/wordpress-develop/pull/10607