PHP_CodeSniffer icon indicating copy to clipboard operation
PHP_CodeSniffer copied to clipboard

4.0 | Show error when user tries to change an unchangeable PHP setting

Open jrfnl opened this issue 10 months ago • 2 comments

Is your feature request related to a problem?

Currently, when a user tries to change a PHP ini option which cannot be changed at runtime, it would be silently ignored (by PHP, PHPCS would still try to handle it, but would not report that PHP did not change the value).

Describe the solution you'd like

I propose to change this behaviour to throw an error instead to alert the end-user to the invalid ini setting passed.

Additional context (optional)

Inspired by #415

This change will go into PHPCS 4.0 as the new error could cause build failures for pre-existing users of PHP_CodeSniffer, so should be considered a breaking change.

Settings for optional PHP extensions

The one thing to still have a think about: What to do when a user tries to change an ini setting for an extension which may or may not be available ?

Ini settings can be passed from the command line -d ini_name=value or via a custom ruleset <ini name="short_open_tag" value="On"/>, but especially with a ruleset, they cannot be passed conditionally.

If the setting applies to a PHP extension which may be turned off, I'm not sure an error should be thrown. In that specific case, it might be better to continue to silently ignore the setting as that will allow ruleset maintainers to set a preferred ini settings for an optional extension, like MbString or Iconv, without the new error breaking PHPCS runs for users which have the extension turned off.

The down-side of that will be, that typos in ini settings will not be caught nor flagged, as there is no way to distinguish between a non-existent ini setting due to an extension not being loaded or a non-existent ini setting due to a typo in the ini name.

  • [x] I intend to create a pull request to implement this feature.

jrfnl avatar Mar 25 '24 02:03 jrfnl

It would be ideal to add a warning in 3.x and switch this to an error in 4.0.0. However, I understand that there is no current functionality for showing warnings of this type, and adding that feature seems out of scope here. Perhaps when that feature (ability to show warnings) has been created, we can add a warning for settings which don't exist (which covers the typo case without throwing errors for the extension-not-loaded case).

fredden avatar Mar 25 '24 19:03 fredden

It would be ideal to add a warning in 3.x and switch this to an error in 4.0.0. However, I understand that there is no current functionality for showing warnings of this type, and adding that feature seems out of scope here.

Agreed and yes, PHPCS currently only errors on invalid settings/ruleset/config issues. Adding a layer to allow for warnings and collecting those in a way that they can be displayed in a structured way to the end-user would be great, but for the moment there are other priorities, so that'll have to wait.

Such a layer could also possibly be used for/has a tie in with issue #30.

Perhaps when that feature (ability to show warnings) has been created, we can add a warning for settings which don't exist (which covers the typo case without throwing errors for the extension-not-loaded case).

Well, it would have to be a warning for both (typos + settings for extensions which are not loaded) as I have not been able to find a way to distinguish between those two situations. But a non-blocking warning should be fine (and informative) for both cases.

jrfnl avatar Mar 26 '24 01:03 jrfnl