Skip to content

Commit

Permalink
Constants/ConstantString: prevent more false positives
Browse files Browse the repository at this point in the history
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 #422 (comment) (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
  • Loading branch information
jrfnl committed Sep 25, 2020
1 parent 7c6dc07 commit b01bca1
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 15 deletions.
26 changes: 16 additions & 10 deletions WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}

}
22 changes: 20 additions & 2 deletions WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ class ConstantStringUnitTest extends AbstractSniffUnitTest {
*/
public function getErrorList() {
return [
7 => 1,
8 => 1,
14 => 1,
7 => 1,
8 => 1,
];
}

Expand Down

0 comments on commit b01bca1

Please sign in to comment.