From ad1de270af2604538a637a00fb99006e883caa79 Mon Sep 17 00:00:00 2001
From: Klaus Purer <klaus.purer@protonmail.ch>
Date: Sat, 20 Apr 2024 19:40:33 +0200
Subject: [PATCH] feat(MultiLineFunctionDeclaration): Add new sniff for
 multi-line function declarations and trailing commas (#3440603)

---
 .../Functions/FunctionDeclarationSniff.php    |   2 +-
 .../MultiLineFunctionDeclarationSniff.php     | 129 ++++++++++++++++++
 coder_sniffer/Drupal/ruleset.xml              |  18 ---
 .../MultiLineFunctionDeclarationUnitTest.inc  |  51 +++++++
 ...iLineFunctionDeclarationUnitTest.inc.fixed |  55 ++++++++
 .../MultiLineFunctionDeclarationUnitTest.php  |  66 +++++++++
 tests/Drupal/bad/BadUnitTest.php              |  18 ++-
 tests/Drupal/bad/bad.php.fixed                |   2 +-
 tests/Drupal/good/good.php                    |   2 +-
 9 files changed, 321 insertions(+), 22 deletions(-)
 create mode 100644 coder_sniffer/Drupal/Sniffs/Functions/MultiLineFunctionDeclarationSniff.php
 create mode 100644 tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc
 create mode 100644 tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed
 create mode 100644 tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.php

diff --git a/coder_sniffer/Drupal/Sniffs/Functions/FunctionDeclarationSniff.php b/coder_sniffer/Drupal/Sniffs/Functions/FunctionDeclarationSniff.php
index b4cdcdd0..923ff9fe 100644
--- a/coder_sniffer/Drupal/Sniffs/Functions/FunctionDeclarationSniff.php
+++ b/coder_sniffer/Drupal/Sniffs/Functions/FunctionDeclarationSniff.php
@@ -17,7 +17,7 @@
  * before the opening parenthesis.
  *
  * @deprecated in Coder 8.x, will be removed in Coder 9.x.
- * Squiz.Functions.MultiLineFunctionDeclaration is used instead, see ruleset.xml.
+ * MultiLineFunctionDeclarationSniff is used instead.
  *
  * @category PHP
  * @package  PHP_CodeSniffer
diff --git a/coder_sniffer/Drupal/Sniffs/Functions/MultiLineFunctionDeclarationSniff.php b/coder_sniffer/Drupal/Sniffs/Functions/MultiLineFunctionDeclarationSniff.php
new file mode 100644
index 00000000..897c4d7e
--- /dev/null
+++ b/coder_sniffer/Drupal/Sniffs/Functions/MultiLineFunctionDeclarationSniff.php
@@ -0,0 +1,129 @@
+<?php
+/**
+ * \Drupal\Sniffs\Functions\MultiLineFunctionDeclarationSniff
+ *
+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @link     http://pear.php.net/package/PHP_CodeSniffer
+ */
+
+namespace Drupal\Sniffs\Functions;
+
+use PHP_CodeSniffer\Standards\Generic\Sniffs\Functions\OpeningFunctionBraceKernighanRitchieSniff;
+use PHP_CodeSniffer\Standards\PEAR\Sniffs\Functions\FunctionDeclarationSniff as PearFunctionDeclarationSniff;
+use PHP_CodeSniffer\Standards\Squiz\Sniffs\Functions\MultiLineFunctionDeclarationSniff as SquizFunctionDeclarationSniff;
+use PHP_CodeSniffer\Util\Tokens;
+
+/**
+ * Multi-line function declarations need to have a trailing comma on the last
+ * parameter. Modified from Squiz, whenever there is a function declaration
+ * closing parenthesis on a new line we treat it as multi-line.
+ *
+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @link     http://pear.php.net/package/PHP_CodeSniffer
+ */
+class MultiLineFunctionDeclarationSniff extends SquizFunctionDeclarationSniff
+{
+
+
+    /**
+     * The number of spaces code should be indented.
+     *
+     * @var integer
+     */
+    public $indent = 2;
+
+
+    /**
+     * Processes single-line declarations.
+     *
+     * Just uses the Generic Kernighan Ritchie sniff.
+     *
+     * @param \PHP_CodeSniffer\Files\File      $phpcsFile The file being scanned.
+     * @param int                              $stackPtr  The position of the current token
+     *                                                    in the stack passed in $tokens.
+     * @param array<int, array<string, mixed>> $tokens    The stack of tokens that make up
+     *                                                    the file.
+     *
+     * @return void
+     */
+    public function processSingleLineDeclaration($phpcsFile, $stackPtr, $tokens)
+    {
+        $sniff = new OpeningFunctionBraceKernighanRitchieSniff();
+        $sniff->checkClosures = true;
+        $sniff->process($phpcsFile, $stackPtr);
+
+    }//end processSingleLineDeclaration()
+
+
+    /**
+     * Determine if this is a multi-line function declaration.
+     *
+     * @param \PHP_CodeSniffer\Files\File      $phpcsFile   The file being scanned.
+     * @param int                              $stackPtr    The position of the current token
+     *                                                      in the stack passed in $tokens.
+     * @param int                              $openBracket The position of the opening bracket
+     *                                                      in the stack passed in $tokens.
+     * @param array<int, array<string, mixed>> $tokens      The stack of tokens that make up
+     *                                                      the file.
+     *
+     * @return bool
+     */
+    public function isMultiLineDeclaration($phpcsFile, $stackPtr, $openBracket, $tokens)
+    {
+        $function = $tokens[$stackPtr];
+        if ($tokens[$function['parenthesis_opener']]['line'] === $tokens[$function['parenthesis_closer']]['line']) {
+            return false;
+        }
+
+        return true;
+
+    }//end isMultiLineDeclaration()
+
+
+    /**
+     * Processes multi-line declarations.
+     *
+     * @param \PHP_CodeSniffer\Files\File      $phpcsFile The file being scanned.
+     * @param int                              $stackPtr  The position of the current token
+     *                                                    in the stack passed in $tokens.
+     * @param array<int, array<string, mixed>> $tokens    The stack of tokens that make up
+     *                                                    the file.
+     *
+     * @return void
+     */
+    public function processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens)
+    {
+        // We do everything the grandparent sniff does, and a bit more.
+        PearFunctionDeclarationSniff::processMultiLineDeclaration($phpcsFile, $stackPtr, $tokens);
+
+        $openBracket = $tokens[$stackPtr]['parenthesis_opener'];
+        $this->processBracket($phpcsFile, $openBracket, $tokens, 'function');
+
+        // Trailing commas on the last function parameter are only possible in
+        // PHP 8.0+.
+        if (version_compare(PHP_VERSION, '8.0.0') < 0) {
+            return;
+        }
+
+        $function = $tokens[$stackPtr];
+
+        $lastTrailingComma = $phpcsFile->findPrevious(
+            Tokens::$emptyTokens,
+            ($function['parenthesis_closer'] - 1),
+            $function['parenthesis_opener'],
+            true
+        );
+        if ($tokens[$lastTrailingComma]['code'] !== T_COMMA) {
+            $error = 'Multi-line function declarations must have a trailing comma after the last parameter';
+            $fix   = $phpcsFile->addFixableError($error, $lastTrailingComma, 'MissingTrailingComma');
+            if ($fix === true) {
+                $phpcsFile->fixer->addContent($lastTrailingComma, ',');
+            }
+        }
+
+    }//end processMultiLineDeclaration()
+
+
+}//end class
diff --git a/coder_sniffer/Drupal/ruleset.xml b/coder_sniffer/Drupal/ruleset.xml
index 99baff4e..73bb669d 100644
--- a/coder_sniffer/Drupal/ruleset.xml
+++ b/coder_sniffer/Drupal/ruleset.xml
@@ -53,11 +53,6 @@
  <rule ref="Generic.Functions.FunctionCallArgumentSpacing.NoSpaceAfterComma">
   <severity>0</severity>
  </rule>
- <rule ref="Generic.Functions.OpeningFunctionBraceKernighanRitchie">
-  <properties>
-   <property name="checkClosures" value="true"/>
-  </properties>
- </rule>
 
  <rule ref="Generic.NamingConventions.ConstructorName" />
  <rule ref="Generic.NamingConventions.UpperCaseConstantName" />
@@ -255,19 +250,6 @@
   <severity>0</severity>
  </rule>
 
- <rule ref="Squiz.Functions.MultiLineFunctionDeclaration">
-  <properties>
-   <property name="indent" value="2"/>
-  </properties>
- </rule>
- <!-- Disable some errors that are already coverd by other sniffs. -->
- <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.BraceOnSameLine">
-  <severity>0</severity>
- </rule>
- <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.ContentAfterBrace">
-  <severity>0</severity>
- </rule>
-
  <rule ref="Squiz.PHP.LowercasePHPFunctions" />
  <rule ref="Squiz.PHP.NonExecutableCode" />
  <rule ref="Squiz.Strings.ConcatenationSpacing">
diff --git a/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc b/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc
new file mode 100644
index 00000000..79d9add7
--- /dev/null
+++ b/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc
@@ -0,0 +1,51 @@
+<?php
+
+/**
+ * @file
+ * Some description.
+ */
+
+/**
+ * Test.
+ */
+function missing_trailing_comma(
+  $a,
+  $b
+) {
+
+}
+
+$foo = 1;
+$bar = 2;
+$x = function (
+  $a,
+  $b
+) use (
+  $foo,
+  $bar
+) {
+
+};
+
+/**
+ *
+ */
+class Test {
+
+  /**
+   * Test.
+   */
+  public function lookupSource(string $key, string $migrationNames = '', array $options = [
+    'all' => NULL,
+    'group' => NULL,
+  ]): void {
+  }
+
+}
+
+$x = function (
+  $a,
+  $b,
+) use ($foo, $bar) {
+
+};
diff --git a/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed b/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed
new file mode 100644
index 00000000..f35c62e8
--- /dev/null
+++ b/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.inc.fixed
@@ -0,0 +1,55 @@
+<?php
+
+/**
+ * @file
+ * Some description.
+ */
+
+/**
+ * Test.
+ */
+function missing_trailing_comma(
+  $a,
+  $b,
+) {
+
+}
+
+$foo = 1;
+$bar = 2;
+$x = function (
+  $a,
+  $b,
+) use (
+  $foo,
+  $bar
+) {
+
+};
+
+/**
+ *
+ */
+class Test {
+
+  /**
+   * Test.
+   */
+  public function lookupSource(
+    string $key,
+    string $migrationNames = '',
+    array $options = [
+      'all' => NULL,
+      'group' => NULL,
+    ],
+  ): void {
+  }
+
+}
+
+$x = function (
+  $a,
+  $b,
+) use ($foo, $bar) {
+
+};
diff --git a/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.php b/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.php
new file mode 100644
index 00000000..fa49aa50
--- /dev/null
+++ b/tests/Drupal/Functions/MultiLineFunctionDeclarationUnitTest.php
@@ -0,0 +1,66 @@
+<?php
+
+namespace Drupal\Test\Functions;
+
+use Drupal\Test\CoderSniffUnitTest;
+
+class MultiLineFunctionDeclarationUnitTest extends CoderSniffUnitTest
+{
+
+
+    /**
+     * Returns the lines where errors should occur.
+     *
+     * The key of the array should represent the line number and the value
+     * should represent the number of errors that should occur on that line.
+     *
+     * @param string $testFile The name of the file being tested.
+     *
+     * @return array<int, int>
+     */
+    protected function getErrorList(string $testFile): array
+    {
+        return [
+            13 => 1,
+            22 => 1,
+            38 => 3,
+            41 => 2,
+        ];
+
+    }//end getErrorList()
+
+
+    /**
+     * Returns the lines where warnings should occur.
+     *
+     * The key of the array should represent the line number and the value
+     * should represent the number of warnings that should occur on that line.
+     *
+     * @param string $testFile The name of the file being tested.
+     *
+     * @return array<int, int>
+     */
+    protected function getWarningList(string $testFile): array
+    {
+        return [];
+
+    }//end getWarningList()
+
+
+    /**
+     * Skip this test on PHP versions lower than 8 because the syntax is not allowed there.
+     *
+     * @return bool
+     */
+    protected function shouldSkipTest()
+    {
+        if (version_compare(PHP_VERSION, '8.0.0') < 0) {
+            return true;
+        }
+
+        return false;
+
+    }//end shouldSkipTest()
+
+
+}//end class
diff --git a/tests/Drupal/bad/BadUnitTest.php b/tests/Drupal/bad/BadUnitTest.php
index 5ccfb4d3..090430c6 100644
--- a/tests/Drupal/bad/BadUnitTest.php
+++ b/tests/Drupal/bad/BadUnitTest.php
@@ -382,7 +382,7 @@ protected function getErrorList(string $testFile): array
                 836 => 1,
                 838 => 1,
                 849 => 2,
-                860 => 1,
+                860 => 2,
                 864 => 2,
             ];
         }//end switch
@@ -480,4 +480,20 @@ protected function checkAllSniffCodes()
     }//end checkAllSniffCodes()
 
 
+    /**
+     * Skip this test on PHP versions lower than 8 because of MultiLineTrailingCommaSniff.
+     *
+     * @return bool
+     */
+    protected function shouldSkipTest()
+    {
+        if (version_compare(PHP_VERSION, '8.0.0') < 0) {
+            return true;
+        }
+
+        return false;
+
+    }//end shouldSkipTest()
+
+
 }//end class
diff --git a/tests/Drupal/bad/bad.php.fixed b/tests/Drupal/bad/bad.php.fixed
index c68773c0..26abaa2e 100644
--- a/tests/Drupal/bad/bad.php.fixed
+++ b/tests/Drupal/bad/bad.php.fixed
@@ -911,7 +911,7 @@ class ScopeKeyword {
  */
 function test29(
   int $a,
-  string $b
+  string $b,
 ) {
   echo "Hello";
 }
diff --git a/tests/Drupal/good/good.php b/tests/Drupal/good/good.php
index 0076a3c8..3c9ecacb 100644
--- a/tests/Drupal/good/good.php
+++ b/tests/Drupal/good/good.php
@@ -1476,7 +1476,7 @@ function test18(
   CacheTagsInvalidatorInterface $cache_invalidator,
   ModuleHandlerInterface $module_handler,
   EntityFieldManagerInterface $entity_field_manager,
-  EntityTypeBundleInfoInterface $entity_type_bundle_info
+  EntityTypeBundleInfoInterface $entity_type_bundle_info,
 ) {
   return 0;
 }