First pass at sniff to confirm inline docs for filters are present

[kraftbj]

Fixes #424

First pass at sniff to confirm inline docs for hooks are present.

I'm adding this into Jetpack via https://github.com/Automattic/jetpack/pull/15693 since I wanted to check for some Jetpack-specific things. Per suggestion, wanted to offer the "base" to core WPCS for everyone's benefit.

In the initial commit, it finds a matching the various hook function (L27) using WPCS' AbstractFunctionRestrictionsSniff and checks for a preceding comment. The preceding comment need to be on the previous line (noting this is an open question from the Jetpack PR https://github.com/Automattic/jetpack/pull/15693#pullrequestreview-406406725) and confirm it is using the docblock format.

For the initial commit, it is verifying it has a @since tag within the docblock and that the value starts with a X.Y.Z version number. I'm not sure about WP itself, but Jetpack requires three levels to both confirm the exact version (e.g. 8.5 vs 8.5.0) and ensure that all hooks in the same major release (X.Y) will have the same value to avoid our docs parser tagging it as two separate versions.

If Core WPCS doesn't need that, I can remove that to just verify a @since tag is present and move the version check itself to Jetpack as I did with our @module tag in https://github.com/Automattic/jetpack/pull/15693/files#diff-eb7bc4afcdf23e6f99ae1f793afd119bR57

[jrfnl]

Hi @kraftbj Welcome to WPCS ;-)

Thanks for this PR. I haven't looked at the code yet, but would like to ask you whether you have run the sniff over WP Core to see what it throws up.

Any false positives from WP Core should be handled, or when in doubt whether something is a false positive, let's discuss it.

For the initial commit, it is verifying it has a @since tag within the docblock and that the value starts with a X.Y.Z version number. I'm not sure about WP itself, but Jetpack requires three levels to both confirm the exact version (e.g. 8.5 vs 8.5.0) and ensure that all hooks in the same major release (X.Y) will have the same value to avoid our docs parser tagging it as two separate versions.

Core requires three levels too as per the Documentation guidelines, so checking for three levels is fine.

FYI: I expect WPCS 3.0.0 to contain a trait for version number checking (written for another sniff), so that can probably be re-used.

With that in mind and the last 2.x release expected soon, after which the pulls for WPCS 3.0.0 will start, I'd like to earmark this PR for WPCS 3.0.0.

[kraftbj]

Sounds good. New to PHPCS overall, so appreciate the guidance on naming scheme. Took a guess that WP seem to fit best based on the existing directories.

I'll get some unit tests worked up early next week since it sounds like this is a sniff that would be considered for inclusion.

[jrfnl]

New to PHPCS overall, so appreciate the guidance ...

Please feel free to ask for as much guidance as you need from me and the others in the team.

In my case, my remarks will often presume you know what you're doing with PHPCS (as why would you send in a sniff otherwise?), please don't let that intimidate you and ask for as much clarification as you need.

it sounds like this is a sniff that would be considered for inclusion.

Definitely. There are long standing open issues about this check missing from WPCS, though I also have to caution you that it may not be the easiest issue to tackle. Can be a great way to learn more about writing sniffs though ;-)

[kraftbj]

Sounds good. I appreciate the tip on running it against WP to find false positives. I'll make a couple of tweaks when sending up the unit tests.

[GaryJones]

For the initial commit, it is verifying it has a @since tag within the docblock and that the value starts with a X.Y.Z version number.

Maybe @since Unknown should also be permitted? I believe that has/had some instances in core function/methods DocBlocks as well at one point (example in SimplePie package).

The PR is going to need some unit tests as well.

[jrfnl]

Maybe @since Unknown should also be permitted?

I don't think we should allow that. Both SVN as well as Git allow to trace things through history, so I can't think of a valid reason to allow @since Unknown.

[kraftbj]

I added unit tests and documentation, but I'm not sure how to unit test when the ruleset would be passing a property as an array.

With the X.Y.Z. format, there are two instances currently in Core which does not follow this which I am not keen at automatically changing:

• 0.71 -- the initial release did not follow the X.Y.Z format.
• MU (3.0.0) -- to denote items that came in when MU was merged into Core. We could modify these to be 3.0.0 (MU) as an alternative.

To allow these, I set a rule property of an array that could be defined in the ruleset XML for any "exceptional" versions. I just don't know how to pass that to the unit test via phpcs:set.

Any pointers?

[kraftbj]

Maybe @since Unknown should also be permitted?

For Core, I would disagree, but for implementing projects, I did add a way to set an "exceptional" version. If an implementing project wanted to allow that, they could in their own phpcs.xml.dist file as the PR stands now.

[jrfnl]

I'm not sure how to unit test when the ruleset would be passing a property as an array.

See: https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc#L6 for an example of setting an array property.

Please also remember to reset the property after the test using it and/or at the end of the test file (see bottom of the above linked test file for an example).

[kraftbj]

@jrfnl I really appreciate the pointers and direction. I think this is at a state where you can ~rip it to shreds~ review it.

[jrfnl]

@kraftbj Wowie! I'll try and have a look some time this week ;-)

[kraftbj]

This is wonderful. Thank you so much for the review. Today is a standard holiday in the U.S. so a short week for me. I'll try to make a good dent on this during the week, but may get bumped to next. If so, my delay doesn't mean I'm discouraged! 🙂

[jrfnl]

While it would be nice to have a sniff for this, the PR as-is is not mergable with way too many open review comments and no action taken on them in over two years, so I'm going to close this PR.