From a3a38b2716d5ee695b4b25cb5571be767ac828c5 Mon Sep 17 00:00:00 2001 From: Rebecca Hum Date: Tue, 27 Aug 2019 15:49:19 -0600 Subject: [PATCH 1/3] ConstantStringSniff: Take into account namespace prefixing --- .../Sniffs/Constants/ConstantStringSniff.php | 8 +++++++- .../Tests/Constants/ConstantStringUnitTest.inc | 12 +++++++++--- .../Tests/Constants/ConstantStringUnitTest.php | 5 +++-- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php index 24a13635..461827bc 100644 --- a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php +++ b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php @@ -55,7 +55,13 @@ public function process_token( $stackPtr ) { return; } - $nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true ); + $nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true ); + $nextNextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true ); + + if ( T_NS_C === $this->tokens[ $nextToken ]['code'] && T_STRING_CONCAT === $this->tokens[ $nextNextToken ]['code'] ) { + // Namespacing being used, skip to next. + $nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextNextToken + 1, null, true, null, true ); + } if ( $this->tokens[ $nextToken ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) { $message = 'Constant name, as a string, should be used along with `%s()`.'; diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc index a330da69..5bace853 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc @@ -4,6 +4,12 @@ if ( ! defined( 'WPCOM_VIP' ) ) { // Okay. define( 'WPCOM_VIP', true ); // Okay. } -if ( ! defined( WPCOM_VIP ) ) { // NOK. - define( WPCOM_VIP ); // NOK. -} \ No newline at end of file +if ( ! defined( WPCOM_VIP ) ) { // Error. + define( WPCOM_VIP ); // Error. +} + +namespace Foo/Bar; +const REST_ALLOWED_META_PREFIXES = [ 'foo-', 'bar-', 'baz-' ]; +if ( defined( __NAMESPACE__ . '\REST_ALLOWED_META_PREFIXES' ) && in_array( 'foo-', REST_ALLOWED_META_PREFIXES, true ) ) { // Ok. + define( __NAMESPACE__ . REST_ALLOWED_META_PREFIXES ); // Error. +} diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php index 9b6b4225..b03d06d6 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php @@ -25,8 +25,9 @@ class ConstantStringUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ - 7 => 1, - 8 => 1, + 7 => 1, + 8 => 1, + 14 => 1, ]; } From 7c6dc07239bc708107f518ab277dc04b1ca87879 Mon Sep 17 00:00:00 2001 From: Rebecca Hum <16962021+rebeccahum@users.noreply.github.com> Date: Wed, 28 Aug 2019 08:34:10 -0600 Subject: [PATCH 2/3] Namespace should be backslash rather than forward slash on unit test Co-Authored-By: Juliette <663378+jrfnl@users.noreply.github.com> --- WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc index 5bace853..b07e5e7f 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc @@ -8,7 +8,7 @@ if ( ! defined( WPCOM_VIP ) ) { // Error. define( WPCOM_VIP ); // Error. } -namespace Foo/Bar; +namespace Foo\Bar; const REST_ALLOWED_META_PREFIXES = [ 'foo-', 'bar-', 'baz-' ]; if ( defined( __NAMESPACE__ . '\REST_ALLOWED_META_PREFIXES' ) && in_array( 'foo-', REST_ALLOWED_META_PREFIXES, true ) ) { // Ok. define( __NAMESPACE__ . REST_ALLOWED_META_PREFIXES ); // Error. From b01bca1c11fc907697b7daf67bdcdf237ff0b4e7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 25 Sep 2020 16:22:42 +0200 Subject: [PATCH 3/3] Constants/ConstantString: prevent more false positives The `define()` and `defined()` functions expect the name of a constant as a text string to be passed as the first parameter. This sniff is intended to guard against a coding mistake where the name of the constant would be passed as a_constant_. While passing the name of the constant as a constant _could_ be perfectly valid when that constant contains the name of another constant as a text string, more often than not, it will be a coding error. As things were, the sniff did not handle parameters build up using more than one token into account. It also did not handle valid situations, like passing the constant name as a text string via a variable to the functions, correctly. As outlined in https://github.com/Automattic/VIP-Coding-Standards/issues/422#issuecomment-698934220 (Proposal 1), this limits the sniff to _only_ trigger an error when a plain constant has been passed as the "constant name" parameter and ignore all other cases. Note: The current implementation of this fix, uses the WPCS `get_function_call_parameter()` method to retrieve the stack pointer to the start and end of the parameter. Once PHPCSUtils is in place, this function call should be replaced by using the PHPCSUtils version of that method. Fixes 422 Fixes 480 --- .../Sniffs/Constants/ConstantStringSniff.php | 26 ++++++++++++------- .../Constants/ConstantStringUnitTest.inc | 22 ++++++++++++++-- .../Constants/ConstantStringUnitTest.php | 5 ++-- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php index 461827bc..d6506c8f 100644 --- a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php +++ b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php @@ -55,20 +55,26 @@ public function process_token( $stackPtr ) { return; } - $nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true ); - $nextNextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextToken + 1, null, true, null, true ); - - if ( T_NS_C === $this->tokens[ $nextToken ]['code'] && T_STRING_CONCAT === $this->tokens[ $nextNextToken ]['code'] ) { - // Namespacing being used, skip to next. - $nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $nextNextToken + 1, null, true, null, true ); + $param = $this->get_function_call_parameter( $stackPtr, 1 ); + if ( $param === false ) { + // Target parameter not found. + return; } - if ( $this->tokens[ $nextToken ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) { - $message = 'Constant name, as a string, should be used along with `%s()`.'; - $data = [ $this->tokens[ $stackPtr ]['content'] ]; - $this->phpcsFile->addError( $message, $nextToken, 'NotCheckingConstantName', $data ); + $search = Tokens::$emptyTokens; + $search[ T_STRING ] = T_STRING; + + $has_only_tstring = $this->phpcsFile->findNext( $search, $param['start'], $param['end'] + 1, true ); + if ( $has_only_tstring !== false ) { + // Came across something other than a T_STRING token. Ignore. return; } + + $tstring_token = $this->phpcsFile->findNext( T_STRING, $param['start'], $param['end'] + 1 ); + + $message = 'Constant name, as a string, should be used along with `%s()`.'; + $data = [ $this->tokens[ $stackPtr ]['content'] ]; + $this->phpcsFile->addError( $message, $tstring_token, 'NotCheckingConstantName', $data ); } } diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc index b07e5e7f..ec4ab2f4 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc @@ -5,11 +5,29 @@ if ( ! defined( 'WPCOM_VIP' ) ) { // Okay. } if ( ! defined( WPCOM_VIP ) ) { // Error. - define( WPCOM_VIP ); // Error. + define( WPCOM_VIP, true ); // Error. } namespace Foo\Bar; const REST_ALLOWED_META_PREFIXES = [ 'foo-', 'bar-', 'baz-' ]; if ( defined( __NAMESPACE__ . '\REST_ALLOWED_META_PREFIXES' ) && in_array( 'foo-', REST_ALLOWED_META_PREFIXES, true ) ) { // Ok. - define( __NAMESPACE__ . REST_ALLOWED_META_PREFIXES ); // Error. + define( __NAMESPACE__ . '\\' . REST_ALLOWED_META_PREFIXES[1], $value ); // OK. } + +define( __NAMESPACE__ . '\PLUGIN_URL', \plugins_url( '/', __FILE__ ) ); // OK. +if ( defined( __NAMESPACE__ . '\\LOADED' ) ) {} // OK. + +if ( defined( $obj->constant_name_property ) === false ) { // OK. + define( $variable_containing_constant_name, $constant_value ); // OK. +} + +if ( defined( MY_PREFIX . '_CONSTANT_NAME' ) === false ) { // OK. + define( 'PREFIX_' . $variable_part, $constant_value ); // OK. +} + +if ( ! defined($generator->get()) { // OK. + define( $generator->getLast(), 'value'); // OK. +} + +$defined = defined(); // OK, ignore. +$defined = defined( /*comment*/ ); // OK, ignore. diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php index b03d06d6..9b6b4225 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php @@ -25,9 +25,8 @@ class ConstantStringUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ - 7 => 1, - 8 => 1, - 14 => 1, + 7 => 1, + 8 => 1, ]; }