From ee3ddb38fb023679373f5461d9e502c3ba368af7 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 16 Aug 2023 09:21:34 -0500 Subject: [PATCH 1/6] Member variables and methods should not require DocBlock when properly typed https://developer.adobe.com/commerce/php/coding-standards/docblock/ > Include all necessary information without duplication. Example 1: ``` private ?ObjectManagerInterface $objectManager; ``` should not require a comment ``` /** @var ObjectManagerInterface|null $objectManager */ ``` because all of that information is duplicated. (Though the nullable is actually harder to read in DocBlock standard.) Example 2: ``` public function getWeakMap() : WeakMap { return $this->weakMap; } ``` This method is expressive. It is obvious what it does. It is strictly typed. There is no reason that it should need a DocBlock. This change no longer requires DocBlock in these cases: 1: member variables that have defined types 2: methods where all parameters have defined types AND method has defined return type (__construct and __destruct won't require return type, since it isn't allowed) --- .../Annotation/MethodArgumentsSniff.php | 19 +++++++++++++++++++ .../ClassPropertyPHPDocFormattingSniff.php | 4 ++++ 2 files changed, 23 insertions(+) diff --git a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php index 27ac0dfd..dafaedd3 100644 --- a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php +++ b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php @@ -568,6 +568,25 @@ 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)) { + $foundAllParameterTypes = true; + $methodParameters = $phpcsFile->getMethodParameters($stackPtr); + foreach ($methodParameters as $parameter) { + if (!$parameter['type_hint']) { + $foundAllParameterTypes = false; + break; + } + } + if ($foundAllParameterTypes) { + $methodProperties = $phpcsFile->getMethodProperties($stackPtr); + $foundReturnType = !!$methodProperties['return_type']; + if ($foundReturnType) { + return; // We don't need comment if all parameters and return value are typed + } + $methodName = $phpcsFile->getDeclarationName($stackPtr); + if ('__construct' == $methodName || '__destruct' == $methodName) { + return; // __construct and __destruct can't have return types, so they don't need comment + } + } $phpcsFile->addError('Comment block is missing', $stackPtr, 'NoCommentBlock'); return; } diff --git a/Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php b/Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php index 3732e2ac..aa08a013 100644 --- a/Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php +++ b/Magento2/Sniffs/Commenting/ClassPropertyPHPDocFormattingSniff.php @@ -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; } From 83c8668cb95ecbd6bf0de62372d8646710f86c06 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 4 Oct 2023 14:20:32 -0500 Subject: [PATCH 2/6] API class/interfaces should still require DocBlocks for methods Adding in code to check the namespace of the class/interfaces to see if it is API. Magento's API code requires method Doc Blocks because it doesn't use Reflection for types. https://developer.adobe.com/commerce/php/development/components/web-api/services/ --- .../Annotation/MethodArgumentsSniff.php | 70 ++++++++++++++----- 1 file changed, 52 insertions(+), 18 deletions(-) diff --git a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php index dafaedd3..e272fd22 100644 --- a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php +++ b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php @@ -568,24 +568,8 @@ 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)) { - $foundAllParameterTypes = true; - $methodParameters = $phpcsFile->getMethodParameters($stackPtr); - foreach ($methodParameters as $parameter) { - if (!$parameter['type_hint']) { - $foundAllParameterTypes = false; - break; - } - } - if ($foundAllParameterTypes) { - $methodProperties = $phpcsFile->getMethodProperties($stackPtr); - $foundReturnType = !!$methodProperties['return_type']; - if ($foundReturnType) { - return; // We don't need comment if all parameters and return value are typed - } - $methodName = $phpcsFile->getDeclarationName($stackPtr); - if ('__construct' == $methodName || '__destruct' == $methodName) { - return; // __construct and __destruct can't have return types, so they don't need comment - } + if (!$this->checkIfMethodNeedsDocBlock($phpcsFile, $stackPtr)) { + return; } $phpcsFile->addError('Comment block is missing', $stackPtr, 'NoCommentBlock'); return; @@ -636,6 +620,56 @@ 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; + } + $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; + } + } + return false; + } + + /** + * Check if namespace contains API + * + * @param File $phpcsFile + * @return bool + */ + private function checkIfNamespaceContainsApi(File $phpcsFile) : bool + { + $namespaceStackPtr = $phpcsFile->findNext(T_NAMESPACE, 0); + $tokens = $phpcsFile->getTokens(); + for ($index = $namespaceStackPtr; 'T_SEMICOLON' !== $tokens[$index]['type']; $index++) { + if ('T_STRING' === $tokens[$index]['type'] && 'Api' === $tokens[$index]['content']) { + return true; + } + } + return false; + } + /** * Validates function params format consistency. * From b442f9a964b585f8450aef786ccfce6117379a61 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 4 Oct 2023 14:58:12 -0500 Subject: [PATCH 3/6] Added maximum 5 CyclomaticComplexity for alowing no DocBlock --- .../Annotation/MethodArgumentsSniff.php | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php index e272fd22..f8b1e675 100644 --- a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php +++ b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php @@ -38,6 +38,8 @@ class MethodArgumentsSniff implements Sniff 'self' ]; + private const MAXIMUM_COMPLEXITY_ALLOWED_FOR_NO_DOCBLOCK = 5; + /** * @inheritdoc */ @@ -649,7 +651,8 @@ private function checkIfMethodNeedsDocBlock(File $phpcsFile, $stackPtr) : bool return true; } } - return false; + $complexity = $this->getMethodComplexity($phpcsFile, $stackPtr); + return $complexity > static::MAXIMUM_COMPLEXITY_ALLOWED_FOR_NO_DOCBLOCK; } /** @@ -670,6 +673,42 @@ private function checkIfNamespaceContainsApi(File $phpcsFile) : bool 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. * From 38f565c298ba6ec30110f82113dab72456b57ced Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Mon, 18 Dec 2023 11:39:08 -0600 Subject: [PATCH 4/6] API class/interfaces should still require DocBlocks for methods * Updated unit tests to cover new functionality reguarding when we can safely skip requiring DockBlocks * Fix MethodArgumentsSniff to work on PHP files without namespaces --- .../Annotation/MethodArgumentsSniff.php | 9 +++- .../Annotation/MethodArgumentsUnitTest.inc | 54 ++++++++++++++++++- .../Annotation/MethodArgumentsUnitTest.php | 4 +- .../ClassPropertyPHPDocFormattingUnitTest.inc | 17 ++++++ 4 files changed, 80 insertions(+), 4 deletions(-) diff --git a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php index f8b1e675..07294a54 100644 --- a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php +++ b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php @@ -664,8 +664,15 @@ private function checkIfMethodNeedsDocBlock(File $phpcsFile, $stackPtr) : bool private function checkIfNamespaceContainsApi(File $phpcsFile) : bool { $namespaceStackPtr = $phpcsFile->findNext(T_NAMESPACE, 0); + if (!is_int($namespaceStackPtr)) { + return false; + } $tokens = $phpcsFile->getTokens(); - for ($index = $namespaceStackPtr; 'T_SEMICOLON' !== $tokens[$index]['type']; $index++) { + for ( + $index = $namespaceStackPtr; + array_key_exists($index, $tokens) && 'T_SEMICOLON' !== $tokens[$index]['type']; + $index++ + ) { if ('T_STRING' === $tokens[$index]['type'] && 'Api' === $tokens[$index]['content']) { return true; } diff --git a/Magento2/Tests/Annotation/MethodArgumentsUnitTest.inc b/Magento2/Tests/Annotation/MethodArgumentsUnitTest.inc index 6694c8cc..d11bbc88 100644 --- a/Magento2/Tests/Annotation/MethodArgumentsUnitTest.inc +++ b/Magento2/Tests/Annotation/MethodArgumentsUnitTest.inc @@ -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() { 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; } diff --git a/Magento2/Tests/Annotation/MethodArgumentsUnitTest.php b/Magento2/Tests/Annotation/MethodArgumentsUnitTest.php index b47c4159..017a5e12 100644 --- a/Magento2/Tests/Annotation/MethodArgumentsUnitTest.php +++ b/Magento2/Tests/Annotation/MethodArgumentsUnitTest.php @@ -18,7 +18,9 @@ public function getErrorList() 12 => 1, 21 => 1, 32 => 1, - 50 => 1 + 68 => 1, + 73 => 1, + 78 => 1, ]; } diff --git a/Magento2/Tests/Commenting/ClassPropertyPHPDocFormattingUnitTest.inc b/Magento2/Tests/Commenting/ClassPropertyPHPDocFormattingUnitTest.inc index cde71269..153c0823 100644 --- a/Magento2/Tests/Commenting/ClassPropertyPHPDocFormattingUnitTest.inc +++ b/Magento2/Tests/Commenting/ClassPropertyPHPDocFormattingUnitTest.inc @@ -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; + } From 59b5d46f91e11c071821c4fd2b26ebe85377a541 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Mon, 18 Dec 2023 12:42:17 -0600 Subject: [PATCH 5/6] Adding phpcs:ignore for for loop in MethodArgumentsSniff --- Magento2/Sniffs/Annotation/MethodArgumentsSniff.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php index 07294a54..b0be59ec 100644 --- a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php +++ b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php @@ -668,10 +668,14 @@ private function checkIfNamespaceContainsApi(File $phpcsFile) : bool return false; } $tokens = $phpcsFile->getTokens(); + // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterOpen,Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed for ( + // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterFirst $index = $namespaceStackPtr; + // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterSecond array_key_exists($index, $tokens) && 'T_SEMICOLON' !== $tokens[$index]['type']; $index++ + // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeClose ) { if ('T_STRING' === $tokens[$index]['type'] && 'Api' === $tokens[$index]['content']) { return true; From 17e120ff553a9b5123c3b11602cdb98491293fe6 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Thu, 21 Dec 2023 20:54:43 -0600 Subject: [PATCH 6/6] removing array_key_exists from for loop to avoid ignoring phpcs --- Magento2/Sniffs/Annotation/MethodArgumentsSniff.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php index b0be59ec..f8751e84 100644 --- a/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php +++ b/Magento2/Sniffs/Annotation/MethodArgumentsSniff.php @@ -668,15 +668,7 @@ private function checkIfNamespaceContainsApi(File $phpcsFile) : bool return false; } $tokens = $phpcsFile->getTokens(); - // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterOpen,Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed - for ( - // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterFirst - $index = $namespaceStackPtr; - // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterSecond - array_key_exists($index, $tokens) && 'T_SEMICOLON' !== $tokens[$index]['type']; - $index++ - // phpcs:ignore Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeClose - ) { + for ($index = $namespaceStackPtr; 'T_SEMICOLON' !== ($tokens[$index]['type'] ?? 'T_SEMICOLON'); $index++) { if ('T_STRING' === $tokens[$index]['type'] && 'Api' === $tokens[$index]['content']) { return true; }