Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core/Extra: tweak the inclusion of the Modernize.FunctionCalls.Dirname sniff #2324

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jul 26, 2023

Follow up on #2137

As WP Core has now dropped support for PHP < 7.0, it can start using the dirname() $levels parameters. Think:

// PHP < 7.0.
$path = dirname( dirname( dirname( __DIR__ ) ) );

// PHP 7.0+.
$path = dirname( __DIR__, 3 );

The Modernize.FunctionCalls.Dirname sniff we include also includes a check (and fixer) for that, so we can now include that sniff completely in the Core ruleset.

For now, I'm proposing to silence the error code related to the PHP 7.0 modernization opportunity (again) for the Extra ruleset as not all plugins/themes will have dropped support for PHP < 7.0 yet.

To be on the safe side, I'm explicitly excluding the whole sniff from the ruleset used for WPCS itself as WPCS still has a PHP 5.4 minimum.

…me` sniff

Follow up on 2137

As  WP Core has now dropped support for PHP < 7.0, it can start using the `dirname()` `$levels` parameters.
Think:
```php
// PHP < 7.0.
$path = dirname( dirname( dirname( __DIR__ ) ) );

// PHP 7.0+.
$path = dirname( __DIR__, 3 );
```

The `Modernize.FunctionCalls.Dirname` sniff we include also includes a check (and fixer) for that, so we can now include that sniff completely in the `Core` ruleset.

For now, I'm proposing to silence the error code related to the PHP 7.0 modernization opportunity (again) for the `Extra` ruleset as not all plugins/themes will have dropped support for PHP < 7.0 yet.

To be on the safe side, I'm explicitly excluding the whole sniff from the ruleset used for WPCS itself as WPCS still has a PHP 5.4 minimum.
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other sniffs where a minimum_supported_php_version attribute could be beneficial to configuring them?

@GaryJones GaryJones merged commit ca2da67 into develop Jul 26, 2023
35 checks passed
@GaryJones GaryJones deleted the feature/core-update-dirname-sniff-handling branch July 26, 2023 19:06
@jrfnl
Copy link
Member Author

jrfnl commented Jul 26, 2023

Are there other sniffs where a minimum_supported_php_version attribute could be beneficial to configuring them?

Well, sort of.... but it's not that clear-cut...

PHPCS natively supports passing a php_version as a configuration option, but that's a single patch version option, like 70305 (= PHP 7.3.5), not a minimum/maximum option and the value cannot be changed using properties.

PHPCS natively has a few sniffs which look at that config variable and if it is not available, PHPCS presumes the PHP version on which the sniff is currently being run.

PHPCSExtra also has a few sniffs which look at that config variable, but if it is not available, PHPCSExtra will not presume the PHP version on which the sniff is currently being run, but err on the side of caution (as the option is too little known, so that would too easily lead to problems).

The Modernize.FunctionCalls.Dirname doesn't actually look at the php_version config variable (though maybe it should).
There are three other PHPCSExtra sniffs which do take the php_version config variable into account and will adapt their behaviour based on the found value if available.

The sniff documentation in PHPCSExtra details exactly which sniffs and what the behavioural difference is when the php_version is set (or not).

Does that help ?

@GaryJones
Copy link
Member

Yes, thank you. I don't think I was aware of the php_version option.

@jrfnl
Copy link
Member Author

jrfnl commented Jul 26, 2023

@GaryJones Few people are, which is why I'm so careful about not drawing conclusions based on the absence of the setting for PHPCSExtra sniffs (and why I keep pointing to the option in the sniff docs for PHPCSExtra).

@jrfnl
Copy link
Member Author

jrfnl commented Aug 9, 2023

FYI: I've updated the Modernize/Dirname sniff in PHPCSExtra now to also respect the php_version, if available: PHPCSStandards/PHPCSExtra#261 (not yet released, but will be in the next version)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants