Suggestion: new sniff to detect code inefficiencies

[jrfnl]

What's the general opinion on having a sniff to detect the following type of code patterns and throw warnings when detected ? Useful or not ?

echo sprintf( 'string %s', 'replacement'); // Should use `printf()` instead.
echo esc_html__( 'string', 'domain' ); // Should use `esc_html_e()` instead.
echo selected( $selected, $current, false ); // Should use `selected( $selected, $current )` instead.
echo get_search_form( false ); // Should use `get_search_form()` instead.

Of course, it would need to be verified that such a call is not followed by a concatenation adding more text, but that shouldn't be too hard to do.

echo sprintf( 'string %s', 'replacement') . 'something else'; // This is fine.

For a list of functions which have a $echo = true toggle as one of the parameters, Google Search will probably work better than the wp.org search.

Oh... and if you like the idea, more examples of code patterns to listen for would be welcome!

[GaryJones]

I like this, and I'd like the option to be able to extend the list of suggested replacements.

For instance, there is some code right now that does:

function genesis_structural_wrap( $context = '', $output = 'open', $echo = true ) {
    // ...Do the logic to populate $output.

    if ( $echo ) {
        echo $output;

        return null;
    } else {
        return $output;
    }

}

My experimental code currently has:

function genesis_get_structural_wrap( $context = '', $output = 'open' ) {

    // ...Do the logic to popuate $output.
    return $output;
}

function genesis_structural_wrap( $context = '', $output = 'open', $deprecated = null ) {

    if ( null !== $deprecated ) {
        $message = 'The default is true, so remove the third argument.';

        if ( false === (bool) $deprecated ) {
            $message = 'Use `genesis_get_structural_wrap()` instead.';
        }

        _deprecated_argument( __FUNCTION__, '2.7.0', $message );
    }

    $output = genesis_get_structural_wrap( $context, $output );

    // Apply original default value.
    $deprecated = null === $deprecated ? true : $deprecated;

    if ( false === (bool) $deprecated ) { // Kept for backwards compatibility.
        return $output;
    }

    echo $output;

}

It seems to work (confirmed with unit tests), but of course, will only show up the deprecated message notice at runtime. Being able to specify a recommended replacement in phpcs.xml.dist would mean that this could be detected when writing the code in the first place.

---

Another set, though perhaps trickier to handle, is where echo is an array key in an args array i.e.

echo wp_terms_checklist( ..., [ ..., 'echo'=false] );

[jrfnl]

@GaryJones I'd initially like to focus this ticket on PHP / WP Core constructs.

I'd like the option to be able to extend the list of suggested replacements.

I will consider that for a later stage as that will not be easy of there are various variations of code patterns to consider and I'm not all that sure, we could allow for that when accepting input from the ruleset.

[jrfnl]

Related #346

[kkmuffme]

I think the use of (s)printf when no argument is used could be added.

e.g.

sprintf( __( 'hello', 'textdomain' ) );
should be:
__( 'hello', 'textdomain' );

also applies to printf or when esc_html__ (or esc_html_x,...) is used

[jrfnl]

For PHP functions, I believe this should be handled by a (new) sniff to be added to PHPCSExtra. For WordPress specific situations, a new sniff in WPCS is needed. The sniff added in #1777 is a good example of that type of sniff.