WordPress / plugin-check

A repository for the new Plugin Check plugin from the WordPress Performance and Plugins Team.
https://wordpress.org/plugins/plugin-check/
GNU General Public License v2.0
195 stars 39 forks source link

False positive for exclusionary parameters #430

Open shawn-digitalpoint opened 4 months ago

shawn-digitalpoint commented 4 months ago

This is new in 1.0... An array with a key of "exclude" will trigger:

WordPressVIPMinimum.Performance.WPQueryParams.PostNotIn_exclude

Using exclusionary parameters, like exclude, in calls to get_posts() should be done with caution, see https://wpvip.com/documentation/performance-improvements-by-removing-usage-of-post__not_in/ for more information.

The code that triggers it does not have anything to do with excluding posts (or anything else coming from the database). The code is this:

$phrases = [
    'allow' => esc_html__('allow', 'app-for-cf'),
    'deny' => esc_html__('deny', 'app-for-cf'),
    'non_identity' => esc_html__('non-identity', 'app-for-cf'),
    'bypass' => esc_html__('bypass', 'app-for-cf'),

    'everyone' => esc_html__('Everyone', 'app-for-cf'),
    'include' => esc_html__('Include', 'app-for-cf'),
    'require' => esc_html__('Require', 'app-for-cf'),
    'exclude' => esc_html__('Exclude', 'app-for-cf'),
];
swissspidy commented 4 months ago

Unfortunately the WordPressVIPMinimum.Performance.WPQueryParams.PostNotIn_exclude is not very smart, so false positives are common. We've had to add one ourselves too:

https://github.com/WordPress/plugin-check/blob/587489ea08655d59317f8b45eb6253f4b708c471/includes/Checker/Checks/Abstract_PHP_CodeSniffer_Check.php#L26-L37

Suggestions for improving this sniff would need to be opened at https://github.com/Automattic/VIP-Coding-Standards.

Of course questions can be raised whether this check should be updated/disabled/documented better in Plugin Check. However, given that it's only a warning, there's probably no need for action and we can instead use this energy to improve the sniff upstream.

shawn-digitalpoint commented 4 months ago

Ran into it again with a different plugin:

$policies = [
    [
        'decision' => 'allow',
        'exclude' => [],
        'include' => $emails,
        'name' => $this->phrase('allow'),
        'precedence' => 1,
        'require' => [],
        'uid' => ''
    ]
];
shawn-digitalpoint commented 4 months ago

Ya, it's not a huge deal... in the case of the second example, it's coming from a library that isn't WordPress specific, so adding the ignore tag isn't an option. But like you said, it's just a warning, not something catastrophic, so it is what it is unless something upstream changes.