Add Support for PHPCS Standards / Rulesets and Related Bug Fixes #216
Conversation
The output of `getBindings()` is largely just a list of the class names registered in the container, but it doesn't contain information on how those classes are actually configured. As such, changing `ecs.php` in a way that updates sniff / fixer rules would not invalidate the cache, meaning those new settings would not be checked against any unchanged files. My approach here still isn't perfectly reliable, but I think it's a usable middle ground. We still report all the registered classes, but now I explicitly pull out the configuration data for sniffers and fixers to add to the serialized data. Then I also hash out the settings used for our sniffers, to support the upcoming PHPCS rulesets / standards support. The ECSConfig previously constructed as-needed, is now provided via DI. This is required for the upcoming feature, but has the added benefits of making sure the cache hash uses the hash for the exact same config that will be used for sniffing *and* improving the performance of sniffing (since every file is currently creating a config, triggering filesystem ops). Closes easy-coding-standard#30
This mostly involved redefining the entire code path on how we determine when sniffs are skipped over to reflect more what PHPCS does in the parent file WRT to sniff severity at a code-level granularity (as opposed to class-level). I, unfortunately, didn't see a great way to re-use their work directly via `parent::` method invocation, we just deviate too much. All the additional `add*` methods are necessary on a sniff-by-sniff basis. *Technically* this is a fix for an existing bug, where some sniffs would be unable to report issues to ECS correctly, but they're not really widely used or only applied to warnings - which we universally ignored before - so no need to make a ticket about it. As you can see in the prior changes to `README.md`, standard / ruleset imports are very much a partially-supported feature. There's a lot I just can't replicate without a significant undertaking doing refactors. And I'm not sure it would fit the ECS founders' intention of his tooling, either - I certainly want to respect that. I left in the previously defined warning escalations, which completely override ruleset / standard severity from PHPCS configs, though I'm not quite sure why we do that or if my integration is exactly how we want to continue integrating it... Closes easy-coding-standard#128
| @@ -58,6 +59,9 @@ return ECSConfig::configure() | |||
| ->withRules([ | |||
| ArraySyntaxFixer::class, | |||
| ]) | |||
| // Warnings for included PHPCS rules are disabled by default, | |||
| // but they can be enabled manually. | |||
| ->withWarnings() | |||
There was a problem hiding this comment.
Hey, thanks for many contributions 🥳
Let's take it one by one to ship them faster :)
I like this feature - please separate it to own PR 👍
There was a problem hiding this comment.
Sure thing!
Just so we're on the same page, do you just want me to pull out the with[out]Warnings methods/parameters with warnings disabled by default, then enabled when using withSnifferStandards?
There was a problem hiding this comment.
Thanks 👍
Lets start with warnings as a standalone feature. Without any dependency on withSnifferStandards.
There was a problem hiding this comment.
Sorry for the delay, been focused on "work" work! I'll be able to get back to this soon, and I'll create four five separate PRs for (in this order):
- The additional test script. Submitted via Add Convenience Composer
testScript #227. - The cache invalidation fix. Submitted via Improve Reliability of Configuration Caching #226.
- Duplicate error reports for PHPCS rules. Submitted via Stop Reporting Duplicate Errors for Sniffs in the Presence of Fixable Changes #228.
- Missing logic to skip self-disabled PHPCS rules during parsing. Submitted via Allow PHPCS Sniffs to Disable Themselves #229.
- Add support for PHPCS warnings.
I'll make sure each is appropriately tested, of course. I'll then rebase this branch and force merge to my fork. Hopefully that'll cut down on the effort required to review these submissions.😄
Let me know if you're opposed to the plan above or have any suggestions. Thanks for your time!
There was a problem hiding this comment.
4/5 of the above have been prepared and are ready for your review! The last changes to allow PHPCS warnings has dependencies on the others though, so I'll need to wait for those PRs to be merged before coming back to finish it.
I appreciate your time, Tomas! Sorry again for taking so long. 😭
There was a problem hiding this comment.
Hey, Tomas! Don't mean to be a bother, but is there any chance we could take a look at some of those above PRs?
There was a problem hiding this comment.
Since it's been a while, I'll need to revisit the PHPCS warnings support feature to see what needs to be done to support it. 😅
|
Most of these features were split into smaller PR that were reviewed and either declined or accepted. Closing this one then 👍 Thank you for the work you've done on these 👏 |
Parent Tickets:
This gives us improved support for PHPCS Sniffs by allowing users to import either the built-in standards or rulesets provided by themselves or third parties, then adjusting how we handle sniffs to more closely match PHPCS itself. The changes required for this are quite involved, so I recommend checking out the commit messages themselves, where I've already gone into detail on the "why" for most of these changes.
Some existing issues were identified during my self-testing, one of which was already reported, and I've made sure to fix them.
I'm leaving this PR as a draft for now, since I still need to write tests to cover the new configuration options. However, in it's current state I have made sure it passes all linting and then manually tested it (against itself, comparing to direct PHPCS output) for:
phpcs.xml.dist.The changes / configs for the above testing were not committed.
It ran well and matched what PHPCS itself reported, so everything should be fine.