WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.54k stars 479 forks source link

Change WordPress.WP.EnqueuedResourceParameters.MissingVersion to warning #1472

Closed GaryJones closed 1 year ago

GaryJones commented 6 years ago

EnqueuedResourceParameters is included in WordPress-Extra.

It was recently added via #1378.

While I agree that it's a best practice to add a script version (WP will use its own version anyway if not given), I think we should reconsider this being reported as an error.

Example case, would be registering a script from a CDN e.g. https://cdn.bootcss.com/backgrid.js/0.3.8/backgrid.min.js - this already contains the version number, so having to add a version number as a separate argument doesn't make sense.

The arg is optional - if it was something that someone must do (for which not doing would be an error), then it wouldn't be optional.

I realise this is for Extra, not Core, and that there are ways for individual projects to ignore, disable or change the type for this violation, but I'd like to see us re-consider changing it in WPCS.

XedinUnknown commented 6 years ago

I would like to add that adding parameters to the URL may disable resource caching on the CDN, or even break the URL altogether.

jrfnl commented 6 years ago

:+1: I think that would be good change. I've also seen this issue crop up with projects using WebPack which automatically adds a hash which changes when the code changes (or something along those lines, I don't know the fine detail of WebPack), but the effect of that is that the version number isn't needed in those cases either.

dingo-d commented 6 years ago

It was this issue: https://github.com/WPTRT/WPThemeReview/issues/170

jrfnl commented 5 years ago

@moorscode As you did most of the work on this sniff, any thoughts ?

CamelKing commented 5 years ago

Any update on this? The boiler plate generated by Underscores failed phpcs, LOL

jrfnl commented 5 years ago

@CamelKing The _s boiler plate fails a WPCS check for a whole lot more than just this as things just aren't getting merged: https://github.com/Automattic/_s/pull/1311

jrfnl commented 1 year ago

Not sure why nobody actioned this before, but PR #2146 should fix it now.