Merge pull request #67 from Yoast/JRF/improve-ifelse-sniff

IfElseDeclaration: Various improvements
Yoast · Jan 25, 2018 · fad5e7c · fad5e7c
2 parents 45d793b + 72c0b93
commit fad5e7c
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 48 deletions.
diff --git a/Yoast/Sniffs/ControlStructures/IfElseDeclarationSniff.php b/Yoast/Sniffs/ControlStructures/IfElseDeclarationSniff.php
@@ -11,6 +11,7 @@
 
 use PHP_CodeSniffer\Sniffs\Sniff;
 use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Util\Tokens;
 
 /**
  * Verifies that else statements are on a new line.
@@ -48,56 +49,52 @@ public function register() {
 	 * @return void
 	 */
 	public function process( File $phpcsFile, $stackPtr ) {
-		$tokens     = $phpcsFile->getTokens();
-		$has_errors = 0;
+		$tokens = $phpcsFile->getTokens();
 
 		if ( isset( $tokens[ $stackPtr ]['scope_opener'] ) ) {
 			$scope_open = $tokens[ $stackPtr ]['scope_opener'];
 		}
-		elseif ( $tokens[ ( $stackPtr + 2 ) ]['code'] === T_IF && isset( $tokens[ ( $stackPtr + 2 ) ]['scope_opener'] ) ) {
-			$scope_open = $tokens[ ( $stackPtr + 2 ) ]['scope_opener'];
+		else {
+			// Deal with "else if".
+			$next = $phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
+			if ( $tokens[ $next ]['code'] === T_IF && isset( $tokens[ $next ]['scope_opener'] ) ) {
+				$scope_open = $tokens[ $next ]['scope_opener'];
+			}
 		}
 
-		if ( isset( $scope_open ) && $tokens[ $scope_open ]['code'] !== T_COLON ) { // Ignore alternative syntax.
-
-			$previous = $phpcsFile->findPrevious( T_CLOSE_CURLY_BRACKET, $stackPtr, null, false );
-
-			if ( $tokens[ $previous ]['line'] === $tokens[ $stackPtr ]['line'] ) {
-				$error = 'else(if) statement must be on a new line';
-				$phpcsFile->addError( $error, $stackPtr, 'NewLine' );
-				$has_errors++;
-				unset( $error );
-			}
+		if ( ! isset( $scope_open ) || $tokens[ $scope_open ]['code'] === T_COLON ) {
+			// No scope opener found or alternative syntax (not our concern).
+			return;
+		}
 
-			$start        = ( $previous + 1 );
-			$other_start  = null;
-			$other_length = 0;
-			for ( $i = $start; $i < $stackPtr; $i++ ) {
-				if ( $tokens[ $i ]['code'] !== T_COMMENT && $tokens[ $i ]['code'] !== T_WHITESPACE ) {
-					if ( ! isset( $other_start ) ) {
-						$other_start = $i;
-					}
-					$other_length++;
-				}
-			}
-			unset( $i );
+		$previous_scope_closer = $phpcsFile->findPrevious( T_CLOSE_CURLY_BRACKET, ( $stackPtr - 1 ) );
 
-			if ( isset( $other_start, $other_length ) ) {
-				$error = 'Nothing but whitespace and comments allowed between closing bracket and else(if) statement, found "%s"';
-				$data  = $phpcsFile->getTokensAsString( $other_start, $other_length );
-				$phpcsFile->addError( $error, $stackPtr, 'StatementFound', $data );
-				$has_errors++;
-				unset( $error, $data, $other_start, $other_length );
+		if ( $tokens[ $previous_scope_closer ]['line'] === $tokens[ $stackPtr ]['line'] ) {
+			$phpcsFile->addError(
+				'%s statement must be on a new line',
+				$stackPtr,
+				'NewLine',
+				array( ucfirst( $tokens[ $stackPtr ]['content'] ) )
+			);
+		}
+		elseif ( $tokens[ $previous_scope_closer ]['column'] !== $tokens[ $stackPtr ]['column'] ) {
+			$phpcsFile->addError(
+				'%s statement not aligned with previous part of the control structure',
+				$stackPtr,
+				'Alignment',
+				array( ucfirst( $tokens[ $stackPtr ]['content'] ) )
+			);
+		}
 
-			}
+		$previous_non_empty = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
 
-			if ( $has_errors === 0 ) {
-				if ( $tokens[ $previous ]['column'] !== $tokens[ $stackPtr ]['column'] ) {
-					$error = 'else(if) statement not aligned with previous part of the control structure';
-					$phpcsFile->addError( $error, $stackPtr, 'Alignment' );
-					unset( $error );
-				}
-			}
+		if ( $previous_scope_closer !== $previous_non_empty ) {
+			$error = 'Nothing but whitespace and comments allowed between closing bracket and %s statement, found "%s"';
+			$data  = array(
+				$tokens[ $stackPtr ]['content'],
+				trim( $phpcsFile->getTokensAsString( ( $previous_scope_closer + 1 ), ( $stackPtr - ( $previous_scope_closer + 1 ) ) ) ),
+			);
+			$phpcsFile->addError( $error, $stackPtr, 'StatementFound', $data );
 		}
 
 	}//end process()

diff --git a/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.inc b/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.inc
@@ -8,12 +8,15 @@ else {
 	// ...
 }
 
+// Ok.
 if ( false ):
+elseif:
+else if:
+else /* comment */
+if:
 else:
 endif;
 
-
-// Bad.
 if ( true ) {
 	// code.
 } else { // Bad - else should be on the next line.
@@ -24,8 +27,60 @@ if ( true ) {
 	// code.
 } elseif( 1 ) { // Bad - elseif should be on the next line.
 	// ...
-} elseif( 2 ) { // Bad - elseif should be on the next line.
+} else if       (2 // Bad - else if should be on the next line.
+	|| 6
+) {
+	// ...
+} else // Bad - else if should be on the next line.
+if (2) {
 	// ...
-} elseif( 3 ) { // Bad - elseif should be on the next line.
+} else ( 3 ) { // Bad - else should be on the next line.
+	// ...
+}
+
+if ( true ) {
+	// code.
+}
+	else { // Bad - else not aligned with if.
 	// ...
+	}
+
+				if ( true ) {
+					// code.
+				}
+	else { // Bad - else not aligned with if.
+	// ...
+	}
+
+if ( true ) {
+	// code.
+}
+
+
+else { // OK - multiple blank lines between if/else are ignored, not the concern of this sniff.
+	// ...
+}
+
+if ( true ) {
+	// code.
+}
+// Some comment - this comment should be ignored.
+else {
+	// ...
+}
+
+if ( true ) {
+	// code.
+}
+	/* phpcs:ignore Standard.Category.Sniffname -- for reasons. */
+	else { // Bad - else not aligned with if.
+		// ...
+	}
+
+if ( true ) {
+	// code.
 }
+?> <!-- This would be a parse error anyway. --> <?php
+	else { // Bad.
+		// ...
+	}
diff --git a/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.php b/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.php
@@ -27,10 +27,15 @@ class IfElseDeclarationUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getErrorList() {
 		return array(
-			19 => 1,
-			25 => 1,
-			27 => 1,
-			29 => 1,
+			22 => 1,
+			28 => 1,
+			30 => 1,
+			34 => 1,
+			37 => 1,
+			44 => 1,
+			51 => 1,
+			76 => 1,
+			84 => 2,
 		);
 
 	} // end getErrorList()