phpcompat icon indicating copy to clipboard operation
phpcompat copied to clipboard

Prevent false positives by ignoring commented lines

Open ryanshoover opened this issue 7 years ago • 5 comments

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
}

ryanshoover avatar Aug 17 '18 21:08 ryanshoover

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.

jrfnl avatar Aug 17 '18 21:08 jrfnl

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.

ryanshoover avatar Aug 17 '18 22:08 ryanshoover

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:

  • https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.2.0
  • https://github.com/squizlabs/PHP_CodeSniffer/issues/604

jrfnl avatar Aug 17 '18 22:08 jrfnl

@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

ryanshoover avatar Aug 19 '18 01:08 ryanshoover

@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

jrfnl avatar Aug 19 '18 02:08 jrfnl