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 | Deprecate and remove support for Sniffs not implementing the PHPCS Sniff interface #694

Open
jrfnl opened this issue Nov 17, 2024 · 2 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Nov 17, 2024

Current Situation

Sniffs should (of course) implement the PHP_CodeSniffer\Sniffs\Sniff interface, either directly or via inheritance, and I expect all sniffs do.

However, there is no defensive coding in place at this time to safeguard this.

The consequences of this are as follows:

  1. Sniffs which do not implement the Sniff interface, but do have a register() and process() method which comply with the interface will run without problems.
  2. Sniffs which do not implement the Sniff interface, but do have a register() and process() method, but the method signatures do not comply with the interface are likely to throw PHP errors.
  3. Sniffs which do not implement the Sniff interface, and are missing the register() method will break a scan run with a Fatal Error during the loading of the ruleset.
  4. Sniffs which do not implement the Sniff interface, and are missing the process() method will break a scan run with a Fatal Error, but only if the token(s) the sniff is listening for are encountered, meaning the broken sniff may go unnoticed if the token() are some of the rarely used ones.

Proposal

  • Soft deprecate support for sniffs not implementing the Sniff interface, but which do work properly, in the next 3.x minor.
  • Add some error prevention in the next 3.x minor to guard against the Fatals mentioned above in point 3 and 4.
    This would take the form of showing a warning to the end-user and ignoring the sniff to prevent scans from breaking and improve the end-user experience.
  • Hard deprecate support for sniffs not implementing the Sniff interface in what is expected to be the last 3.x minor.
  • Remove support for sniffs not implementing the Sniff interface in 4.0.0.

In practice, removing support will mean that sniffs which do not implement the Sniff interface will no longer be included in the run and that a warning will be presented to the end-user letting them know the sniff is being ignored.

Terminology used

Soft deprecation: deprecation via changelog mention and/or announcement only.
Hard deprecation: a deprecation notice will be shown at runtime, but will not affect the exit code of PHPCS.

Impact

The impact of this change is expected to be low to non-existent for properly coded external standards.

For those edge-case external standards where this change may have an impact, the change will improve the end-user experience and may help the developers discover these type of problems sooner.

Related issues

Loosely related to #689

Opinions ?

/cc @asispts @dingo-d @fredden @GaryJones @greg0ire @kukulich @michalbundyra @Ocramius @sirbrillig @stronk7 @weierophinney @wimg

@jrfnl jrfnl added this to the 4.0.0 milestone Nov 17, 2024
@jrfnl jrfnl changed the title Deprecate and remove support for Sniffs not implementing the PHPCS Sniff interface 4.0 | Deprecate and remove support for Sniffs not implementing the PHPCS Sniff interface Nov 17, 2024
@jrfnl jrfnl self-assigned this Nov 17, 2024
@greg0ire
Copy link

I think I'd use hard deprecations for everything, because it's a better experience: you can see if you forgot to migrate something programmatically. If end users are annoyed by the warnings and contribute fixes, it alleviates the workload of the maintainers.

@asispts
Copy link

asispts commented Nov 18, 2024

Agree. Since the support will be removed in v4, IMHO, it's better to use hard deprecation.

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

3 participants