search

Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
236 stars 40 forks source link

`WordPress-VIP-Go` not flagging all expected rules from `WordPressVIPMinimum` #261

Closed brettshumaker closed 5 years ago

brettshumaker commented 5 years ago

Bug Description

When running phpcs * --standard=WordPress-VIP-Go -s -d --severity=1 against the code snippet below, I'd expect to see a warning triggered for WordPress.PHP.StrictComparisons.LooseComparison (among others) but it is not present. If I run phpcs * --standard=WordPressVIPMinimum -s -d --severity=1, however, I do see the warning.

Minimal Code Snippet

<?php

define( 'JETPACK_SEARCH_VIP_INDEX', true );

add_filter( 'jetpack_active_modules', 'x_enable_jetpack_search_module', 9999 );
function x_enable_jetpack_search_module( $modules ) {
    if ( ! in_array( 'search', $modules ) || $x == 1 ) {
        $modules[] = 'search';
    }

    add_user_meta();

    file_get_contents();
    fopen();

    return $modules;
}

Error Code

> phpcs * --standard=WordPress-VIP-Go -s -d --severity=1

FILE: /Users/brettshumaker/Developer/phpcstesting/phpcstesting.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 15 | ERROR | %s() is uncached. If this is being used to query a remote file please use wpcom_vip_file_get_contents() instead. If it's used for a local file please use WP_Filesystem
    |       | instead. Read more here: https://vip.wordpress.com/documentation/using-wp_filesystem-instead-of-direct-file-access-functions/
    |       | (WordPressVIPMinimum.VIP.FetchingRemoteData.fileGetContentsUknown)
 16 | ERROR | File operations should use WP_Filesystem methods instead of direct PHP filesystem calls. Found: fopen(). Read more here:
    |       | https://vip.wordpress.com/documentation/using-wp_filesystem-instead-of-direct-file-access-functions/ (WordPress.WP.AlternativeFunctions.file_system_read_fopen)
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 116ms; Memory: 6Mb
> phpcs * --standard=WordPressVIPMinimum -s -d --severity=1

FILE: /Users/brettshumaker/Developer/phpcstesting/phpcstesting.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 5 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  7 | WARNING | Not using strict comparison for in_array; supply true for third argument. (WordPress.PHP.StrictInArray.MissingTrueStrict)
  7 | WARNING | Variable $x is undefined. (WordPressVIPMinimum.Variables.VariableAnalysis.UndefinedVariable)
  7 | WARNING | Found: ==. Use strict comparisons (=== or !==). (WordPress.PHP.StrictComparisons.LooseComparison)
 13 | ERROR   | add_user_meta() usage is highly discouraged on WordPress.com VIP due to it being a multisite, please see
    |         | https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#wp_users-and-user_meta.
    |         | (WordPressVIPMinimum.VIP.RestrictedFunctions.user_meta_add_user_meta)
 15 | WARNING | `file_get_contents()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.
    |         | (WordPressVIPMinimum.VIP.FetchingRemoteData.fileGetContentsUknown)
 16 | WARNING | File operations should use WP_Filesystem methods instead of direct PHP filesystem calls. Found: fopen() (WordPress.WP.AlternativeFunctions.file_system_read_fopen)
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 92ms; Memory: 6Mb

Environment

Use php -v and composer show to get versions.

Question Answer
PHP version 7.1.19
PHP_CodeSniffer version 3.3.2
VIPCS version Running latest from master branch

Additional Context (optional)

My installed standards

> phpcs -i
The installed coding standards are PEAR, Zend, PSR2, MySource, Squiz, PSR1, PSR12, WordPress-VIP, WordPress, WordPress-Extra, WordPress-Docs, WordPress-Core, WordPressVIPMinimum and WordPress-VIP-Go

Tested Against master branch?

*Edited to update incorrect line numbers

GaryJones commented 5 years ago

Relevant parts of ruleset:

That last ones shows that WordPress-VIP-Go turns off the check for add_user_meta(), so no mystery there.

brettshumaker commented 5 years ago

Did some more testing and adding --warning-severity=1 to my phpcs command brings back the expected warnings. I'm not sure why the --severity=1 flag isn't applying to warnings, because it should be according to the docs.

phpcs * --standard=WordPress-VIP-Go -s -d --severity=1 --warning-severity=1

FILE: /Users/brettshumaker/Developer/phpcstesting/phpcstesting.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  7 | WARNING | Not using strict comparison for in_array; supply true for third argument. (WordPress.PHP.StrictInArray.MissingTrueStrict)
  7 | WARNING | Variable $x is undefined. (WordPressVIPMinimum.Variables.VariableAnalysis.UndefinedVariable)
  7 | WARNING | Found: ==. Use strict comparisons (=== or !==). (WordPress.PHP.StrictComparisons.LooseComparison)
 15 | ERROR   | %s() is uncached. If this is being used to query a remote file please use wpcom_vip_file_get_contents() instead. If it's used for a local file please use WP_Filesystem
    |         | instead. Read more here: https://vip.wordpress.com/documentation/using-wp_filesystem-instead-of-direct-file-access-functions/
    |         | (WordPressVIPMinimum.VIP.FetchingRemoteData.fileGetContentsUknown)
 16 | ERROR   | File operations should use WP_Filesystem methods instead of direct PHP filesystem calls. Found: fopen(). Read more here:
    |         | https://vip.wordpress.com/documentation/using-wp_filesystem-instead-of-direct-file-access-functions/ (WordPress.WP.AlternativeFunctions.file_system_read_fopen)
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 60ms; Memory: 6Mb
phpcs * --standard=WordPressVIPMinimum -s -d --severity=1 --warning-severity=1

FILE: /Users/brettshumaker/Developer/phpcstesting/phpcstesting.php
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 5 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  7 | WARNING | Not using strict comparison for in_array; supply true for third argument. (WordPress.PHP.StrictInArray.MissingTrueStrict)
  7 | WARNING | Variable $x is undefined. (WordPressVIPMinimum.Variables.VariableAnalysis.UndefinedVariable)
  7 | WARNING | Found: ==. Use strict comparisons (=== or !==). (WordPress.PHP.StrictComparisons.LooseComparison)
 13 | ERROR   | add_user_meta() usage is highly discouraged on WordPress.com VIP due to it being a multisite, please see
    |         | https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#wp_users-and-user_meta.
    |         | (WordPressVIPMinimum.VIP.RestrictedFunctions.user_meta_add_user_meta)
 15 | WARNING | `file_get_contents()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead.
    |         | (WordPressVIPMinimum.VIP.FetchingRemoteData.fileGetContentsUknown)
 16 | WARNING | File operations should use WP_Filesystem methods instead of direct PHP filesystem calls. Found: fopen() (WordPress.WP.AlternativeFunctions.file_system_read_fopen)
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 54ms; Memory: 6Mb
GaryJones commented 5 years ago

I'm not sure why the --severity=1 flag isn't applying to warnings, because it should be according to the docs.

Worth raising that on the PHPCS repo, even if only to get clarification on the intended usage.

brettshumaker commented 5 years ago

Closing this out. I was doing something silly. I had seen the -d flag used in an example in slack and I didn’t understand its usage. -d is used to set php.ini values and since I was putting --severity=1 after -d it was trying to set severity in php.ini and wasn’t seeing it as a phpcs flag.

The following command works as expected:

phpcs /path/to/code --standard=WordPress-VIP-Go -s --severity=1