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

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

Open
1 task done
jrfnl opened this issue Mar 25, 2024 · 2 comments
Open
1 task done

Comments

@jrfnl
Copy link
Member

jrfnl commented Mar 25, 2024

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.

  • I intend to create a pull request to implement this feature.
@fredden
Copy link
Member

fredden commented Mar 25, 2024

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).

@jrfnl
Copy link
Member Author

jrfnl commented Mar 26, 2024

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.

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

No branches or pull requests

2 participants