Improve sniff handling of modern PHP code

[jrfnl]

trafficstars

While WP still has a minimum requirement of PHP 5.2 and a lot of related projects code accordingly, there are also numerous projects out there which have set their plugin/theme minimum requirement at PHP 5.3 or higher.

Most sniffs currently don't take things like namespaces into account so the below code would trigger an error (even though the fwrite function in this case is namespaced).

While this may not be the best example (or good practice in the first place), I would like to suggest we review code to see if we can make it more forward compatible with modern PHP code.

namespace MyNamespace;

function fwrite() {
	// A compat layer or something.
}

class My_Class {
	function do_something() {
		fwrite( $param );
	}
}

[jrfnl]

Another typical one which is currently not or hardly taken into account: T_HEREDOC

[jrfnl]

Let's try & keep track of the reviews done:

Globally checked (I might well have missed something) that sniffs allow correctly for:

• [x] short array tokens
• [x] closures/anonymous functions
• [x] anonymous classes
• [x] interfaces
• [x] traits
• [ ] try/catch/finally constructs
• [x] __NAMESPACE__, __TRAIT__, __DIR__ magic constants
• [x] T_NOWDOC and related tokens
• [x] T_HEREDOC and related tokens
• [ ] namespaces
• [ ] use statements
• [ ] type hints
• [ ] yield (for generators) and yield from
• [x] ternary without middle $x ? : $y
• [x] new operators: T_POW, T_POW_EQUAL, T_ELLIPSIS, T_SPACESHIP, T_COALESCE, T_COALESCE_EQUAL

N.B.: I don't "own" this ticket. Everyone is warmly invited to contribute, both in ideas of what to check for as well as doing the reviews!

[dingo-d]

Just a check: would this handle things like

namespace Custom_Namespace;

class My_Class {

  public function get_posts() {
    $args = array(
      'post_type'   => 'post',
      'post_status' => 'publish',
      'perm'        => 'readable',
    );
    
    $query = new WP_Query( $args ); // This would trigger error, since it's not called from global namespace like \WP_Query.
    
    if ( $query->have_posts() ) {
      while ( $query->have_posts() ) {
        $query->the_post();
      }
      wp_reset_postdata();
    } else {
      // no posts found
    }
  }

}

Basically any WP class called within a namespace should be called from a global namespace \WP_Query in the above example.

[jrfnl]

@dingo-d

Basically any WP class called within a namespace should be called from a global namespace \WP_Query in the above example.

Or should have a use statement ;-)

That's not actually what this issue is about. The current issue is about making the existing sniffs more resilient when they encounter modern PHP code, including prevent false positives/negatives.

I.e. say there would be a sniff which would detect the use of WP_Query, that sniff should not trigger on your code example as the WP_Query used in your code is not the WP class, but a custom Custom_Namespace\WP_Query class.

There is currently no sniff available in WPCS which checks that WP native classes when used in a namespaced context are prefixed with a \ or have a use statement. I think it could be a valid feature request for such a sniff to be created, but that should be discussed in another issue.

In the mean time, there might be a sniff in the Slevomat standard which could help detect these issues.

[dingo-d]

There is currently no sniff available in WPCS which checks that WP native classes when used in a namespaced context are prefixed with a \ or have a use statement. I think it could be a valid feature request for such a sniff to be created, but that should be discussed in another issue.

Yeah, this was basically what I was thinking about, but I didn't want to create an issue if there is no need for it 🙂

Thanks for clarifying it out :+1:

[jrfnl]

All sniffs have been reviewed and fixed to support modern PHP in as far as my imagination reached.

There is still a known issue with resolving namespaces/use statements. That is expected to be fixed via a future PHPCSUtils version and abstract sniffs provided by PHPCSUtils.

[westonruter]

Speaking of use statements, we just noticed in https://github.com/WordPress/plugin-check/pull/334#discussion_r1423191350 that WPCS spacing isn't currently enforced around use. Core always includes a space after use, so WPCS should be making this change:

  add_action(
  	'populate_options',
- 	static function () use( $permalink_structure ) {
+ 	static function () use ( $permalink_structure ) {
  		add_option( 'permalink_structure', $permalink_structure );
  	}
  );

[jrfnl]

@westonruter Interesting. In my original tests that resulted in a duplicate error being thrown - both from Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterUse as well as Generic.WhiteSpace.LanguageConstructSpacing, but with your code sample, no error is thrown from the Generic.WhiteSpace.LanguageConstructSpacing.

Looks like the duplication exists only when there is too much whitespace, not when there is not enough.

This is related to https://github.com/WordPress/WordPress-Coding-Standards/blob/15c3037c35b2867ad87c81e5d87d2d92ecee59b5/WordPress-Core/ruleset.xml#L221-L224, which was introduced in #2328.

I suppose we can remove that exclusion in favour of sometimes receiving duplicate messages.

[jrfnl]

@westonruter See #2415