Make WordPress Core

Opened 6 months ago

Closed 3 months ago

Last modified 3 months ago

#64372 closed defect (bug) (fixed)

Additional URL args supplied via enqueued script/style handle cause malformed ver query param when null

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
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 westonruter)

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&amp;bar=baz&amp;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&amp;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

#2 @westonruter
6 months ago

  • Description modified (diff)

#3 @westonruter
6 months ago

  • Owner set to westonruter
  • Status changed from new to accepted

@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.

#5 @westonruter
6 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 61397:

Script Loader: Fix adding default version to script/style URL when args are supplied via enqueued handle.

Also fixes phpdoc for some member variables of WP_Scripts and WP_Styles.

Developed in https://github.com/WordPress/wordpress-develop/pull/10608

Follow-up to [61358].

Props westonruter, peterwilsoncc.
See #64224, #64238.
Fixes #64372.

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>

  1. Initial instruction regarding add_query_arg() eliminating repeated query vars in WP_Styles and WP_Scripts, with a request for a better solution and unit tests.
  2. 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
  3. You can supply --group=64372 to just one call to npm run test:php
  4. 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
  5. 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
  6. Let's also add assertTrue() checks to the next_tag calls
  7. ok, now you provided an abbreviated "$frag" variable. Let's avoid this abbreviation. use $fragment
  8. None of the tests provide any URLs with fragments
  9. 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?
  10. for the tests, the phpdoc descriptions should all use 3rd singular person verbs to start (e.g. "Tests")
  11. I see there are failures now when I run pm run test:php -- --group=scripts
  12. 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
  13. That doesn't look relevant. I mean literally to undo the fixture changes I had made in c4d8047c965eb14232052e5f1ca8959ed3f34b2e.
  14. Your sed commands keep failing.
  15. 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>

#10 @westonruter
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@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

#14 @westonruter
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 61927:

Script Loader: Preserve duplicate URL query params in enqueued scripts and styles.

Previously in r61397, add_query_arg() was used to append versions or handle-specific query arguments. This resulted in stripping 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.

The URL encoding changes in tests/phpunit/tests/dependencies/scripts.php are reversions of what had previously been done in r61397.

Developed in https://github.com/WordPress/wordpress-develop/pull/11164

Follow-up to r61397, r61358.

Props westonruter, jonsurrell.
Fixes #64372.

@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.

Note: See TracTickets for help on using tickets.