Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New WordPress.WP.DiscouragedConstants sniff #1089

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions WordPress-Extra/ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
<rule ref="WordPress.WP.DeprecatedClasses"/>
<rule ref="WordPress.WP.DeprecatedParameters"/>
<rule ref="WordPress.WP.AlternativeFunctions"/>
<rule ref="WordPress.WP.DiscouragedConstants"/>
<rule ref="WordPress.WP.DiscouragedFunctions"/>

<rule ref="Squiz.PHP.Eval"/>
Expand Down
4 changes: 4 additions & 0 deletions WordPress-VIP/ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,8 @@
<rule ref="WordPress.WP.AlternativeFunctions.file_get_contents">
<message>%s() is highly discouraged, please use vip_safe_wp_remote_get() instead.</message>
</rule>

<!-- https://vip.wordpress.com/documentation/code-review-what-we-look-for/#using-theme-constants -->
<rule ref="WordPress.WP.DiscouragedConstants"/>

</ruleset>
214 changes: 214 additions & 0 deletions WordPress/Sniffs/WP/DiscouragedConstantsSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
<?php
/**
* WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
* @license https://opensource.org/licenses/MIT MIT
*/

namespace WordPress\Sniffs\WP;

use WordPress\AbstractFunctionParameterSniff;
use PHP_CodeSniffer_Tokens as Tokens;

/**
* Warns against usage of discouraged WP CONSTANTS and recommends alternatives.
*
* @package WPCS\WordPressCodingStandards
*
* @since 0.14.0
*/
class DiscouragedConstantsSniff extends AbstractFunctionParameterSniff {

/**
* List of discouraged WP constants and their replacements.
*
* @since 0.14.0
*
* @var array
*/
protected $discouraged_constants = array(
'STYLESHEETPATH' => 'get_stylesheet_directory()',
'TEMPLATEPATH' => 'get_template_directory()',
'PLUGINDIR' => 'WP_PLUGIN_DIR',
'MUPLUGINDIR' => 'WPMU_PLUGIN_DIR',
'HEADER_IMAGE' => 'add_theme_support( \'custom-header\' )',
'NO_HEADER_TEXT' => 'add_theme_support( \'custom-header\' )',
'HEADER_TEXTCOLOR' => 'add_theme_support( \'custom-header\' )',
'HEADER_IMAGE_WIDTH' => 'add_theme_support( \'custom-header\' )',
'HEADER_IMAGE_HEIGHT' => 'add_theme_support( \'custom-header\' )',
'BACKGROUND_COLOR' => 'add_theme_support( \'custom-background\' )',
'BACKGROUND_IMAGE' => 'add_theme_support( \'custom-background\' )',
);

/**
* Array of functions to check.
*
* @since 0.14.0
*
* @var array <string function name> => <int parameter position>
*/
protected $target_functions = array(
'define' => 1,
);

/**
* Array of tokens which if found preceding the $stackPtr indicate that a T_STRING is not a constant.
*
* @var array
*/
private $preceding_tokens_to_ignore = array(
T_NAMESPACE => true,
T_USE => true,
T_CLASS => true,
T_TRAIT => true,
T_INTERFACE => true,
T_EXTENDS => true,
T_IMPLEMENTS => true,
T_NEW => true,
T_FUNCTION => true,
T_CONST => true,
T_DOUBLE_COLON => true,
T_OBJECT_OPERATOR => true,
T_INSTANCEOF => true,
T_GOTO => true,

);

/**
* Processes this test, when one of its tokens is encountered.
*
* @since 0.14.0
*
* @param int $stackPtr The position of the current token in the stack.
*
* @return int|void Integer stack pointer to skip forward or void to continue
* normal file processing.
*/
public function process_token( $stackPtr ) {
if ( isset( $this->target_functions[ strtolower( $this->tokens[ $stackPtr ]['content'] ) ] ) ) {
// Disallow excluding function groups for this sniff.
$this->exclude = '';

return parent::process_token( $stackPtr );

} else {
return $this->process_arbitrary_tstring( $stackPtr );
}
}

/**
* Process an arbitrary T_STRING token to determine whether it is one of the target constants.
*
* @since 0.14.0
*
* @param int $stackPtr The position of the current token in the stack.
*
* @return void
*/
public function process_arbitrary_tstring( $stackPtr ) {
$content = $this->tokens[ $stackPtr ]['content'];

if ( ! isset( $this->discouraged_constants[ $content ] ) ) {
return;
}

$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
if ( false !== $next && T_OPEN_PARENTHESIS === $this->tokens[ $next ]['code'] ) {
// Function call or declaration.
return;
}

$prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
if ( false !== $prev && isset( $this->preceding_tokens_to_ignore[ $this->tokens[ $prev ]['code'] ] ) ) {
// Not the use of a constant.
return;
}

if ( false !== $prev
&& T_NS_SEPARATOR === $this->tokens[ $prev ]['code']
&& T_STRING === $this->tokens[ ( $prev - 1 ) ]['code']
) {
// Namespaced constant of the same name.
return;
}

/*
* Deal with a number of variations of use statements.
*/
for ( $i = $stackPtr; $i > 0; $i-- ) {
if ( $this->tokens[ $i ]['line'] !== $this->tokens[ $stackPtr ]['line'] ) {
break;
}
}

$first_on_line = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true );
if ( false !== $first_on_line && T_USE === $this->tokens[ $first_on_line ]['code'] ) {
$next_on_line = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $first_on_line + 1 ), null, true );
if ( false !== $next_on_line ) {
if ( ( T_STRING === $this->tokens[ $next_on_line ]['code']
&& 'const' === $this->tokens[ $next_on_line ]['content'] )
|| T_CONST === $this->tokens[ $next_on_line ]['code'] // Happens in some PHPCS versions.
) {
$has_ns_sep = $this->phpcsFile->findNext( T_NS_SEPARATOR, ( $next_on_line + 1 ), $stackPtr );
if ( false !== $has_ns_sep ) {
// Namespaced const (group) use statement.
return;
}
} else {
// Not a const use statement.
return;
}
}
}

// Ok, this is really one of the discouraged constants.
$this->phpcsFile->addWarning(
'Found usage of constant "%s". Use %s instead.',
$stackPtr,
'UsageFound',
array(
$content,
$this->discouraged_constants[ $content ],
)
);
}

/**
* Process the parameters of a matched `define` function call.
*
* @since 0.14.0
*
* @param int $stackPtr The position of the current token in the stack.
* @param array $group_name The name of the group which was matched.
* @param string $matched_content The token content (function name) which was matched.
* @param array $parameters Array with information about the parameters.
*
* @return void
*/
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
$function_name = strtolower( $matched_content );
$target_param = $this->target_functions[ $function_name ];

// Was the target parameter passed ?
if ( ! isset( $parameters[ $target_param ] ) ) {
return;
}

$raw_content = $this->strip_quotes( $parameters[ $target_param ]['raw'] );

if ( isset( $this->discouraged_constants[ $raw_content ] ) ) {
$this->phpcsFile->addWarning(
'Found declaration of constant "%s". Use %s instead.',
$stackPtr,
'DeclarationFound',
array(
$raw_content,
$this->discouraged_constants[ $raw_content ],
)
);
}
}

} // End class.
73 changes: 73 additions & 0 deletions WordPress/Tests/WP/DiscouragedConstantsUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
<?php

/*
* Make sure that global constants are correctly identified.
*
* The below are all ok.
*/
// Ok, not a constant.
namespace STYLESHEETPATH {}
namespace MY\OTHER\STYLESHEETPATH\NAMESPACE {}
use STYLESHEETPATH;
use Something, STYLESHEETPATH, SomethingElse;
class STYLESHEETPATH {
const STYLESHEETPATH = 'something';
private function STYLESHEETPATH() {}
}
class ABC extends STYLESHEETPATH {}
class DEF implements STYLESHEETPATH {}
interface STYLESHEETPATH {}
trait STYLESHEETPATH {}
const STYLESHEETPATH = 'something';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this OK, if calling define() is not OK? I'd think that if we sniff for define(), we ought to also sniff for const outside of a class or namespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I added a lot of code to prevent false positives and only addressed the define() bit after that. Clearly forgot to also change things for const.

I'm going to change this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, see separate commit.

$a = new STYLESHEETPATH;
$a = new STYLESHEETPATH();
function STYLESHEETPATH() {}
echo STYLESHEETPATH();
echo My\UsedAsNamespace\STYLESHEETPATH\something;
My\UsedAsNamespace\STYLESHEETPATH\something::something_else();
if ( $abc instanceof STYLESHEETPATH ) {}

goto STYLESHEETPATH;
// Something.
STYLESHEETPATH:
// Something.

// Ok, not a global constant.
echo \mynamespace\STYLESHEETPATH;
echo My_Class::STYLESHEETPATH;
echo $this->STYLESHEETPATH;
use const SomeNamespace\STYLESHEETPATH as SSP; // PHP 5.6+
use const SomeNamespace\{STYLESHEETPATH, TEMPLATEPATH}; // PHP 7.0+
define( 'My\STYLESHEETPATH', 'something' );

// Ok, not usage of the constant as such.
if ( defined( 'STYLESHEETPATH' ) ) { // Ok.
// Do something unrelated.
}


/*
* These are all bad.
*/
echo STYLESHEETPATH;
echo \STYLESHEETPATH; // Global constant.
$folder = basename( TEMPLATEPATH );
include PLUGINDIR . '/js/myfile.js';
echo MUPLUGINDIR;
echo HEADER_IMAGE;
echo NO_HEADER_TEXT;
echo HEADER_TEXTCOLOR;
echo HEADER_IMAGE_WIDTH;
echo HEADER_IMAGE_HEIGHT;
echo BACKGROUND_COLOR;
echo BACKGROUND_IMAGE;

use const STYLESHEETPATH as SSP;
use const ABC as STYLESHEETPATH;

switch( STYLESHEETPATH ) {
case STYLESHEETPATH:
break;
}

define( 'STYLESHEETPATH', 'something' );
58 changes: 58 additions & 0 deletions WordPress/Tests/WP/DiscouragedConstantsUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php
/**
* Unit test class for WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
* @license https://opensource.org/licenses/MIT MIT
*/

namespace WordPress\Tests\WP;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

/**
* Unit test class for the WP_DiscouragedConstants sniff.
*
* @package WPCS\WordPressCodingStandards
* @since 0.14.0
*/
class DiscouragedConstantsUnitTest extends AbstractSniffUnitTest {

/**
* Returns the lines where errors should occur.
*
* @return array <int line number> => <int number of errors>
*/
public function getErrorList() {
return array();
}

/**
* Returns the lines where warnings should occur.
*
* @return array <int line number> => <int number of warnings>
*/
public function getWarningList() {
return array(
52 => 1,
53 => 1,
54 => 1,
55 => 1,
56 => 1,
57 => 1,
58 => 1,
59 => 1,
60 => 1,
61 => 1,
62 => 1,
63 => 1,
65 => 1,
66 => 1,
68 => 1,
69 => 1,
73 => 1,
);
}

} // End class.