ConstantsHelper::is_use_of_global_constant(): do not treat use const alias as global constant usage

[rodrigoprimo]

ConstantsHelper::is_use_of_global_constant() was incorrectly identifying a constant alias as a global constant.

[jrfnl]

Probably a good idea to double-check why that test what added (did it come from the Theme Review sniff and if so, was it added there for a specific reason ?)

I can imagine the reasoning was something along the lines of "we don't track import use statements, so having an alias to one of the discouraged constants will be confusing for humans (they see the constant in the code, think it's the WP constant, turns out it's a different constant) as well as for the sniff itself, as if the sniff would not flag the alias, but it would flag usages of the alias, it would flag something which isn't discouraged"

If this does get merged, I would like to see a comment in the test case file on line 64 documenting that the test would now be a "no false positives" test.

[rodrigoprimo]

Probably a good idea to double-check why that test what added (did it come from the Theme Review sniff and if so, was it added there for a specific reason ?)

That is a good point. After seeing your comment, I checked the original issue and PR in the WPThemeReview repository, and as far as I can see, this specific case was not discussed there.

The original implementation of the sniff in that repository did not contain a test for a constant alias: https://github.com/WPTT/WPThemeReview/pull/30/files#diff-e0de8e3cd4545002e7d00b32b1ebe749aa609f4b0435ce825fc86e1878cff5a4

And it still did not contain such a test when the sniff was removed from the WPThemeReview repository: WPTT/WPThemeReview@35ef377#diff-e0de8e3cd4545002e7d00b32b1ebe749aa609f4b0435ce825fc86e1878cff5a4.

My best guess is that this test was added to the sniff when you introduced a new version to this repository, including more tests: 80faf1d#diff-bbe0c6d3ccadb9e75fa5ba2b7dce49ee94c6705c55f93d99650517d55c86babaR66.

I can imagine the reasoning was something along the lines of "we don't track import use statements, so having an alias to one of the discouraged constants will be confusing for humans (they see the constant in the code, think it's the WP constant, turns out it's a different constant) as well as for the sniff itself, as if the sniff would not flag the alias, but it would flag usages of the alias, it would flag something which isn't discouraged"

That is a fair point, and something I hadn't considered. I was only thinking that the sniff should not treat use const aliases as a global constant as it effectively is not. I'm unsure now what is best. Do you have a suggestion? I understand if you prefer not to change the sniff behavior.

If this does get merged, I would like to see a comment in the test case file on line 64 documenting that the test would now be a "no false positives" test.

I'm unsure if I understand your suggestion. I added a comment to the test, highlighting that now the code on line 64 does not trigger an error, even though, for historical reasons, it is in a block where all other code examples do trigger the sniff. Is this more or less what you had in mind?

[jrfnl]

Considering I introduced this (even though, I did it when I was much less experienced) and that I can still come up with a reason why it may be good to continue flagging this, I believe a second opinion is warranted on this issue.

@GaryJones @dingo-d Either of you have an opinion on the above ?

[GaryJones]

What other sniffs tests are using this helper? Can you determine if the constant alias should continue to be flagged in them?

[rodrigoprimo]

@GaryJones, besides DiscouragedConstants, the only other sniff that uses ConstantsHelper::is_use_of_global_constant() is EscapeOutput:

WordPress-Coding-Standards/WordPress/Sniffs/Security/EscapeOutputSniff.php

Lines 577 to 583 in afcb17e

	// Ignore safe PHP native constants.
	if ( \T_STRING === $this->tokens[ $i ]['code']
	&& isset( $this->safe_php_constants[ $this->tokens[ $i ]['content'] ] )
	&& ConstantsHelper::is_use_of_global_constant( $this->phpcsFile, $i )
	) {
	continue;
	}

It uses this method when checking if a given code needs to be escaped to bail when it finds a predetermined list of safe PHP constants. Example:

WordPress-Coding-Standards/WordPress/Tests/Security/EscapeOutputUnitTest.1.inc

Line 261 in afcb17e

echo PHP_VERSION_ID, PHP_VERSION, PHP_EOL, PHP_EXTRA_VERSION; // OK.

I don't see a reason to continue flagging constant aliases for this specific sniff.

[GaryJones]

I'm struggling to comprehend exactly what's being proposed here - @dingo-d I'll let you be the tie-breaker if needed.

[dingo-d]

Tbh I am not sure anybody is still using WPThemeReview sniffs (the standard hasn't been updated in 4 years from what I can see). I'd like to see if there is anybody from the themes team still using this when checking themes on .org.

To me, it's ok to fix the 'faulty' behavior here, and then make exceptions downstream (in WPThemeReview for example).

I'm also having a hard time understanding why this was added (let alone trying to understand what was done back in 2016 🙈 ).