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

✨ Native handling of sniff deprecations #281

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jan 16, 2024

Background

There are quite a few sniffs slated for removal in PHPCS 4.0 - 45 so far, to be exact - and save for two sniffs which have received a deprecation mention in the changelogs, this has only been announced in issues (squizlabs/PHP_CodeSniffer#2448 + squizlabs/PHP_CodeSniffer#2471) in the Squizlabs repo and should therefore only be considered "known" to a very small group of people.

Increasing awareness of the upcoming sniff removals should allow for a smoother upgrade experience to PHPCS 4.0 for end-users.

Aside from use by PHPCS itself, this feature can also be used by external standards to signal sniff deprecations to their end-users.

All in all, this feature should hopefully improve the end-user experience.

Policy for use of this feature in PHP_CodeSniffer itself

As per the notes in #188, the intention is for PHPCS itself to use this feature as follows:

  • Soft deprecate (changelog notice and @deprecated tag in sniff) sniffs during the lifetime of a major.
  • Hard deprecate sniffs, i.e. implement the DeprecatedSniff interface, in (what is expected to be) the last minor before the next major release.

External standards are, of course, free to apply a different deprecation policy.

Description

PHPUnit config: make sure unexpected output fails the tests

✨ Native handling of sniff deprecations

As per #164, this commit introduces a new feature to PHPCS: native handling of sniff deprecations.

A new PHP_CodeSniffer\Sniffs\DeprecatedSniff interface

This interface enforces the implementation of three new methods:

  • getDeprecationVersion(): string - the return value should be a non-empty string with the version in which the sniff was deprecated.
  • getRemovalVersion(): string - the return value should be a non-empty string with the version in which the sniff will be removed.
    If the removal version is not yet known, it is recommended to set this to: "a future version".
  • getDeprecationMessage(): string - the return value allows for an arbitrary message to be added, such as a replacement recommendation. If no additional information needs to be conveyed to end-users, an empty string can be returned.
    Custom messages are allowed to contain new lines.

Changes to the Ruleset class to allow for showing the deprecation notices

The Ruleset class contains two new methods:

  • hasSniffDeprecations(): bool to allow for checking whether a loading ruleset includes deprecated sniffs.
  • showSniffDeprecations(): void to display the applicable deprecation notices.

The implementation for showing the sniff deprecation notices is as follows:

  • No notices will be shown when PHPCS is running with the -q flag (quite mode).
  • No notices will be shown when PHPCS was giving the -e flag to "explain" a standard (list all sniffs).
  • No notices will be shown when PHPCS was asked to display documentation using the --generator=... CLI argument.
  • No notices will be shown when PHPCS is asked for information which doesn't involve loading a ruleset, such as running phpcs with the any of the following flags: -i (listing installed standards), --version, --help, --config-show, --config-set, --config-delete.
  • Only deprecation notices will be shown for active sniffs.
    This means that when --exclude=... is used and deprecated sniffs are excluded, no notices will be shown for the excluded sniffs.
    It also means that when --sniffs=... is used, deprecation notices will only be shown for those sniffs selected (if deprecated).
  • The deprecation notices have no impact on the PHPCS (or PHPCBF) run itself.
    • Deprecated sniffs will still be loaded and run.
    • Properties can be set on deprecated sniffs.
    • The exit codes of runs are not affected by whether or not a ruleset contains deprecated sniffs.
  • As things are, deprecation notices will show both for phpcs as well as phpcbf runs.
    I did considered silencing them by default for phpcbf. For now, however, I've decided against this as the whole point of showing deprecation notices is to increase awareness of upcoming changes and as a subsection of the PHPCS users only run phpcbf and rarely phpcs, silencing the notices for phpcbf could be counter-productive.

Additional implementation notes:

  • Any user set --report-width will be respected. This includes when the report-width is set to auto.
  • New lines in custom messages will be normalized to be suitable for the OS of the end-user.
  • The output will have select colourization if --colors is turned on.
  • The deprecation notices will not be included in generated reports saved to file using --report-file=... (and variants thereof).
  • If the interface is implemented incorrectly, a PHP_CodeSniffer\Exceptions\RuntimeException will be thrown.
    The intention is to add return type declarations to the interface in the future.

The complete new feature is extensively covered by tests.

Explain: mark deprecated sniffs as such

This commit makes a small adjustment to the output of the -e (explain) command.
Deprecated sniffs will now be marked with a trailing * asterix and if the standard contains deprecated sniffs, a line will show at the end of the output to explain that the * means that a sniff is deprecated.

Includes a test documenting and safeguarding this behaviour.

Suggested changelog entry

  • New PHP_CodeSniffer\Sniffs\DeprecatedSniff interface to allow for marking a sniff as deprecated.
    • If a ruleset loaded contains deprecated sniffs, the deprecation notices will be listed before sniffs are run.
    • Deprecated sniffs will still run and a ruleset including deprecated sniffs will have no impact on the exit code for a scan.
    • Sniff maintainers are advised to read through ✨ Native handling of sniff deprecations #281 for full details on how to use this feature for their own sniffs.

Related issues/external references

Related issues: #188, #276

Fixes #164

Types of changes

  • New feature (non-breaking change which adds functionality)

@jrfnl
Copy link
Member Author

jrfnl commented Jan 16, 2024

Screenshots

List of sniff deprecations (example output as used when testing the feature)

image

What it would look like in practice for the deprecated MySource standard

image

Output of "explain" for a standard containing deprecated sniffs

image

@jrfnl
Copy link
Member Author

jrfnl commented Jan 22, 2024

Unless anyone leaves a comment/feedback on this PR, I will merge it tomorrow.

As per 164, this commit introduces a new feature to PHPCS: native handling of sniff deprecations.

### Background

There are quite a few sniffs slated for removal in PHPCS 4.0 - 45 so far, to be exact - and save for two sniffs which have received a deprecation mention in the changelogs, this has only been announced in issues (squizlabs/PHP_CodeSniffer 2448 + squizlabs/PHP_CodeSniffer 2471) in the Squizlabs repo and should therefore only be considered "known" to a very small group of people.

Increasing awareness of the upcoming sniff removals should allow for a smoother upgrade experience to PHPCS 4.0 for end-users.

Aside from use by PHPCS itself, this feature can also be used by external standards to signal sniff deprecations to _their_ end-users.

All in all, this feature should hopefully improve the end-user experience.

### Policy for use of this feature in PHP_CodeSniffer itself

As per the notes in 188, the intention is for PHPCS itself to use this feature as follows:
* Soft deprecate (changelog notice and `@deprecated` tag in sniff) sniffs during the lifetime of a major.
* Hard deprecate sniffs, i.e. implement the `DeprecatedSniff` interface, in the last minor before the next major release.

External standards are, of course, free to apply a different deprecation policy.

### Implementation details

This commit introduces:

#### A new `PHP_CodeSniffer\Sniffs\DeprecatedSniff` interface

This interface enforces the implementation of three new methods:
* `getDeprecationVersion(): string` - the return value should be a non-empty string with the version in which the sniff was deprecated.
* `getRemovalVersion(): string` - the return value should be a non-empty string with the version in which the sniff will be removed.
    If the removal version is not yet known, it is recommended to set this to: "a future version".
* `getDeprecationMessage(): string` - the return value allows for an arbitrary message to be added, such as a replacement recommendation. If no additional information needs to be conveyed to end-users, an empty string can be returned.
    Custom messages are allowed to contain new lines.

#### Changes to the `Ruleset` class to allow for showing the deprecation notices

The Ruleset class contains two new methods:
* `hasSniffDeprecations(): bool` to allow for checking whether a loading ruleset includes deprecated sniffs.
* `showSniffDeprecations(): void` to display the applicable deprecation notices.

The implementation for showing the sniff deprecation notices is as follows:
* No notices will be shown when PHPCS is running with the `-q` flag (quite mode).
* No notices will be shown when PHPCS was giving the `-e` flag to "explain" a standard (list all sniffs).
* No notices will be shown when PHPCS was asked to display documentation using the `--generator=...` CLI argument.
* No notices will be shown when PHPCS is asked for information which doesn't involve loading a ruleset, such as running `phpcs` with the any of the following flags: `-i` (listing installed standards), `--version`, `--help`, `--config-show`, `--config-set`, `--config-delete`.
* Only deprecation notices will be shown for _active_ sniffs.
    This means that when `--exclude=...` is used and deprecated sniffs are excluded, no notices will be shown for the excluded sniffs.
    It also means that when `--sniffs=...` is used, deprecation notices will only be shown for those sniffs selected (if deprecated).
* The deprecation notices have no impact on the PHPCS (or PHPCBF) run itself.
    - Deprecated sniffs will still be loaded and run.
    - Properties can be set on deprecated sniffs.
    - The exit codes of runs are not affected by whether or not a ruleset contains deprecated sniffs.
* As things are, deprecation notices will show both for `phpcs` as well as `phpcbf` runs.
    I did considered silencing them by default for `phpcbf`. For now, however, I've decided against this as the whole point of showing deprecation notices is to increase awareness of upcoming changes and as a subsection of the PHPCS users only run `phpcbf` and rarely `phpcs`, silencing the notices for `phpcbf` could be counter-productive.

Additional implementation notes:
* Any user set `--report-width` will be respected. This includes when the `report-width` is set to `auto`.
* New lines in custom messages will be normalized to be suitable for the OS of the end-user.
* The output will have select colourization if `--colors` is turned on.
* The deprecation notices will not be included in generated reports saved to file using `--report-file=...` (and variants thereof).
* If the interface is implemented incorrectly, a `PHP_CodeSniffer\Exceptions\RuntimeException` will be thrown.
    The intention is to add return type declarations to the interface in the future.

The complete new feature is extensively covered by tests.

Related issues: 188, 276

Fixes 164
This commit makes a small adjustment to the output of the `-e` (explain) command.
Deprecated sniffs will now be marked with a trailing `*` asterix and if the standard contains deprecated sniffs, a line will show at the end of the output to explain that the `*` means that a sniff is deprecated.

Includes a test documenting and safeguarding this behaviour.
@jrfnl jrfnl force-pushed the feature/164-handle-sniff-deprecation-natively branch from 014bc9c to 3fb0d95 Compare January 25, 2024 15:51
@jrfnl
Copy link
Member Author

jrfnl commented Jan 25, 2024

Rebased without changes to be sure. Merging once the build passes.

@jrfnl jrfnl merged commit 905c6fb into master Jan 25, 2024
44 checks passed
@jrfnl jrfnl deleted the feature/164-handle-sniff-deprecation-natively branch January 25, 2024 15:59
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.

New Feature: handle deprecation of sniffs natively
1 participant