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.

Sign up for GitHub

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

PrefixAllGlobals: verify that namespace names use a prefix #1515

Merged
merged 1 commit into from
Nov 2, 2018
Merged

PrefixAllGlobals: verify that namespace names use a prefix #1515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 77 additions & 4 deletions WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
*
* @since 0.12.0
* @since 0.13.0 Class name changed: this class is now namespaced.
* @since 1.2.0 Now also checks whether namespaces are prefixed.
*
* @uses \WordPress\Sniff::$custom_test_class_whitelist
*/
Expand Down Expand Up @@ -65,6 +66,20 @@ class PrefixAllGlobalsSniff extends AbstractFunctionParameterSniff {
*/
private $validated_prefixes = array();

/**
* Target namespace prefixes after validation with regex indicator.
*
* All prefixes are lowercased for case-insensitive compare.
* If the prefix doesn't already contain a namespace separator, but does contain
* non-word characters, these will have been replaced with regex syntax to allow
* for namespace separators and the `is_regex` indicator will have been set to `true`.
*
* @since 1.2.0
*
* @var array
*/
private $validated_namespace_prefixes = array();

/**
* Cache of previously set prefixes.
*
Expand Down Expand Up @@ -167,6 +182,7 @@ class PrefixAllGlobalsSniff extends AbstractFunctionParameterSniff {
*/
public function register() {
$targets = array(
\T_NAMESPACE => \T_NAMESPACE,
\T_FUNCTION => \T_FUNCTION,
\T_CLASS => \T_CLASS,
\T_INTERFACE => \T_INTERFACE,
Expand Down Expand Up @@ -274,6 +290,46 @@ public function process_token( $stackPtr ) {

return $this->process_variable_assignment( $stackPtr );

} elseif ( \T_NAMESPACE === $this->tokens[ $stackPtr ]['code'] ) {
$namespace_name = $this->get_declared_namespace_name( $stackPtr );

if ( false === $namespace_name || '' === $namespace_name || '\\' === $namespace_name ) {
return;
}

foreach ( $this->validated_namespace_prefixes as $key => $prefix_info ) {
if ( false === $prefix_info['is_regex'] ) {
if ( stripos( $namespace_name, $prefix_info['prefix'] ) === 0 ) {
$this->phpcsFile->recordMetric( $stackPtr, 'Prefix all globals: allowed prefixes', $key );
return;
}
} else {
// Ok, so this prefix should be used as a regex.
$regex = '`^' . $prefix_info['prefix'] . '`i';
if ( preg_match( $regex, $namespace_name ) > 0 ) {
$this->phpcsFile->recordMetric( $stackPtr, 'Prefix all globals: allowed prefixes', $key );
return;
}
}
}

// Still here ? In that case, we have a non-prefixed namespace name.
$recorded = $this->phpcsFile->addError(
self::ERROR_MSG,
$stackPtr,
'NonPrefixedNamespaceFound',
array(
'Namespaces declared',
$namespace_name,
)
);

if ( true === $recorded ) {
$this->record_potential_prefix_metric( $stackPtr, $namespace_name );
}

return;

} else {

// Namespaced methods, classes and constants do not need to be prefixed.
Expand Down Expand Up @@ -824,6 +880,7 @@ private function validate_prefixes() {
$this->previous_prefixes = $this->prefixes;

// Validate the passed prefix(es).
$ns_prefixes = array();
foreach ( $this->prefixes as $key => $prefix ) {
$prefixLC = strtolower( $prefix );

Expand All @@ -839,9 +896,9 @@ private function validate_prefixes() {
}

// Validate the prefix against characters allowed for function, class, constant names etc.
if ( preg_match( '`^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$`', $prefix ) !== 1 ) {
if ( preg_match( '`^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff\\\\]*$`', $prefix ) !== 1 ) {
$this->phpcsFile->addWarning(
'The "%s" prefix is not a valid function/class/variable/constant prefix in PHP.',
'The "%s" prefix is not a valid namespace/function/class/variable/constant prefix in PHP.',
0,
'InvalidPrefixPassed',
array( $prefix )
Expand All @@ -851,10 +908,26 @@ private function validate_prefixes() {

// Lowercase the prefix to allow for direct compare.
$this->prefixes[ $key ] = $prefixLC;

/*
* Replace non-word characters in the prefix with a regex snippet, but only if the
* string doesn't already contain namespace separators.
*/
$is_regex = false;
if ( strpos( $prefix, '\\' ) === false && preg_match( '`[_\W]`', $prefix ) > 0 ) {
$prefix = preg_replace( '`([_\W])`', '[\\\\\\\\$1]', $prefixLC );
$is_regex = true;
}

$ns_prefixes[ $prefixLC ] = array(
'prefix' => $prefix,
'is_regex' => $is_regex,
);
}

// Set the validated prefixes cache.
$this->validated_prefixes = $this->prefixes;
// Set the validated prefixes caches.
$this->validated_prefixes = $this->prefixes;
$this->validated_namespace_prefixes = $ns_prefixes;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.1.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ define( '\OtherNameSpace\PLUGIN_FILE', __FILE__ ); // OK.
define( __NAMESPACE__ . '\PLUGIN_FILE', __FILE__ );
define( '\PLUGIN_FILE', __FILE__ );

namespace Testing {
namespace TGMPA\Testing {
define( 'MY' . __NAMESPACE__, __FILE__ ); // Error, not actually namespaced.
define( 'MY\\' . __NAMESPACE__, __FILE__ ); // OK, even though strangely setup, the constant is in a namespace.
}
Expand Down
30 changes: 30 additions & 0 deletions WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.4.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php
// @codingStandardsChangeSetting WordPress.NamingConventions.PrefixAllGlobals prefixes acronym,tgmpa,test_this,test\that

Copy link
Member

Choose a reason for hiding this comment

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

One thing I've found beneficial when working on VIPCS, was to add a comment for each test case to identify what case was being tested. e.g.

namespace TGMPA; // Ok - case-insensitive namespace.

namespace Acronym\B\C; // Ok - prefix is top-level namespace, with sub-namespaces.

namespace AcronymPlugin\B\C; // Ok - prefix is start of top-level namespace, with sub-namespaces.

namespace Test_This\C; // Ok - namespace with underscore preserved from prefix.

namespace Test\This\C; // Ok - namespace with underscore converted to sub-namespace from prefix.

namespace Test\That\C; // Ok - prefix is multi-level namespace.

etc. Same goes for // Bad cases - adding a comment as to what aspect of the logic something fails.

I think this makes it easier to follow and understand the test cases, and helps avoid redundant test cases that don't test anything new.

Could comments like that be added here?

Copy link
Member Author

@jrfnl jrfnl Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Go right ahead if you feel the need. To me, those comments don't add any value as this sniff only has one purpose which is checking if something is prefixed.

Copy link
Member

Choose a reason for hiding this comment

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

But the different test cases are checking all the different ways that it can be prefixed - the how, rather than the what.

I'm not going to be able to get these comments in the near future. The review is already approved. Probably needs a new ticket to add comments in for OK and Bad tests cases across all the sniffs.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the different test cases are checking all the different ways that it can be prefixed - the how, rather than the what.

That is documented in both the sniff as well as the commit message.

There are definitely some cases where additional comments in the unit test files could have value, but let's not go overboard.

Probably needs a new ticket to add comments in for OK and Bad tests cases across all the sniffs.

Sounds like a plan, low priority though as there are plenty of real issues to address still. Possibly mark it as good first issue ?

The review is already approved.

As we changed to one approval, feel free to merge it.

namespace TGMPA;

namespace Acronym\B\C;

namespace AcronymPlugin\B\C;

namespace Test_This\C;

namespace Test\This\C;

namespace Test\That\C;

namespace Test \ /* comment */ This \ C;

namespace A\B\C; // Bad.

use namespace\DEF; // Not our target.
namespace\cname::method(); // Not our target.
$acronym_result = namespace \ blah \ mine(); // Not our target.

// @codingStandardsChangeSetting WordPress.NamingConventions.PrefixAllGlobals prefixes wordpress
namespace wordpress\myplugin;

// @codingStandardsChangeSetting WordPress.NamingConventions.PrefixAllGlobals prefixes wordpress\myplugin
namespace wordpress\myplugin;

// @codingStandardsChangeSetting WordPress.NamingConventions.PrefixAllGlobals prefixes false
8 changes: 7 additions & 1 deletion WordPress/Tests/NamingConventions/PrefixAllGlobalsUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ public function getErrorList( $testFile = 'PrefixAllGlobalsUnitTest.1.inc' ) {
403 => 1,
);

case 'PrefixAllGlobalsUnitTest.4.inc':
return array(
1 => 1, // 1 x error for blacklisted prefix passed.
18 => 1,
);

case 'PrefixAllGlobalsUnitTest.2.inc':
// Namespaced - all OK, fall through to the default case.
case 'PrefixAllGlobalsUnitTest.3.inc':
Expand All @@ -90,7 +96,7 @@ public function getWarningList( $testFile = 'PrefixAllGlobalsUnitTest.1.inc' ) {
switch ( $testFile ) {
case 'PrefixAllGlobalsUnitTest.1.inc':
return array(
1 => 3, // 3 x error for potentially incorrect prefix passed.
1 => 2, // 2 x warning for potentially incorrect prefix passed.
249 => 1,
250 => 1,
253 => 1,
Expand Down