Skip to content

Commit

Permalink
GlobalVariablesOverride/PrefixAllGlobals: handle WP variables intende…
Browse files Browse the repository at this point in the history
…d to be overwritten

WP Core contains the global `$content_width` variable which is intended to be set/overwritten by plugins and themes.

For that reason the variable was previously removed from the `Sniff::$wp_globals` list in WPCS 0.4.0. See 276, 331.

The downside of the variable not being in the list is that the `PrefixAllGlobals` sniff complains about it not being prefixed, as it doesn't realize it is a WP native global variable.

The upside was that the `GlobalVariablesOverride` sniff did not complain about the variable being overwritten.

Adding the variable to the `Sniff::$wp_globals` list would reverse that situation with the `PrefixAllGlobals` sniff staying silent and the `GlobalVariablesOverride` sniff starting to complain.

This PR intends to solve this conundrum.

* The list of WP Core globals in `Sniff::$wp_globals` should be complete and not intentionally miss certain variables without there being any documentation on why there are not listed there.
* To still allow for the `GlobalVariablesOverride` sniff to function correctly, a new `$override_allowed` property has been added to that sniff, as well as logic to handle this.

Unit tests confirming that this fixes the issue have been added to both sniffs.

Additional notes:
* There may be more variables in WP Core which are intended to be overwritten by plugins/themes. I have not verified this.
    If we do come across additional ones, it will now be easy enough to add them to the whitelist anyway.
    For now, only `content_width` and `wp_cockneyreplace` have been added.
    Also see: #924 (comment) and https://core.trac.wordpress.org/browser/trunk/src/wp-includes/formatting.php#L123
* This PR does not address the fact that the `Sniff::$wp_globals` list is grossly out of date. See 924

Fixes 1043
  • Loading branch information
jrfnl committed Jul 23, 2019
1 parent 4011319 commit 096f0cd
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 1 deletion.
1 change: 1 addition & 0 deletions WordPress/Sniff.php
Expand Up @@ -638,6 +638,7 @@ abstract class Sniff implements PHPCS_Sniff {
'compress_css' => true,
'compress_scripts' => true,
'concatenate_scripts' => true,
'content_width' => true,
'current_blog' => true,
'current_screen' => true,
'current_site' => true,
Expand Down
27 changes: 26 additions & 1 deletion WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php
Expand Up @@ -45,6 +45,21 @@ class GlobalVariablesOverrideSniff extends Sniff {
*/
public $treat_files_as_scoped = false;

/**
* Whitelist select variables from the Sniff::$wp_globals array.
*
* A few select variables in WP Core are _intended_ to be overwritten
* by themes/plugins. This sniff should not throw an error for those.
*
* @since 2.2.0
*
* @var array
*/
protected $override_allowed = array(
'content_width' => true,
'wp_cockneyreplace' => true,
);

/**
* Scoped object and function structures to skip over as
* variables will have a different scope within those.
Expand Down Expand Up @@ -194,6 +209,13 @@ protected function process_variable_assignment( $stackPtr ) {
return;
}

/*
* Is this one of the WP global variables which are allowed to be overwritten ?
*/
if ( isset( $this->override_allowed[ $var_name ] ) === true ) {
return;
}

/*
* Check if the variable value is being changed.
*/
Expand Down Expand Up @@ -256,7 +278,10 @@ protected function process_global_statement( $stackPtr, $in_function_scope ) {
}

if ( \T_VARIABLE === $var['code'] ) {
if ( isset( $this->wp_globals[ substr( $var['content'], 1 ) ] ) ) {
$var_name = substr( $var['content'], 1 );
if ( isset( $this->wp_globals[ $var_name ] )
&& isset( $this->override_allowed[ $var_name ] ) === false
) {
$search[] = $var['content'];
}
}
Expand Down
Expand Up @@ -424,4 +424,9 @@ define(
[ 1, 2, 3 ]
);

// Issue #1043.
function acronym_content_width() {
$GLOBALS['content_width'] = apply_filters( 'acronym_content_width', 640 );
}

// phpcs:set WordPress.NamingConventions.PrefixAllGlobals prefixes[]
13 changes: 13 additions & 0 deletions WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc
Expand Up @@ -210,3 +210,16 @@ class MyClass {

// Test assigning to multiple variables at once.
$is_NS4 = $is_opera = $is_safari = $GLOBALS['is_winIE'] = true; // Bad x 4.

// Issue #1043.
function globals_content_width() {
$GLOBALS['content_width'] = apply_filters( 'acronym_content_width', 640 );
}

function global_content_width() {
global $content_width;

$content_width = apply_filters( 'acronym_content_width', 640 );
}

$content_width = 1000;

0 comments on commit 096f0cd

Please sign in to comment.