ConstantString doesn't allow for namespaces before the string

[GaryJones]

Bug Description

ConstantStringSniff doesn't allow for namespaces before the string.

Minimal Code Snippet

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 ) ) {
}

Error Code

Constant name, as a string, should be used along with defined().
(WordPressVIPMinimum.Constants.ConstantString.NotCheckingConstantName)

Environment

Question	Answer
PHP version	7.3.2
PHP_CodeSniffer version	3.4.2
VIPCS version	1.0.0

Tested Against master branch?

• I have verified the issue still exists in the master branch of VIPCS.

[GaryJones]

Real client code examples where this violation has incorrectly appeared (false positive):

define( __NAMESPACE__ . '\PLUGIN_URL', \plugins_url( '/', __FILE__ ) );

and

if ( defined( __NAMESPACE__ . '\\LOADED' ) ) {

[jrfnl]

Related #480

I've been looking at a fix for this issue to add to PR #444, but am running into a decision point.

I can easily come up with plenty more valid examples which would trigger this sniff.
Think:

if ( defined( MY_PREFIX . '_CONSTANT_NAME' ) === false ) {}

// Or the example from #480:
if ( defined( $key ) ) {}

If I read the history of the sniff correctly, the sniff was put in place to guard against a potential coding mistake where a developer would pass the constant name in calls to define() and defined() as a constant, not as a text string.

Aside from that actually being valid code as long as the constant passed contains the name of the target constant as a text string, the sniff itself, is flawed no matter what, as it only looks at the first token within the "name", not at all tokens, so a code sample like define( 'PREFIX_' . CONSTANT_NAME, $value ) would pass inspection.

There are two options for ways to go here and I'd like some opinions on which to take:

1. Simple: Only trigger an error if there is only one non-empty token in the parameter and that token is a T_STRING.
   That covers the original case for which the sniff was build, but it wouldn't trigger an error on the new test case on line 14 of PR ConstantStringSniff: Take into account namespace prefixing #444.
2. Complex: Look at all tokens within the function call parameter individually and work out their context/whether they could be valid and only trigger an error if they aren't.

I'd personally advocate for going with option 1, as, as soon as there is a compound parameter, 1) it indicates that there is a good chance the developer knew what they were doing and 2) the chances of throwing false positives multiply.

@rebeccahum @GaryJones Opinions ?

[GaryJones]

@jrfnl 1 works for me.

I imagine option 2 would be more easily solvable with PHPCSUtils anyway?

[jrfnl]

I imagine option 2 would be more easily solvable with PHPCSUtils anyway?

Not necessarily, but other things about this sniff will definitely benefit from Utils, like the function call determination and parameter finding.

[jrfnl]

I've added a new commit to the open PR #444 now which implements option 1.