Add sniff: WordPress.WP.EnqueuedResourceParameters

[jrfnl]

WPCS 1.0.0 added a new WordPress.WP.EnqueuedResourceParameters sniff which checks that when registering/enqueuing scripts and styles, a $version is passed correctly so new versions will break out of the browser cache and for scripts, that the $in_footer parameter is passed to prevent unnecessary layout-rendering blocking scripts.

I would like to suggest adding this sniff to the TRTCS ruleset.

Opinions ?

Refs:

• Enhancement: add new sniff to verify optimized use of wp_enqueue_... functions WordPress/WordPress-Coding-Standards#1146
• Added Enqueued functions check for version and in footer WordPress/WordPress-Coding-Standards#1378
• Added Enqueued functions check for version and in footer WordPress/WordPress-Coding-Standards#1233
• https://developer.wordpress.org/reference/functions/wp_register_script/
• https://developer.wordpress.org/reference/functions/wp_enqueue_script/
• https://developer.wordpress.org/reference/functions/wp_register_style/
• https://developer.wordpress.org/reference/functions/wp_enqueue_style/
• https://codex.wordpress.org/Function_Reference/wp_register_style

[justintadlock]

I'd say no to the version parameter. There are other cache-busting techniques that theme authors use that work with their build tools, such as Laravel Mix.

As for the in footer parameter, we can't make that call. Theme authors should determine where a script should be loaded.

Both would make for good notices but not blockers.

[jrfnl]

Both would make for good notices but not blockers.

PHPCS can only throw errors and warnings, not notices.

For the above mentioned sniff, we could make sure all messages thrown are warnings. The in_footer one already is. The version one could be downgraded from within the ruleset.

As for the in footer parameter, we can't make that call. Theme authors should determine where a script should be loaded.

The sniff will only throw a warning when the parameter isn't passed. If the parameter is passed, but explicitly set to false, the sniff will do nothing as in that case, it is clearly a conscious choice by the developer to load in the script in the header.

There are other cache-busting techniques that theme authors use that work with their build tools, such as Laravel Mix.

I'm not familiar with Laravel Mix, could you expand a bit on how that works in combination with registered/enqueue styles and scripts ?

[justintadlock]

I can go into more detail when I get on my computer and share a code example. But, basically, Mix creates a manifest file for the script/style files with a ?id=xxxxxx appended to the file names. The theme uses those file names instead. Same concept as the version parameter but done automatically by the build tool when a change is made.

[dingo-d]

Webpack has something similar. It appends hashes when bundling the assets, and you can use the webpack manifest plugin that will create manifest.json file that you can reference when enqueueing scripts and styles (here and here).

Not what is used in the core and Gutenberg when using webpack, we could get some input from them as well.

[justintadlock]

Laravel Mix is built on top of Webpack, so it probably does it in a similar way.

Here's an example of an asset() function that I'm using. It basically pulls some JSON data that we'ved looked up and stored from a mix-manifest.json file, which looks something like this:

{
    "/scripts/app.js": "/scripts/app.js?id=80419afdb240041e373e",
    "/styles/screen.css": "/styles/screen.css?id=d41d8cd98f00b204e980",
    "/styles/editor.css": "/styles/editor.css?id=d41d8cd98f00b204e980",
    "/styles/customize-controls.css": "/styles/customize-controls.css?id=d41d8cd98f00b204e980",
    "/scripts/customize-preview.js": "/scripts/customize-preview.js?id=5d7890f008eb42cfd35a",
    "/scripts/customize-controls.js": "/scripts/customize-controls.js?id=a010549458e1f8dddc47"
}

In the theme, whenever we want an asset, the asset() function swaps the asset filename with the filename that has the ID appended.

When using the wp_enqueue_*() functions, I just explicitly pass in null for the $version parameter like so:

wp_enqueue_script( 'mythic-app', asset( 'scripts/app.js' ), null, null, true );

This is output on the front end as:

<script type="text/javascript" src="http://localhost/wp-content/themes/abc/dist/scripts/app.js?id=80419afdb240041e373e"></script>

[jrfnl]

Ok, so I understand the argument against the $version argument check, but I still wonder how frequently this is used in themes submitted to wp.org. Is this used in 5% of themes ? 20% ? 80 % ?

[justintadlock]

Ok, so I understand the argument against the $version argument check, but I still wonder how frequently this is used in themes submitted to wp.org. Is this used in 5% of themes ? 20% ? 80 % ?

Warnings are fine but not errors. The goal here is to eventually have this tool as part of the theme submission process. Basically, anything I say is from that point of view -- should this block a theme from being submitted to the directory for review?

• If yes, it should be an error.
• If no, it should be a warning (the author and reviewer can further look into it).

As far as I'm concerned, if it's used in 1 theme, we need to account for it. It's the validity of the use case that matters here.

More and more theme authors are using build tools like this now. WordPress' $version parameter is from a time when build tools were not in common use that handled versioning.

Also, the example I pointed to above is from a starter theme that I'm going to be introducing to the community soon. It's my hope that many themes in the directory start using that method.

[jrfnl]

@justintadlock Changing the error to a warning is not a problem at all.

So the question basically is:

• Should the check for $version be included in the TRTCS ruleset at all ?
• If yes, should it be a warning or an error ?

The second question looks to be answered, i.e. if it is included, it should be a warning, not an error. The first is still under discussion.

As a side-note: I can imagine the Webpack/Laravel Mix argument can also be made for the sniff itself, so maybe an issue should be opened in WPCS suggesting lowering the error level to warning ?

[joyously]

I think this has no place in this ruleset, not even as a warning.

[dingo-d]

Care to explain why not? I'm fine with excluding this, due to the fact I'm working with webpack, but many people are not.
The idea is that if you updated the theme and made some changes, if you don't use some bust caching mechanism, people who update the theme may not get visual updates (because the version appended at the end of the css and js files is the WP version number).

[ernilambar]

I am up for checking $version and keeping it as warning.

[justintadlock]

I'm also fine with including it. It just needs to be a warning rather than an error.

On the other hand, the sniff itself doesn't address the problem of keeping the version updated for cache busting. The theme authors who need this the most are likely the same theme authors who'll set it once and never update it. In that case, it's even more beneficial for it to not be set (at least it'll get the WP version and change on occasion). And, it's not something the review team is going to manually check. So, I do question the usefulness of having this in.

[joyously]

Yes, what Justin said. Also, there is no requirement to tie it to. Also, there are plugins that remove the version numbers. And it doesn't apply to add_editor_style, where it is needed. And because of that, the versions could be added in a filter.

[timelsass]

I think about 3rd party deps that get included frequently, so something like Bootstrap v4 should have $handle of bootstrap and $version @ 4.x.x. The purpose of the version on third party enqueued scripts is to provide resolution for which script @ version to actually enqueue to help avoid conflicts. I haven't fully thought out all the scenarios, but maybe a better solution might be along the lines of:

• if $handle isn't prefixed with the theme prefix, then check for $version param. Error if that's the case.

I agree with @joyously - I don't think warning/error would be necessary for any of the prefixed script handles, as the author could handle via a cache busting technique similar to what is outlined above, or leaving it out to use the fallback. I do however see validity in it being an error for thirdparty enqueued assets, which aren't prefixed.

---

As far as real use case scenarios go, I recall seeing on multiple occasions theme and plugin issues which complain about fontawesome icons not working. It's usually not hard for the authors to sort it out, and the root cause is usually from the version strings not being added for the right resolution.

Example: relying on fallback for WP 4.8 worked fine for a theme enqueuing FA 4.x, and a plugin enqueues FA 5.0.x. The user upgrades WP to 5.2.1, and now the theme's version of FA 4.x version is loaded and they are missing icons only in 5.0.x which were provided from the plugin initially.

[carolinan]

If this is kept as a warning, it shouldn't be used for the external fonts.

[dingo-d]

We didn't reach a conclusion about adding this or not at the triage. I'm for including this with a warning notice, Joy is not.