MC-19366: Fixes that enum values with directives would be parsed as expected and would thus lead to false positives

Author: jean-bernard-valentaten

Magento2/Sniffs/GraphQL/AbstractGraphQLSniff.php

@@ -43,6 +43,30 @@ protected function isSnakeCase($name, $upperCase = false)
         return preg_match($pattern, $name);
     }
 
+    /**
+     * Returns the pointer to the last token of a directive if the token at <var>$startPointer</var> starts a directive.
+     *
+     * @param array $tokens
+     * @param int $startPointer
+     * @return int The end of the directive if one is found, the start pointer otherwise
+     */
+    protected function seekEndOfDirective(array $tokens, $startPointer)
+    {
+        $endPointer = $startPointer;
+
+        if ($tokens[$startPointer]['code'] === T_DOC_COMMENT_TAG) {
+            //advance to next token
+            $endPointer += 1;
+
+            //if next token is an opening parenthesis, we consume everything up to the closing parenthesis
+            if ($tokens[$endPointer + 1]['code'] === T_OPEN_PARENTHESIS) {
+                $endPointer = $tokens[$endPointer + 1]['parenthesis_closer'];
+            }
+        }
+
+        return $endPointer;
+    }
+
     /**
      * Searches for the first token that has <var>$tokenCode</var> in <var>$tokens</var> from position
      * <var>$startPointer</var> (excluded).

Magento2/Sniffs/GraphQL/ValidArgumentNameSniff.php

@@ -108,13 +108,7 @@ private function getArgumentDefinitionEndPointer($argumentDefinitionStartPointer
 
         //while next token starts a directive, we advance to the end of the directive
         while ($tokens[$endPointer + 1]['code'] === T_DOC_COMMENT_TAG) {
-            //consume next two tokens
-            $endPointer += 2;
-
-            //if next token is an opening parenthesis, we consume everything up to the closing parenthesis
-            if ($tokens[$endPointer + 1]['code'] === T_OPEN_PARENTHESIS) {
-                $endPointer = $tokens[$endPointer + 1]['parenthesis_closer'];
-            }
+            $endPointer = $this->seekEndOfDirective($tokens, $endPointer + 1);
         }
 
         return $endPointer;
@@ -181,7 +175,7 @@ private function getArguments($startPointer, $endPointer, array $tokens)
 
             switch (true) {
                 case in_array($tokenCode, $skipTypes):
-                    //NOP This is a toke that we have to skip
+                    //NOP This is a token that we have to skip
                     break;
                 case $tokenCode === T_COLON:
                     //we have reached the end of the argument name, thus we store its pointer and value

Magento2/Sniffs/GraphQL/ValidEnumValueSniff.php

@@ -3,6 +3,7 @@
  * Copyright © Magento. All rights reserved.
  * See COPYING.txt for license details.
  */
+
 namespace Magento2\Sniffs\GraphQL;
 
 use PHP_CodeSniffer\Files\File;
@@ -46,16 +47,14 @@ public function process(File $phpcsFile, $stackPtr)
 
         $values = $this->getValues($openingCurlyPointer, $closingCurlyPointer, $tokens, $phpcsFile->eolChar);
 
-        foreach ($values as $value) {
-            $pointer = $value[0];
-            $name    = $value[1];
+        foreach ($values as $pointer => $value) {
 
-            if (!$this->isSnakeCase($name, true)) {
+            if (!$this->isSnakeCase($value, true)) {
                 $type  = 'Enum value';
                 $error = '%s "%s" is not in SCREAMING_SNAKE_CASE format';
                 $data  = [
                     $type,
-                    $name,
+                    $value,
                 ];
 
                 $phpcsFile->addError($error, $pointer, 'NotScreamingSnakeCase', $data);
@@ -98,11 +97,13 @@ private function getOpeningCurlyBracketPointer($startPointer, array $tokens)
      * Finds all enum values contained in <var>$tokens</var> in range <var>$startPointer</var> to
      * <var>$endPointer</var>.
      *
+     * The returned array uses token pointers as keys and value names as values.
+     *
      * @param int $startPointer
      * @param int $endPointer
      * @param array $tokens
      * @param string $eolChar
-     * @return array[]
+     * @return array<int,string>
      */
     private function getValues($startPointer, $endPointer, array $tokens, $eolChar)
     {
@@ -112,20 +113,31 @@ private function getValues($startPointer, $endPointer, array $tokens, $eolChar)
         $skipTypes         = [T_COMMENT, T_WHITESPACE];
 
         for ($i = $startPointer + 1; $i < $endPointer; ++$i) {
-            //skip some tokens
             if (in_array($tokens[$i]['code'], $skipTypes)) {
+                //NOP This is a token that we have to skip
                 continue;
             }
-            $enumValue .= $tokens[$i]['content'];
 
-            if ($valueTokenPointer === null) {
-                $valueTokenPointer = $i;
+            //add current tokens content to enum value if we have a string
+            if ($tokens[$i]['code'] === T_STRING) {
+                $enumValue .= $tokens[$i]['content'];
+
+                //and store the pointer if we have not done it already
+                if ($valueTokenPointer === null) {
+                    $valueTokenPointer = $i;
+                }
+            }
+
+            //consume directive if we have found one
+            if ($tokens[$i]['code'] === T_DOC_COMMENT_TAG) {
+                $i = $this->seekEndOfDirective($tokens, $i);
             }
 
-            if (strpos($enumValue, $eolChar) !== false) {
-                $values[]          = [$valueTokenPointer, rtrim($enumValue, $eolChar)];
-                $enumValue         = '';
-                $valueTokenPointer = null;
+            //if current token has a line break, we have found the end of the value definition
+            if (strpos($tokens[$i]['content'], $eolChar) !== false) {
+                $values[$valueTokenPointer] = trim($enumValue);
+                $enumValue                  = '';
+                $valueTokenPointer          = null;
             }
         }
 

Magento2/Tests/GraphQL/ValidArgumentNameUnitTest.graphqls

@@ -40,7 +40,7 @@ type Foo @doc(descripton: "Foo Bar Baz") {
         validArgument: String = "My fair lady" @doc(
             # A comment
             description: "This is a valid argument, spanned over multiple lines."
-        ) @foo
+        ) @unparametrizedDirective
         validArgumentWithDefaultValue: Int = 20 @doc(description: "This is another valid argument with a default value.")
         validArgumentListType: [String] @doc(description: "This is a valid argument that uses a list type.")
         validArgumentNonNullType: String! @doc(description: "This is a valid argument that uses a non null type.")

Magento2/Tests/GraphQL/ValidEnumValueUnitTest.graphqls

@@ -5,12 +5,20 @@ enum Foo {
     VALID_SCREAMING_SNAKE_CASE_VALUE_WITH_12345_NUMBERS
     VALID_SCREAMING_SNAKE_CASE_VALUE_ENDING_WITH_NUMBER_5
     VALIDUPPERCASEVALUE
+    VALID_SCREMING_CASE_VALUE_WITH_DIRECTIVE @doc(description: "This is a valid enum value with a directive")
+    VALID_SCREMING_CASE_VALUE_WITH_TWO_DIRECTIVES @doc(
+        description: "This is a valid enum value with a directive"
+    ) @unparametrizedDirective
 
     # Invalid values
     1_INVALID_SCREAMING_SNAKE_CASE_VALUE_STARTING_WITH_NUMBER
     invalidCamelCaseValue
     InvalidCamelCaseCapsValue
     invalidlowercasevalue
+    invalidCamelCaseValueWithDirective @doc(description: "This is an invalid enum value with a directive")
+    invalidCamelCaseValueWithTwoDirectives @doc(
+        description: "This is an invalid enum value with a directive"
+    ) @unparametrizedDirective
 }
 
 # Ignored although it triggers a T_CLASS token

Magento2/Tests/GraphQL/ValidEnumValueUnitTest.php

@@ -16,10 +16,12 @@ class ValidEnumValueUnitTest extends AbstractGraphQLSniffUnitTestCase
     protected function getErrorList()
     {
         return [
-            10 => 1,
-            11 => 1,
-            12 => 1,
-            13 => 1,
+            14 => 1,
+            15 => 1,
+            16 => 1,
+            17 => 1,
+            18 => 1,
+            19 => 1,
         ];
     }