WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11315 closed enhancement (fixed)

Versioning is being forced for JavaScript and CSS assets with no override

Reported by: amattie Owned by: azaozz
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: JavaScript Keywords: has-patch
Focuses: Cc:

Description

It seems that there's currently no way to perform script and css resource versioning manually with wp_enqueue_script and wp_enqueue_style. When false, null, an empty string, or anything that evaluates to false is passed as the version argument, the version of WordPress is appended to the asset.

This causes problems when trying to use wp_enqueue_script or wp_enqueue_style to manage assets on external web servers where something that is interpreted as a query string on a "normal" web server is interpreted as part of the asset name on that "special" web server. In my case, I'm trying to reference a script placed on Amazon S3 and served through their CloudFront CDN. I think a case can also be made where you don't want versioning on assets that you don't fully control. A good example of this is Google Maps, which I'm also using. Passing ?ver=2.9 to the Google Maps JavaScript loader could have unknown consequences in the future.

My proposal is to skip the setting of the version in the query string for the asset if an empty string is passed in for that argument. The way I did this in my patch was by simply implementing strict type checking in a few key spots. This change leaves the default functionality in place while still providing a way for developers to override the version setting functionality.

Attachments (5)

wp_enqueue_versioning_override.patch (2.2 KB) - added by amattie 5 years ago.
Patch for new functionality of wp_enqueue_*
11315_remove.diff (2.5 KB) - added by scribu 5 years ago.
Remove default version altogether
11315_override.diff (4.2 KB) - added by scribu 5 years ago.
Add default version only for null value
12320_override.patch (2.3 KB) - added by amattie 5 years ago.
Override versioning with empty string, strict false assignment removed in _WP_Dependency
11315_override.2.diff (5.6 KB) - added by scribu 5 years ago.
Add documentation, use empty string instead of null

Download all attachments as: .zip

Change History (24)

@amattie5 years ago

Patch for new functionality of wp_enqueue_*

comment:1 @scribu5 years ago

  • Milestone changed from Unassigned to 3.0

+1 on this: shouldn't force version argument on users.

That being said, I don't get what the purpose of the following code from your patch is:

 	235	                if ( $this->ver === false ) 
236	236	                        $this->ver = false; 

:)

comment:2 @Denis-de-Bernardy5 years ago

  • Component changed from General to JavaScript
  • Keywords needs-patch added
  • Owner set to azaozz
  • Type changed from defect (bug) to enhancement

agreeing with scribu. the idea is good, but the patch is extremely fishy.

comment:3 @dd325 years ago

That !=== false is required as if you pass a empty string, PHP will interprate that as false. ( == false && 0 == false & null == false && array() == false && false == false) == true

comment:4 follow-up: @dd325 years ago

Damn formatter..

 ('' == false && 0 == false & null == false && array() == false && false == false) == true 

comment:5 @lloydbudd5 years ago

Be sure to update the php doc... and http://codex.wordpress.org/Function_Reference/wp_enqueue_script is already incorrect.

@scribu5 years ago

Remove default version altogether

@scribu5 years ago

Add default version only for null value

comment:6 in reply to: ↑ 4 ; follow-up: @scribu5 years ago

Replying to dd32:

Damn formatter..

 ('' == false && 0 == false & null == false && array() == false && false == false) == true 

No, I mean what's the point of checking for false, and then assigning false to the same variable?

I've added two patches:

11315_remove.diff removes the default version altogether, since it isn't that useful, IMO

11315_override.diff corrects the patch from amattie and includes the default version only when not explicitly set by the user.

comment:7 @scribu5 years ago

  • Keywords has-patch added; needs-patch removed

This is what I've tested the patches agains:

add_action('template_redirect', 'debug_script_loader');
function debug_script_loader() {
	wp_enqueue_script('test1', 'example.com', array());
	wp_enqueue_script('test2', 'example.com', array(), 1.2);
	wp_enqueue_script('test3', 'example.com', array(), false);

	wp_enqueue_style('test1', 'example.com', array());
	wp_enqueue_style('test2', 'example.com', array(), 1.2);
	wp_enqueue_style('test3', 'example.com', array(), false);
}

comment:8 follow-up: @dd325 years ago

No, I mean what's the point of checking for false, and then assigning false to the same variable?

Sorry.. I thought one of them was $ver, not $this->ver :(

comment:9 in reply to: ↑ 8 @scribu5 years ago

Replying to dd32:

No, I mean what's the point of checking for false, and then assigning false to the same variable?

Sorry.. I thought one of them was $ver, not $this->ver :(

No worries. :)

comment:10 in reply to: ↑ 6 @lloydbudd5 years ago

  • Keywords has-patch removed

Replying to scribu:

11315_remove.diff removes the default version altogether, since it isn't that useful, IMO

It's quite useful. It breaks the browser cache when you upgrade WordPress, so your site's visitors and bloggers are not served a hodge hodge . Debugging nightmare.

11315_override.diff corrects the patch from amattie and includes the default version only when not explicitly set by the user.

Sounds good... still no documentation of the behavior -- removed 'has-patch' pending doc.

comment:11 follow-up: @amattie5 years ago

Regarding the purpose of this:

 	235	                if ( $this->ver === false ) 
236	236	                        $this->ver = false; 

You're correct in that that's kinda useless, but perhaps 235-236 should be removed altogether then. Without removing it (as it is in 11315_override.diff or 11315_remove.diff), $this->ver is set to false if an empty string is passed in since evals to false. If $this->ver is set to false (strict) at that point, checking for === false in the other spots will become useless.

In other words, the suggested behavior of setting the value to an empty string to override the versioning will have no effect if 235-236 is left in there.

Thoughts on removing 235-236 in _WP_Dependency?

@amattie5 years ago

Override versioning with empty string, strict false assignment removed in _WP_Dependency

comment:12 in reply to: ↑ 11 ; follow-up: @scribu5 years ago

Replying to amattie:

Regarding the purpose of this:

 	235	                if ( $this->ver === false ) 
236	236	                        $this->ver = false; 

You're correct in that that's kinda useless, but perhaps 235-236 should be removed altogether then. Without removing it (as it is in 11315_override.diff or 11315_remove.diff), $this->ver is set to false if an empty string is passed in since evals to false.

But they are removed altogether in both patches.

comment:13 in reply to: ↑ 12 @amattie5 years ago

Replying to scribu:

But they are removed altogether in both patches.

Argh, my apologies -- I totally missed that.

@scribu5 years ago

Add documentation, use empty string instead of null

comment:14 @scribu5 years ago

  • Keywords has-patch added

comment:16 @azaozz5 years ago

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

(In [12558]) WP_Dependencies: pass NULL to disable script and style version query strings, props scribu amattie, fixes #11315

comment:17 @azaozz5 years ago

Don't see a good reason to change the current behaviour. Passing FALSE should still append the default version, pass NULL to disable it.

comment:18 @azaozz5 years ago

(In [12559]) Too many question marks, see #11315

Note: See TracTickets for help on using tickets.