-
Notifications
You must be signed in to change notification settings - Fork 51
feat: require squizlabs/php_codesniffer v4 #346
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
Conversation
f67256b
to
f1d2d7f
Compare
"slevomat/coding-standard": "^8.16", | ||
"squizlabs/php_codesniffer": "^3.7" | ||
"slevomat/coding-standard": "^8.23", | ||
"squizlabs/php_codesniffer": "^4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have make a hard bump here or could we support both major versions at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slevomat/coding-standard force the 4.0 too (in the 8.23). I may be possible to support both versions but there are some internal changes in PHPCS and it requires more work than I was willing to put into it…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simPod Eh... sorry, but no. What's on the right there has been supported by PHPCS since a very very long time, so definitely cross-version compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that I missed. But still, this CS relies heavily on Slevomat's CS which requires v4 so allowing v3 here has no point if I'm not missing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I saw that and respect @kukulich's decision in that.
In most cases, making sniffs PHPCS 3/4 cross-version compatibility isn't necessarily that hard, though yes, it does mean more work/higher cognitive load for a period of time and I totally get it if someone maintaining an external standard does not want to have to deal with that.
For the record: PHPCSUtils, PHPCSExtra and PHPCompatibility have all been made cross-version compatible.
09f5686
Thanks @simPod ! |
@simPod sorry for not asking earlier, but do you know the breaking changes are exactly? Are there new sniffs? Or will this maybe break some CIs somehow? |
Sure. I went through the upgrade guide and didn’t find anything that would significantly impact end users. From what I understand, the v4 release is mainly a cleanup and consolidation effort—removing legacy code and streamlining things. It seems to primarily affect sniff developers, while regular PHPCS users with clean configurations shouldn’t notice much change. I even did not have to touch tests here. (maybe Juliette can elaborate) |
Oh OK, then I guess it's not worth publishing 14.0.0 yet? |
IMO yes cuz otherwise people would be blocked from upgrading Slevomat CS to further versions (new sniffs and fixes) 🙃. E.g. I require both Doctrine CS and Slevomat CS. But Doctrine's one now blocks me from upgrading to Squizlab's v4 and thus upgrading Slevomat's. |
That's correct and anything I could think of that would impact end/dev users is documented in the upgrade guides:
Aside from that, there is also the changelog of course: The reason I made PHPCSUtils, PHPCSExtra and PHPCompatibility cross-version compatible is to allow for end-users which use a combination of external standards, some of which may be ready for 4.x and some of which may not.
|
Closes #345
Resolves #263