Skip to content

PrefixAllGlobals: prevent false negatives for autoloaded user-defined global functions#1633

Merged
GaryJones merged 1 commit into
developfrom
feature/1632-prefixallglobals-fix-false-negative-composer-file-autoload
Jan 23, 2019
Merged

PrefixAllGlobals: prevent false negatives for autoloaded user-defined global functions#1633
GaryJones merged 1 commit into
developfrom
feature/1632-prefixallglobals-fix-false-negative-composer-file-autoload

Conversation

@jrfnl

@jrfnl jrfnl commented Jan 22, 2019

Copy link
Copy Markdown
Member

The PrefixAllGlobals sniff verifies that functions declared in the global namespace are prefixed with one of the whitelisted prefixes as passed to the sniff in the prefixes property in a custom ruleset.

The sniff prevents false positives for polyfills for PHP native functions - which should be named exactly as named in PHP without prefix for them to be usable as a polyfill - by checking that the function doesn't exist.

Generally speaking this works fine in 98% of all cases as PHPCS is normally run stand-alone and doesn't contain any functions defined in the global namespace.

Similarly, when PHPCS is installed via Composer, this would normally work fine as the commonly used autoload options are PSR-0, PSR-4 and classmap which are all based on code being in classes.

However, if the Composer autoload files option is used to load, for instance, a functions file declaring functions in the global namespace, this "polyfill false positive prevention" would incorrectly cause errors not to be thrown for functions declared in the global namespace which were now autoloaded via Composer.

I have now fixed this by, instead of using function_exists(), using a check against a functions list retrieved via get_defined_functions().

This change does not have unit tests as:

  • Preventing false positives for polyfills is already unit tested.
  • Checking that autoloaded user defined functions are not recognized as PHP native functions is not something which can be unit tested as such. This would need an integration test including a composer setup to be tested.

Based on the test case provided in the issue which originally reported this, I have confirmed that this PR fixes the issue though.

Fixes #1632

@GaryJones GaryJones left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor docs tweak. Code otherwise good to go.

Comment thread WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php Outdated
… global functions

The `PrefixAllGlobals` sniff verifies that functions declared in the global namespace are prefixed with one of the whitelisted prefixes as passed to the sniff in the `prefixes` property in a custom ruleset.

The sniff prevents false positives for polyfills for PHP native functions - which should be named exactly as named in PHP without prefix for them to be usable as a polyfill - by checking that the function didn't exist.

Generally speaking this works fine in 95% of all cases as PHPCS is normally run stand-alone and doesn't contain any functions defined in the global namespace.

Similarly, when PHPCS is installed via Composer, this would normally work fine as the commonly used `autoload` options are `PSR-0`, `PSR-4` and `classmap` which are all based on code being in classes.

However, if the Composer `autoload` `files` option is used to load, for instance, a functions file declaring functions in the global namespace, this "polyfill false positive prevention" would incorrectly cause errors _not_ to be thrown for functions declared in the global namespace which were now autoloaded via Composer.

I have now fixed this by, instead of using `function_exists()`, using a check against a functions list retrieved via `get_defined_functions()`.

This change does not have unit tests as:
* Preventing false positives for polyfills is already unit tested.
* Checking that autoloaded user defined functions are not recognized as PHP native functions is not something which can be unit tested as such. This would need an integration test including a composer setup to be tested.

Based on the test case provided in the issue which originally reported this, I have confirmed that this PR fixes the issue though.

Fixes 1632
@jrfnl jrfnl force-pushed the feature/1632-prefixallglobals-fix-false-negative-composer-file-autoload branch from d60cbc6 to 97167b2 Compare January 22, 2019 17:18
@GaryJones GaryJones merged commit 3666bbb into develop Jan 23, 2019
@GaryJones GaryJones deleted the feature/1632-prefixallglobals-fix-false-negative-composer-file-autoload branch January 23, 2019 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PrefixAllGlobals not reporting for file autoloaded in composer.json

2 participants