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 Automattic/jetpack#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 Automattic/jetpack#15693 (review)) 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.

[jrfnl]

Checklist of things to do before this PR will be reviewed:

• Move the sniff to WordPress/Sniffs/Commenting.
  The Commenting subdirectory name is common for documentation sniffs in the PHPCS-sphere.
• Add unit tests.

Optional, but highly recommended:

• Add XML documentation for the sniff to the WordPress/Docs directory.

Suggested:

• Have a look at the Sniff::$hookInvokeFunctions property and see about using it.

[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 ;-)

[GaryJones]

Left a few comments for you to consider.

[GaryJones]

We have a lot of arrays of functions names in the Sniff class that all of the Abstract (and therefore concrete) classes extend from - might be worth renaming the property to a bit more explicitly ($hook_executing_functions?) and then moving them to there so then other sniffs that need this collection can use them.

[kraftbj]

Nice pointer. In 11e3a8f, I removed my array. There is already a $hookInvokeFunctions array in the Sniff class.

The difference is the value of each key in the existing one is true whereas mine had filter|action. Originally, I was going to ensure that the "previously documented" check (This filter is documented) used filter or action correctly, but opted against being that nit-picky.

Since I wasn't used the key, let's just use what's already there.

[GaryJones]

If the existing values were all true, then chances are the values weren't being used elsewhere. If there is benefit in having them be either 'filter' or 'action' so that this sniff can check a little more finely (would you propose to throw a different violation if they didn't match?), then I'm all for that.

cc @jrfnl for consideration. Unresolving this comment so it doesn't get missed.

[jrfnl]

I'm pretty sure that the values in the array aren't currently used, but a search of the codebase can verify this for sure.

I'm all for it to have filter|action as the values. The error messages could then potentially also use that term if it makes the error message clearer.

[GaryJones]

As noted previously, this may not always be possible. Consider this example from Juliette:

/**
 * Hook docblock
 */
if ( $a === true
    && apply_filter('some_filter', $value) > 10
) {
    // Do something.
}

Likewise, this should also be valid IMO, since it matches the PSR-5 definition of associating a DocBlock with a Structural Element with an optional number of clear lines in between:

/**
 * Hook docblock
 */

$value = apply_filters( 'some_filter', $value );

[jrfnl]

I concur. And also consider a call to a filter hook nested in another function call, which may be multi-line.

Think:

$foo = function_call(
    $param,
    array( 1, 2, 3 ),
    apply_filters( 'hook_name', $something)
);

Other things to consider:

• PHPCS ignore comments, those should be accepted between the docblock and the code.
• What if the comment found is a trailing comment for a previous code line ?

I suggest having a good read through the hook documentation rules to get this right. pay special attention to:

If a hook is in the middle of a block of HTML or a long conditional, the DocBlock should be placed on the line immediately before the start of the HTML block or conditional, even if it means forcing line-breaks/PHP tags in a continuous line of HTML.

From a code perspective, this should probably be a "Missing hook docblock" error and not complain about the line at all.

The way things are coded now, the "previous comment" might well be the docblock for a function declaration further up in the code or some other completely unrelated comment.

And if the hook docblock is "missing", the sniff should bow out from doing the rest of the checks as it may be examining a comment completely unrelated to the hook.

[GaryJones]

These are good, but some real-world edge cases will be needed!

Consider:

/**
 * This is my function DocBlock.
 *
 * @since 0000.00000.0000000000
 */
function my_function_has_a_hook() {
    do_action( 'foo' );
}

Likewise:

/**
 * This is my function DocBlock.
 *
 * @since 0000.00000.0000000000
 */
function my_function_has_a_hook() {
    echo 'foo';
}
do_action( 'foo' );

If these both already get reported as missing the hook DocBlock, then great! Add them in as unit tests, and then try:

/**
 * This is my function DocBlock.
 *
 * @since 0000.00000.0000000000
 */
function my_function_has_a_hook() { do_action( 'foo' ); }

😈

[kraftbj]

The first two did catch "correctly", but probably wouldn't after I resolve the preceding line issue mentioned in #1892 (comment)

But, the third example of a single-line function fails the unit test since it doesn't see that as an error.

I'm going to push up what I have (and mark this as resolved since the tests are added) and think on it a bit.

I'm thinking for the third example, we can search for a comment previous to the hook but stop at a function declaration at least. May be others.

Thanks!

[kraftbj]

ad220fd

[GaryJones]

Either here or PHPCSUtils, I bet @jrfnl has examples / helper functions to find something within a specific scope i.e. find a DocComment within the current scope.

[jrfnl]

I do have docblock finder helper functions lined up to go into PHPCSUtils at some point, but as hook docblocks are a bit of a different beast compared to most structural elements I don't think that will help here.

I do have a draft HooksTrait lined up for WPCS 3.0.0 with logic for finding hook docblocks (for another sniff).

When I pull that, I will combine the logic for finding hook docblocks from this sniff with the logic in the trait to get the best of both brains in the trait (and switch this sniff over to using the trait).

[GaryJones]

A "DocBlock" is made up of two parts:

• the /** */ structure, known as a DocComment.
• the contents with Summary, optional description, tags, etc. known as the PHPDoc.

As such, the "DocBlock comment" here, which is referencing whether one asterisk or two is used, is really checking if it's a multi-line comment (/*) or a DocComment (/**), hence my earlier suggestion of `Invalid: not using a DocComment."

(Note that the period is still missing at the end from your version.)

[jrfnl]

Hi @kraftbj, thanks for your patience.

I've done an initial review of the sniff and have left some (lots...) of comments inline. Please don't be discouraged by that. A lot of comments are only minor things, though the docblock finding logic does still need further refinement.

Overall, this is not bad at all for a first pass at writing sniffs/writing this sniff. 👍

Other than that:

1. Sniff name: let's have a little discussion about the name:
   Basically within the WP context, hook invocations are a structural element, just like class or function declarations, which is why a docblock is required.
   The sniff adheres to that and explicitly does not allow inline comment-style, but expressly looks for docblocks and will only accept those.
   So with that in mind, I personally find the sniff name containing InlineDocs confusing.
   Suggestion for an alternative name: HookDocBlockSniff.
   I'm open to other names if someone comes up with a better one.
   Note: when you do change this, please change it everywhere the name is used, including in the docs.
2. Ruleset: the sniff will currently only be included if people include the WordPress ruleset, which automatically includes all sniffs.
   I think it would be a good idea to actually add the sniff to the appropriate ruleset WordPress-Docs.
3. Please do at least minimal validation of the received property input. I.e. make sure it is an array which was passed and filter out potential empty values before using the property.
4. The sniff class and function docblocks should all have a @since tag. (For the unit tests this is only needed for the class docblock, where it is set correctly.) You can use 3.0.0 as the version number.

Hope my comments help you to further iterate on the sniff. Let me know if you have any questions.

[jrfnl]

This is not how array values are declared in a ruleset. Aside from the XML parse error, this also won't work if used per the example.

Try this instead:

	<rule ref="WordPress.Commenting.HooksInlineDocs">
		<properties>
			<property name="allowed_extra_versions" type="array">
				<element value="0.71"/>
				<element value="MU (3.0.0)"/>
			</property>
		</properties>
	</rule>

[jrfnl]

Please remove the example from the abstract. This property is not intended to be overruled.

[jrfnl]

Please add a proper description of what the sniff is about and what it is trying to detect.
Include links to the handbook where relevant.

[jrfnl]

Nitpick: one blank line after the class declaration line please (also for the unit tests).

[jrfnl]

Please add a process_token() method to prevent an exclude property (inherited from the parent sniff) which can be set in a custom ruleset from disabling this sniff.

Something like this:

	/**
	 * Processes this test, when one of its tokens is encountered.
	 *
	 * @since x.x.x
	 *
	 * @param int $stackPtr The position of the current token in the stack.
	 *
	 * @return int|void Integer stack pointer to skip forward or void to continue
	 *                  normal file processing.
	 */
	public function process_token( $stackPtr ) {
		// Disallow excluding function groups for this sniff.
		$this->exclude = array();

		parent::process_token( $stackPtr );
	}

[jrfnl]

This doesn't do what you expect/documented.

Test case:

/**
 * Summary.
 *
 * @since
 * 
 * @param int $value Description.
 */

As it is, it will find the comment string related to the parameter. You account for that by doing a line check, but the sniff would be more efficient by doing a reverse find (find the next non-docblock-whitespace token and checking it's a T_DOC_COMMENT_STRING token). You'll still need the same line check though.

[jrfnl]

👆 Some (early) continue/breaks could make this whole foreach block a lot more readable.

[jrfnl]

Don't declare $matches if you're not going to use it. (though using it in a Found: %s might not be a bad idea.)

Also be aware that \d is locale dependent.

Suggested change

	} elseif ( ! preg_match( '/^\d+\.\d+\.\d+/', $this->tokens[ $string ]['content'], $matches ) ) { // Requires X.Y.Z. Trailing 0 is needed for a major release.
	} elseif ( preg_match( '/^\d+\.\d+\.\d+/', $this->tokens[ $string ]['content'] ) !== 1 ) {

[jrfnl]

This could be a utility function if other sniffs would have a use for it. As it is, it is a sniff local function and only used for one specific check, so you may as well refer directly to the class property instead of passing the $array in.

Also the method name could probably be more descriptive.

[jrfnl]

I'm missing a test case to confirm that the sniff throws an error for each @since tag with an incorrect/missing version in a docblock.

/**
 * Summary.
 *
 * @since
 * @since 1.2.3
 * @since Some description.
 */
apply_filters(' hook', 'function' );

The sniff should (and will) throw an error for the first and the third @since tag.

Also missing a test cases with a "more than three-#" type version nrs. The sniff as-is will handle this correctly, but we should safeguard that it does for the future.

/**
 * Summary.
 *
 * @since 1.2.3.4
 * @since 1.2.3-RC1
 */
apply_filters(' hook', 'function' );

[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.