squizlabs
Allow the filter to return LocalFile objects to FileList
This gets us most of the way to fixing #2551. Now it's just a matter of writing a filter that updates the Config and/or Ruleset as appropriate while processing the directory tree.
We're using such a filter in Automattic/jetpack, which you can look at if you want to see how we intend to make use of this. Other people could do something similar but make other decisions, such as the looking for composer.json as suggested in #2551.
Since this change to Filter::current() may break expectations of existing filters, a filter must opt in to this change by overriding the protected $this->produceLocalFileObjects to true.
Howdy @gsherwood — I wanted to check-in to see your thoughts on this PR and set my expectations if this seems feasible to be included or if we needed to figure out an alternative approach toward 2551. Our particular goal is to be able to have per-directory rules or overrides. We have a similar setup for eslint and hopeful to have similar flexibility for phpcs.
Thank you!
@gsherwood FYI, we're now making use of this patch in production in our monorepo at https://github.com/Automattic/jetpack. It'd be nice if this could get reviewed.
@gsherwood We've been using this in production in our monorepo for three months now. It seems to work well.
Without having an express opinion on this PR, I would like to warn/point out that this contains a BC-break for external standards and other integrations, at the very least for those external standards using their own testing framework, but also for integrations using a custom runner. For that reason, if this change would be accepted, IMO it should go into 4.0.0.
@jrfnl Would you clarify? I tried to avoid having a BC break in here by making the filter have to specifically request the new behavior.
Or is it the thing about LocalFile needing to have reloadContent() called? I suppose we could change it to call that on demand in parse() if it hasn't already been called instead of throwing an exception.
Or is it the thing about LocalFile needing to have reloadContent() called?
Exactly that.
@gsherwood or @jrfnl — Just wanted to check-in to see if this PR looked suitable now or we needed to do more work until it could be considered?
@kraftbj I think the question which needs to be answered first is whether #2551 is a feature which PHPCS wants to support. That question never got answered and the #2551 ticket has seen little traction, so there doesn't seem to be that much community interest in it either.
This PR is an implementation detail, which isn't interesting to look at, unless the underlying question is first answered with "yes, the feature is a candidate to be added to PHPCS". (and no, I'm not the right person to answer that question)
I like that general idea of #2551, but it's never been requested enough to put time into it and is not something I'd be looking to support in the core at this time. If I was, I'd make that change in version 4 regardless of BC breaks.
If this PR makes it easier to support outside of PHPCS, then I'm happy to consider that for inclusion sooner, but I have not looked into the purpose of these changes yet.
If this PR makes it easier to support outside of PHPCS, then I'm happy to consider that for inclusion sooner, but I have not looked into the purpose of these changes yet.
That's exactly what this does.
PHPCS already uses a Ruleset and Config referenced by the File object rather than using a global Ruleset and Config. I didn't change anything with respect to that.
The main thing this PR does is allow an implementation of PHP_CodeSniffer\Filters\Filter to return a PHP_CodeSniffer\Files\LocalFile object rather than a string filename (or SplFileInfo object) that PHPCS will later wrap in a LocalFile anyway. It also, as a resource management measure, updates LocalFile to load the file content when it's needed instead of on construction. That's all.
All the logic for reading the per-directory configuration and constructing derived Rulesets and Configs can then be in the filter.
We've been using this since December in production in our monorepo, and it has worked really well. The main use for us has been to configure the properties of an i18n-related sniff with different text domains for different projects. We've also found it easier to use than the normal syntax when we need to disable a rule in a subdirectory.
@gsherwood Just wondering if there's any further thoughts on this moving forward, or if there's any other info we can provide in the meanwhile.
