Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Member variables and methods should not require DocBlock when properly typed #476

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
103 changes: 103 additions & 0 deletions Magento2/Sniffs/Annotation/MethodArgumentsSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ class MethodArgumentsSniff implements Sniff
'self'
];

private const MAXIMUM_COMPLEXITY_ALLOWED_FOR_NO_DOCBLOCK = 5;

/**
* @inheritdoc
*/
Expand Down Expand Up @@ -568,6 +570,9 @@ public function process(File $phpcsFile, $stackPtr)
$previousCommentClosePtr = $phpcsFile->findPrevious(T_DOC_COMMENT_CLOSE_TAG, $stackPtr - 1, 0);
if ($previousCommentClosePtr && $previousCommentOpenPtr) {
if (!$this->validateCommentBlockExists($phpcsFile, $previousCommentClosePtr, $stackPtr)) {
if (!$this->checkIfMethodNeedsDocBlock($phpcsFile, $stackPtr)) {
return;
}
$phpcsFile->addError('Comment block is missing', $stackPtr, 'NoCommentBlock');
return;
}
Expand Down Expand Up @@ -617,6 +622,104 @@ public function process(File $phpcsFile, $stackPtr)
);
}

/**
* Check method if it has defined return & parameter types, then it doesn't need DocBlock
*
* @param File $phpcsFile
* @param int $stackPtr
* @return bool
*/
private function checkIfMethodNeedsDocBlock(File $phpcsFile, $stackPtr) : bool
{
if ($this->checkIfNamespaceContainsApi($phpcsFile)) {
// Note: API classes MUST have DocBlock because it uses them instead of reflection for types
return true;
}
Comment on lines +634 to +637
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to cover this case - where a docblock is required due to a class being an API.

$foundAllParameterTypes = true;
$methodParameters = $phpcsFile->getMethodParameters($stackPtr);
foreach ($methodParameters as $parameter) {
if (!$parameter['type_hint']) {
return true; // doesn't have type hint
}
}
$methodProperties = $phpcsFile->getMethodProperties($stackPtr);
$foundReturnType = !!$methodProperties['return_type'];
if (!$foundReturnType) {
$methodName = $phpcsFile->getDeclarationName($stackPtr);
// Note: __construct and __destruct can't have return types
if ('__construct' !== $methodName && '__destruct' !== $methodName) {
return true;
}
}
$complexity = $this->getMethodComplexity($phpcsFile, $stackPtr);
return $complexity > static::MAXIMUM_COMPLEXITY_ALLOWED_FOR_NO_DOCBLOCK;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to cover this case - where a docblock is required due to high complexity score.

}

/**
* Check if namespace contains API
*
* @param File $phpcsFile
* @return bool
*/
private function checkIfNamespaceContainsApi(File $phpcsFile) : bool
{
$namespaceStackPtr = $phpcsFile->findNext(T_NAMESPACE, 0);
if (!is_int($namespaceStackPtr)) {
return false;
}
$tokens = $phpcsFile->getTokens();
// phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterOpen,Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed
JacobBrownAustin marked this conversation as resolved.
Show resolved Hide resolved
for (
// phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterFirst
JacobBrownAustin marked this conversation as resolved.
Show resolved Hide resolved
$index = $namespaceStackPtr;
// phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterSecond
JacobBrownAustin marked this conversation as resolved.
Show resolved Hide resolved
array_key_exists($index, $tokens) && 'T_SEMICOLON' !== $tokens[$index]['type'];
$index++
// phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeClose
JacobBrownAustin marked this conversation as resolved.
Show resolved Hide resolved
) {
if ('T_STRING' === $tokens[$index]['type'] && 'Api' === $tokens[$index]['content']) {
return true;
}
}
return false;
}

/**
* Get method CyclomaticComplexity
*
* @param File $phpcsFile
* @param int $stackPtr
* @return int
*/
private function getMethodComplexity(File $phpcsFile, $stackPtr) : int
{
$tokens = $phpcsFile->getTokens();
$start = $tokens[$stackPtr]['scope_opener'];
$end = $tokens[$stackPtr]['scope_closer'];
$predicateNodes = [
T_CASE => true,
T_DEFAULT => true,
T_CATCH => true,
T_IF => true,
T_FOR => true,
T_FOREACH => true,
T_WHILE => true,
T_ELSEIF => true,
T_INLINE_THEN => true,
T_COALESCE => true,
T_COALESCE_EQUAL => true,
T_MATCH_ARROW => true,
T_NULLSAFE_OBJECT_OPERATOR => true,
];
$complexity = 1;
for ($stackPtr = $start + 1; $stackPtr < $end; $stackPtr++) {
if (isset($predicateNodes[$tokens[$stackPtr]['code']])) {
$complexity++;
}
}
return $complexity;
}

/**
* Validates function params format consistency.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ public function processMemberVar(File $phpcsFile, $stackPtr)
);

if ($commentEnd === false || $tokens[$commentEnd]['code'] !== T_DOC_COMMENT_CLOSE_TAG) {
$propertyInfo = $phpcsFile->getMemberProperties($stackPtr);
if ($propertyInfo['type']) {
return; // If variable has a type, it is okay to not have a DocBlock
}
$phpcsFile->addWarning('Missing PHP DocBlock for class property.', $stackPtr, 'Missing');
return;
}
Expand Down
54 changes: 52 additions & 2 deletions Magento2/Tests/Annotation/MethodArgumentsUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,63 @@ public function invalidDocBlockShouldNotCauseFatalErrorInSniff(int $number): int
* @return string
*/
#[\ReturnTypeWillChange]
public function methodWithAttributeAndValidDocblock(string $text): string
public function methodWithAttributeAndValidDocblockWithTypes(string $text): string
{
return $text;
}

/**
* Short description.
*
* @param string $text
* @return string
*/
#[\ReturnTypeWillChange]
public function methodWithAttributeAndValidDocblockWithoutTypes()
Comment on lines +52 to +56
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has no $text parameter. If this is an intentional test, please add a comment to show the same.

{
return $text;
}

#[\ReturnTypeWillChange]
public function methodWithAttributeAndWithoutDocblock(string $text): string
public function methodWithAttributeAndWithoutDocblockWithTypes(string $text): string
{
return $text;
}

#[\ReturnTypeWillChange]
public function methodWithAttributeAndWithoutDocblockWithoutTypes($text)
{
return $text;
}

public function methodWithoutDockblockWithParameterTypeWithoutReturnType(string $text)
{
return $text;
}

public function methodWithoutDockblockWithoutParameterTypeWithReturnType($text) : string
{
return $text;
}

/**
* Short description.
*
* @param string $text
* @return string
*/
public function methodWithDockblockWithParameterTypeWithoutReturnType(string $text)
{
return $text;
}

/**
* Short description.
*
* @param mixed $text
* @return string
*/
public function methodWithDockblockWithoutParameterTypeWithReturnType($text) : string
{
return $text;
}
4 changes: 3 additions & 1 deletion Magento2/Tests/Annotation/MethodArgumentsUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ public function getErrorList()
12 => 1,
21 => 1,
32 => 1,
50 => 1
68 => 1,
73 => 1,
78 => 1,
];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,3 +201,20 @@ class correctlyFormattedClassMemberDocBlock
*/
protected string $deprecatedWithKeyword;
}

class classWithPropertiesWithTypesAndWithoutDocBlock
{
protected string $test1 = 'asdf';

protected ?string $test2;

private array $array1 = [];

public array $array2;

protected readonly int $readOnlyInteger;

protected readonly string $readOnlyString;

private ?\Exception $exception;
}
Loading