Repository: WordPress/WordPress-Coding-Standards (php)

Issue:
snake_case checking doesn't work with characters outside a-z0-9 range
## Bug Description

I get `Variable "$lähtöaika" is not in valid snake_case format, try "$lähtöaika"` error, when clearly the variable name is in valid snake_case format.

## Minimal Code Snippet
```php
$lähtöaika = true;
```

This is because the `isSnakeCase()` function only allows characters within `[a-z0-9_]`. However, this is not the whole set of allowed characters in PHP variable names. Quoting from the [PHP manual](https://www.php.net/manual/en/language.variables.basics.php):

> Variable names follow the same rules as other labels in PHP. A valid variable name starts with a letter or underscore, followed by any number of letters, numbers, or underscores. As a regular expression, it would be expressed thus: `^[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*$`

If the `isSnakeCase()` function is changed to use this regular expression (minus the uppercase A-Z), the problem is gone:

```php
public static function isSnakeCase( $var_name ) {
	return (bool) preg_match( '/^[a-z_\x80-\xff][a-z0-9_\x80-\xff]*$/', $var_name );
}
```

This is not perfect, as the 128-255 character range contains uppercase letters, so some non-snake variable names would still be accepted: `$lähtÖaika` would not trigger an error. I'm not sure if checking for these cases would be worth making the regex a lot more complicated, though.

## Error Code
`WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase`

## Environment

| Question               | Answer
| ------------------------| -------
| PHP version             | 7.3.11
| PHP_CodeSniffer version | 3.4.2
| WPCS version            | 2.2.1
| WPCS install type       | global, VS Code plugin
| IDE (if relevant)       | VS Code 1.44.2

## Tested Against `develop` branch?
- [X] I have verified the issue still exists in the `develop` branch of WPCS.


Pull request:
NamingConventions/Valid(Function|Variable)Name: bug fix for error determination
This commit fixes two, closely related, issues:
1. The "is a name in snake case ?" determination was done in three slightly different ways between the `ValidFunctionName` sniff and the `ValidVariableName` sniff and the `SnakeCaseHelper::get_suggestion()` method, which meant that a name which would be regarded valid snakecase for the one, may not be regarded as valid snakecase for the other.
    It could also result in the suggested name being the same as the existing name.
2. The "is a name in snake case ?" as well as the suggestion determination didn't take non-ASCII characters into account consistently.

While PHPCSUtils will at some point get better methods for handling these things, for now, we can still make a quick fix to improve this situation.

To that end, I've made the following changes:
* Instead of having each sniff determine whether or not something is valid snakecase itself, let the sniffs retrieve the suggested alternative name and compare the original name against that and only throw an error if the suggestion is not the same as the original name. This also allows for removing some code related to potential leading underscores (which looks to be redundant anyway).
* Improved the creation of the suggestion to handle non-ASCII names better (in as far as possible).

Includes:
* The `Mbstring` extension has been added as a suggested extension to the `composer.json` file.
* Adding one extra test run to the CI workflow which runs the tests without the Mbstring extension to safeguard the fallback code.

Includes unit tests.

Fixes #1891

---

Note: as of PHP 8.2, `strtolower()` no longer acts on non-ASCII characters, which largely got rid of the issue already and made for some interesting debugging. Unfortunately, we cannot rely on everyone being on PHP 8.2...

Patch:
diff --git a/WordPress/Helpers/SnakeCaseHelper.php b/WordPress/Helpers/SnakeCaseHelper.php
index 45925941..a5104867 100644
--- a/WordPress/Helpers/SnakeCaseHelper.php
+++ b/WordPress/Helpers/SnakeCaseHelper.php
@@ -9,6 +9,8 @@
 
 namespace WordPressCS\WordPress\Helpers;
 
+use PHPCSUtils\BackCompat\Helper;
+
 /**
  * Helper utilities for checking if a name is in snake_case.
  *
@@ -40,7 +42,17 @@ final class SnakeCaseHelper {
 	 */
 	public static function get_suggestion( $name ) {
 		$suggested = preg_replace( '`(?<!_|^)([A-Z])`', '_$1', $name );
-		$suggested = strtolower( $suggested );
+
+		if ( preg_match( '`^[a-z0-9_]+$i`', $suggested ) === 1 ) {
+			// If the name only contains ASCII characters, we can safely lowercase it.
+			$suggested = strtolower( $suggested );
+		} elseif ( function_exists( 'mb_strtolower' ) ) {
+			$suggested = mb_strtolower( $suggested, Helper::getEncoding() );
+		} else {
+			// If the name contains non-ASCII chars and Mbstring is not available, only transliterate the ASCII chars.
+			$suggested = strtr( $suggested, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', 'abcdefghijklmnopqrstuvwxyz' );
+		}
+
 		return $suggested;
 	}
 }
diff --git a/WordPress/Sniffs/NamingConventions/ValidFunctionNameSniff.php b/WordPress/Sniffs/NamingConventions/ValidFunctionNameSniff.php
index 368d2fd0..e927563d 100644
--- a/WordPress/Sniffs/NamingConventions/ValidFunctionNameSniff.php
+++ b/WordPress/Sniffs/NamingConventions/ValidFunctionNameSniff.php
@@ -100,7 +100,6 @@ class ValidFunctionNameSniff extends Sniff {
 	 * @return void
 	 */
 	protected function process_function_declaration( $stackPtr, $functionName ) {
-
 		// PHP magic functions are exempt from our rules.
 		if ( FunctionDeclarations::isMagicFunctionName( $functionName ) === true ) {
 			return;
@@ -113,11 +112,12 @@ class ValidFunctionNameSniff extends Sniff {
 			$this->phpcsFile->addError( $error, $stackPtr, 'FunctionDoubleUnderscore', $errorData );
 		}
 
-		if ( strtolower( $functionName ) !== $functionName ) {
+		$suggested_name = SnakeCaseHelper::get_suggestion( $functionName );
+		if ( $suggested_name !== $functionName ) {
 			$error     = 'Function name "%s" is not in snake case format, try "%s"';
 			$errorData = array(
 				$functionName,
-				SnakeCaseHelper::get_suggestion( $functionName ),
+				$suggested_name,
 			);
 			$this->phpcsFile->addError( $error, $stackPtr, 'FunctionNameInvalid', $errorData );
 		}
@@ -176,12 +176,13 @@ class ValidFunctionNameSniff extends Sniff {
 		}
 
 		// Check for all lowercase.
-		if ( strtolower( $methodName ) !== $methodName ) {
+		$suggested_name = SnakeCaseHelper::get_suggestion( $methodName );
+		if ( $suggested_name !== $methodName ) {
 			$error     = 'Method name "%s" in class %s is not in snake case format, try "%s"';
 			$errorData = array(
 				$methodName,
 				$className,
-				SnakeCaseHelper::get_suggestion( $methodName ),
+				$suggested_name,
 			);
 			$this->phpcsFile->addError( $error, $stackPtr, 'MethodNameInvalid', $errorData );
 		}
diff --git a/WordPress/Sniffs/NamingConventions/ValidVariableNameSniff.php b/WordPress/Sniffs/NamingConventions/ValidVariableNameSniff.php
index cb8c7471..7886ab93 100644
--- a/WordPress/Sniffs/NamingConventions/ValidVariableNameSniff.php
+++ b/WordPress/Sniffs/NamingConventions/ValidVariableNameSniff.php
@@ -137,19 +137,16 @@ class ValidVariableNameSniff extends PHPCS_AbstractVariableSniff {
 				if ( \T_OPEN_PARENTHESIS !== $tokens[ $bracket ]['code'] ) {
 					$obj_var_name = $tokens[ $var ]['content'];
 
-					// There is no way for us to know if the var is public or
-					// private, so we have to ignore a leading underscore if there is
-					// one and just check the main part of the variable name.
-					$original_var_name = $obj_var_name;
-					if ( '_' === substr( $obj_var_name, 0, 1 ) ) {
-						$obj_var_name = substr( $obj_var_name, 1 );
+					if ( isset( $this->allowed_mixed_case_member_var_names[ $obj_var_name ] ) ) {
+						return;
 					}
 
-					if ( ! isset( $this->allowed_mixed_case_member_var_names[ $obj_var_name ] ) && self::isSnakeCase( $obj_var_name ) === false ) {
+					$suggested_name = SnakeCaseHelper::get_suggestion( $obj_var_name );
+					if ( $suggested_name !== $obj_var_name ) {
 						$error = 'Object property "$%s" is not in valid snake_case format, try "$%s"';
 						$data  = array(
-							$original_var_name,
-							SnakeCaseHelper::get_suggestion( $original_var_name ),
+							$obj_var_name,
+							$suggested_name,
 						);
 						$phpcs_file->addError( $error, $var, 'UsedPropertyNotSnakeCase', $data );
 					}
@@ -165,15 +162,8 @@ class ValidVariableNameSniff extends PHPCS_AbstractVariableSniff {
 			$in_class = true;
 		}
 
-		// There is no way for us to know if the var is public or private,
-		// so we have to ignore a leading underscore if there is one and just
-		// check the main part of the variable name.
-		$original_var_name = $var_name;
-		if ( '_' === substr( $var_name, 0, 1 ) && true === $in_class ) {
-			$var_name = substr( $var_name, 1 );
-		}
-
-		if ( self::isSnakeCase( $var_name ) === false ) {
+		$suggested_name = SnakeCaseHelper::get_suggestion( $var_name );
+		if ( $suggested_name !== $var_name ) {
 			if ( $in_class && ! isset( $this->allowed_mixed_case_member_var_names[ $var_name ] ) ) {
 				$error      = 'Object property "$%s" is not in valid snake_case format, try "$%s"';
 				$error_name = 'UsedPropertyNotSnakeCase';
@@ -184,8 +174,8 @@ class ValidVariableNameSniff extends PHPCS_AbstractVariableSniff {
 
 			if ( isset( $error, $error_name ) ) {
 				$data = array(
-					$original_var_name,
-					SnakeCaseHelper::get_suggestion( $original_var_name ),
+					$var_name,
+					$suggested_name,
 				);
 				$phpcs_file->addError( $error, $stack_ptr, $error_name, $data );
 			}
@@ -213,11 +203,16 @@ class ValidVariableNameSniff extends PHPCS_AbstractVariableSniff {
 		$tokens   = $phpcs_file->getTokens();
 		$var_name = ltrim( $tokens[ $stack_ptr ]['content'], '$' );
 
-		if ( ! isset( $this->allowed_mixed_case_member_var_names[ $var_name ] ) && false === self::isSnakeCase( $var_name ) ) {
+		if ( isset( $this->allowed_mixed_case_member_var_names[ $var_name ] ) ) {
+			return;
+		}
+
+		$suggested_name = SnakeCaseHelper::get_suggestion( $var_name );
+		if ( $suggested_name !== $var_name ) {
 			$error = 'Member variable "$%s" is not in valid snake_case format, try "$%s"';
 			$data  = array(
 				$var_name,
-				SnakeCaseHelper::get_suggestion( $var_name ),
+				$suggested_name,
 			);
 			$phpcs_file->addError( $error, $stack_ptr, 'PropertyNotSnakeCase', $data );
 		}
@@ -258,11 +253,12 @@ class ValidVariableNameSniff extends PHPCS_AbstractVariableSniff {
 					continue;
 				}
 
-				if ( false === self::isSnakeCase( $var_name ) ) {
+				$suggested_name = SnakeCaseHelper::get_suggestion( $var_name );
+				if ( $suggested_name !== $var_name ) {
 					$error = 'Variable "$%s" is not in valid snake_case format, try "$%s"';
 					$data  = array(
 						$var_name,
-						SnakeCaseHelper::get_suggestion( $var_name ),
+						$suggested_name,
 					);
 					$phpcs_file->addError( $error, $stack_ptr, 'InterpolatedVariableNotSnakeCase', $data );
 				}
@@ -270,16 +266,6 @@ class ValidVariableNameSniff extends PHPCS_AbstractVariableSniff {
 		}
 	}
 
-	/**
-	 * Return whether the variable is in snake_case.
-	 *
-	 * @param string $var_name Variable name.
-	 * @return bool
-	 */
-	public static function isSnakeCase( $var_name ) {
-		return (bool) preg_match( '/^[a-z0-9_]+$/', $var_name );
-	}
-
 	/**
 	 * Merge a custom allow list provided via a custom ruleset with the predefined allow list,
 	 * if we haven't already.
diff --git a/composer.json b/composer.json
index 8c236b03..3459ab50 100644
--- a/composer.json
+++ b/composer.json
@@ -29,6 +29,9 @@
 		"php-parallel-lint/php-parallel-lint": "^1.3.2",
 		"php-parallel-lint/php-console-highlighter": "^1.0.0"
 	},
+	"suggest": {
+		"ext-mbstring": "For improved results"
+	},
 	"config": {
 		"allow-plugins": {
 			"dealerdirect/phpcodesniffer-composer-installer": true


Test patch:
diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml
index 06469666..4a4a59f4 100644
--- a/.github/workflows/unit-tests.yml
+++ b/.github/workflows/unit-tests.yml
@@ -23,7 +23,13 @@ jobs:
       matrix:
         php: [ '5.4', '5.5', '5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3' ]
         phpcs_version: [ '3.7.1', 'dev-master' ]
-        #include:
+        extensions: [ '' ]
+
+        include:
+          - php: '7.4'
+            phpcs_version: 'dev-master'
+            extensions: ':mbstring' # = Disable Mbstring.
+
           # Add extra build to test against PHPCS 4.
           #- php: '7.4'
           #  phpcs_version: '4.0.x-dev as 3.9.99'
diff --git a/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.inc b/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.inc
index e87eff22..7aca5780 100644
--- a/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.inc
+++ b/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.inc
@@ -218,6 +218,11 @@ enum Suit {
 	public function __invoke() {} // Ok.
 }
 
+// Related to #1891 - ensure the sniff does not throw an error if the suggested alternative would be the same as the original name.
+function lähtöaika() {} // OK.
+function lÄhtÖaika() {} // Bad, but only handled by the sniff if Mbstring is available.
+function lÄhtOaika() {} // Bad, handled via transliteration of non-ASCII chars if Mbstring is not available.
+
 // Live coding/parse error.
 // This has to be the last test in the file.
 function
diff --git a/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.php b/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.php
index c4d74c22..76d5dcbc 100644
--- a/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.php
+++ b/WordPress/Tests/NamingConventions/ValidFunctionNameUnitTest.php
@@ -53,6 +53,8 @@ class ValidFunctionNameUnitTest extends AbstractSniffUnitTest {
 			199 => 1,
 			208 => 2,
 			210 => 1,
+			223 => function_exists( 'mb_strtolower' ) ? 1 : 0,
+			224 => 1,
 		);
 	}
 
diff --git a/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.inc b/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.inc
index 267c02c4..22b258c3 100644
--- a/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.inc
+++ b/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.inc
@@ -232,3 +232,8 @@ enum EnumExample {
 class Has_Mixed_Case_Property {
 	public $post_ID; // OK.
 }
+
+// Issue #1891 - ensure the sniff does not throw an error if the suggested alternative would be the same as the original name.
+$lähtöaika = true; // OK.
+$lÄhtÖaika = true; // Bad, but only handled by the sniff if Mbstring is available.
+$lÄhtOaika = true; // Bad, handled via transliteration of non-ASCII chars if Mbstring is not available.
diff --git a/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.php b/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.php
index dbb8725c..46688109 100644
--- a/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.php
+++ b/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.php
@@ -94,6 +94,8 @@ class ValidVariableNameUnitTest extends AbstractSniffUnitTest {
 			219 => 1,
 			225 => 1,
 			227 => 1,
+			238 => function_exists( 'mb_strtolower' ) ? 1 : 0,
+			239 => 1,
 		);
 	}