Prevent false positives by ignoring commented lines

ryanshoover commented 6 years ago

An ongoing "scary" issue with the compatibility checker is the false positives from plugins that safely use deprecated code (see the entire whitelist of plugins). But plugin authors can clear their code for PHP Compatibility by using the PHPCS exemption comments. Adding a simple comment to their code will prevent the false positives.

Can we encourage plugin authors to use PHPCS exemptions in their code to white-list known good behavior?

No extra coding is needed for phpcompat. Rather, this is a community relations request.

This is a global catch-all to prevent testing on any PHP compatibility concerns.

// phpcs:ignore PHPCompatibility

Example to fix #184:

if ( $wpdb->use_mysqli ) {
    $output = mysqli_real_escape_string( $wpdb->dbh, $input );
} else {
    // phpcs:ignore PHPCompatibility.PHP.RemovedExtensions
    $output = mysql_real_escape_string( $input, $wpdb->dbh );
}

Example to fix #189:

protected function get_raw_post_data() {
    // phpcs:disable PHPCompatibility.PHP.RemovedGlobalVariables
    global $HTTP_RAW_POST_DATA;

    if ( ! isset( $HTTP_RAW_POST_DATA ) ) {
        $HTTP_RAW_POST_DATA = file_get_contents( 'php://input' );
    }

    return $HTTP_RAW_POST_DATA;
    // phpcs:enable PHPCompatibility.PHP.RemovedGlobalVariables
}

jrfnl commented 6 years ago

Why not just use the PHPCS native ignore/disable/enable comments ? That way, those comments will work independently of in what manner PHPCS is run, like using PHPCS itself, using this plugin or using Tide.

ryanshoover commented 6 years ago

Whether something is PHP 7.2 compatible or whether it follows a project's linting styles are two separate concerns. Just because I flag something as "don't check for compatibility" doesn't mean it shouldn't be checked for code style.

Seems like we need a separate flag for this need.

jrfnl commented 6 years ago

You are missing the point. In PHPCS 3.2.0, new ignore/disable/enable annotations were introduced via which you can modularly ignore just one particular sniff or error code, so the CS checks would still work.

I though you knew that as the syntax you propose is nearly the same ?

See the following for more details:

ryanshoover commented 6 years ago

@jrfnl Thanks for the extra info! My confusion came from misunderstanding the backbone behind phpcompat.

I'm updating the issue to reflect phpcs's commenting style

jrfnl commented 6 years ago

@ryanshoover :+1:

Timing-wise for the community-outreach / documentation update, I would strongly recommend to wait until PHPCompatibility 9.0.0 has been released and this plugin has been updated to use the PHPCompatibilityWP version compatible with PHPCompatibility 9.0.0. I'm not sure yet what that release will be called, but I suspect it will 2.0.0.

As announced in the PHPCompatibility 8.2.0 changelog:

The next version of PHPCompatibility will include a major directory layout restructuring which means that the sniff codes of all sniffs will change.

Most sniffs will be moved into categories in PHPCompatibility 9.0.0 and some will be renamed as well. We're trying to get this sorted ASAP to prevent too many people having to update their inline annotations. Expect the release before the end of the summer.

For more information:

https://github.com/PHPCompatibility/PHPCompatibility/issues/688 (point 5)
The tracking issue https://github.com/PHPCompatibility/PHPCompatibility/issues/692 and the original issue https://github.com/PHPCompatibility/PHPCompatibility/issues/601

Also: https://github.com/PHPCompatibility/PHPCompatibilityWP

wpengine / phpcompat

Prevent false positives by ignoring commented lines #195