Squiz/FunctionComment: support DNF types

[devfrey]

Description

If a doc comment contained an intersection type, Squiz.Commenting.FunctionComment would report Doc comment for parameter "<param>" missing. This PR fixes that. The updated regex was taken from a similar fix to PEAR.Commenting.FunctionComment in f797a35.

Suggested changelog entry

Squiz/FunctionComment: support intersection types

Related issues/external references

N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@devfrey Thank you for this PR. Tested and confirmed working.

I just have one question: why did you change the condition order ?
As far as I can see, this makes no difference for the fix in this PR... ? And it does change the behaviour for an earlier test.

Other than that, please give credit where credit is due. As both the fix + the tests are literal copies of the original commit by @gsherwood, just in another sniff, it would be appropriate to add a Co-authored-by: Greg Sherwood <[email protected]> tag.

[devfrey]

@jrfnl Hi Juliette, thanks for reviewing!

I had to change the order of the conditions to make the tests pass for this example:

/**
 * @param $noTypeWithComment This parameter has no type specified.
 * @return void
 */

[LINE 1145] Expected 1 error(s) in FunctionCommentUnitTest.inc but found 0 error(s).

Otherwise it would not report the missing type here. Since the first condition (isset($matches[2]) === true) passes, the only error path after that is for a missing comment – which does not apply here. Do you have any suggestions for handling this differently?

You say it changes behaviour for an earlier test, which one are you referring to?

I've amended my commit message to include Greg.

[jrfnl]

I've amended my commit message to include Greg.

Thanks for doing that @devfrey!

You say it changes behaviour for an earlier test, which one are you referring to?

When running:

phpcs -ps ./squiz/tests/commenting/functioncommentunittest.inc --standard=squiz --report=full,summary,source --sniffs=squiz.commenting.functioncomment

I'm seeing the following difference with vs without the patch:

-  794 | ERROR | [ ] Missing parameter name (Squiz.Commenting.FunctionComment.MissingParamName)
+  794 | ERROR | [ ] Missing parameter type (Squiz.Commenting.FunctionComment.MissingParamType)

If I keep the change to the regex, but remove the order change, the diff becomes:

-  792 | ERROR | [ ] Missing parameter type (Squiz.Commenting.FunctionComment.MissingParamType)
+  792 | ERROR | [ ] Missing parameter comment (Squiz.Commenting.FunctionComment.MissingParamComment)
- 1139 | ERROR | [ ] Missing parameter type (Squiz.Commenting.FunctionComment.MissingParamType)
+ 1139 | ERROR | [ ] Missing parameter comment (Squiz.Commenting.FunctionComment.MissingParamComment)
- 1145 | ERROR | [ ] Missing parameter type (Squiz.Commenting.FunctionComment.MissingParamType)

So, I see your point about line 1145. I've left a suggestion inline. AFAICS, with that applied, there is no difference in behaviour for the pre-existing tests, while the new tests still pass.

[devfrey]

@jrfnl I've applied your suggestion, thank you for providing detailed steps! It definitely helped me understand what changed.

[jrfnl]

Thanks for making that update @devfrey ! All good now.

I will merge this once I've merged #954 so there will be a passing build.

Thanks for contributing to PHP_CodeSniffer 👍🏻

[jrfnl]

Old issues which may well be fixed by this (unverified - linking here for visibility in those issues):

• FunctionComment.SpacingAfterParamType not working with intersection types squizlabs/PHP_CodeSniffer#2887
• Fix intersection type annotations squizlabs/PHP_CodeSniffer#2919
• Fix intersection type annotations squizlabs/PHP_CodeSniffer#3459