search

WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
209 stars 37 forks source link

Add new upstream sniffs #240

Open dingo-d opened 5 years ago

dingo-d commented 5 years ago

Detached the two new sniffs from #239 and add them here.

Short ternary addition seems like it promotes best practices, and escaped not translated sniff seems like a useful sniff for themes to have.

dingo-d commented 5 years ago

Valid points! Will put this for next major version milestone 👍

Good point about shorthand ternary. Tbh I almost never used it (I use null coalesce most of the time), and from what I've seen by searching the theme directory (link), they are using it wrong - checking the existence of key in array with short ternary will either throw an error or true instead of the key (which I think is what the authors intended). So adding it as a warning might be a good thing, but could also confuse reviewers who aren't on good terms with PHP. Definitely one for discussion (I'll open an issue for it :+1:)

There is one more new sniff WordPress.DateTime.CurrentTimeTimestamp and a second group in the WordPress.DateTime.RestrictedFunctions sniff. Any particular reason why those haven't be pulled (either in this PR or in a separate one)

The first one I think I wasn't 100% sure if this is allowed in a theme (from your comment it looks like it has a merit of being in the theme). The second group I didn't add because you mentioned it in the comment in the PR

The WordPress.DateTime.RestrictedFunctions sniff contains a second function group, so we need to make sure that one is not (yet) included.

So I didn't include it 😄

I presume you didn't pull the PostTypeSlug one as registering post types is not allowed in themes ? Or am I remembering that wrong ?

Correct 👍 Themes cannot register post types (this is covered by the ForbiddenFunctions sniff).

jrfnl commented 5 years ago

The second group I didn't add because you mentioned it in the comment in the PR

It didn't belong in the other PR as it would change the functionality in TRTCS, while that PR was just about updating the WPCS dependency.

For this PR, which is intended to change the TRTCS functionality, it would be a good fit.

jrfnl commented 5 years ago

Basically, it's about decision points: For the other PR the decision was supposed to be "should we update the minimum WPCS version to 2.2.0 ?".

By having the extra datetime check in that same PR, that PR would be asking for two decisions: "should we update the minimum WPCS version to 2.2.0 ?" AND "should this new check be added to TRTCS ?"

As those are two different decisions which may have different answers, they shouldn't be in the same PR.

To bring the point home: the first decision is something which could go into a patch version, the second decision means that PR would have to go into the next minor (as discussed above).

Does that help ?

dingo-d commented 5 years ago

Yes, it makes sense. I've moved the milestone so we can work on this after 0.2.1 is released 🙂