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

Common::getSniffCode(): be more lenient about sniffs not following naming conventions #676

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 12, 2024

Description

Sniff classes should always follow the directory layout and naming conventions for sniffs as per the Tutorial.

However, before the change made in #524, when given a sniff class name for a sniff not following the naming conventions, the Common::getSniffCode() method would return a "sniff code" anyway. And even though the sniff code would be unconventional and not really valid, it would still work.

Since #524 this is no longer the case.

Given the above, the change from #524 breaks custom rulesets which use <rule> includes for individual sniff files not complying with the naming conventions. Breaking changes can only be made in majors, so this commit reverts the problematic part of the change from #524.

Includes additional tests safeguarding the (broken) behaviour which needs to be maintained (at least until the next major).

Suggested changelog entry

  • Fixed InvalidArgumentException when a ruleset includes a sniff by file name and the included sniff does not comply with the PHPCS naming conventions.
    • Notwithstanding this fix, it is strongly recommended to ensure custom sniff classes comply with the PHPCS naming conventions

Related issues/external references

Fixes #675

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

@tomasnorre
Copy link

I can confirm that this works in the setup of https://github.com/exercism/php

@jrfnl
Copy link
Member Author

jrfnl commented Nov 13, 2024

@tomasnorre Thanks for testing and confirming!

@williamdes Just checking if you've had the chance to test this PR as well ? I'd like to get this released ASAP, but would prefer to do this with confirmation from you that this fixes your problem too.

@williamdes
Copy link

@williamdes Just checking if you've had the chance to test this PR as well ? I'd like to get this released ASAP, but would prefer to do this with confirmation from you that this fixes your problem too.

Well, since I applied the standard way of things I no longer have this issue
I may test in the following days with the old code

…ming conventions

Sniff classes _should_ always follow the directory layout and naming conventions for sniffs as per the [Tutorial](https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Coding-Standard-Tutorial).

However, before the change made in 524, when given a sniff class name for a sniff not following the naming conventions, the `Common::getSniffCode()` method would return a "sniff code" anyway. And even though the sniff code would be unconventional and not really valid, it would still _work_.

Since 524 this is no longer the case.

Given the above, the change from 524 breaks custom rulesets which use `<rule>` includes for individual sniff files not complying with the naming conventions. Breaking changes can only be made in majors, so this commit reverts the problematic part of the change from 524.

Includes additional tests safeguarding the (broken) behaviour which needs to be maintained until the next major.

Fixes 675
@jrfnl
Copy link
Member Author

jrfnl commented Nov 13, 2024

@williamdes Thanks for getting back to me. In that case, I think I'll just go ahead and get this merged for release before the end of the week.

I'll rebase the PR (without changes) now - as the CI setup has undergone some changes and some new required builds are missing. Once the build has passed, I'll get this merged.

@jrfnl jrfnl force-pushed the feature/675-common-sniffcode-be-more-lenient branch from 8e2190c to 71b4524 Compare November 13, 2024 20:55
@jrfnl jrfnl merged commit 7d33bcd into master Nov 13, 2024
70 checks passed
@jrfnl jrfnl deleted the feature/675-common-sniffcode-be-more-lenient branch November 13, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants