CLI: make it easier to exclude specific checks

[swissspidy]

Right now it's only possible to pass an allowlist, not a denylist

[swissspidy]

Related example: WooCommerce would want to disable the trademark check, because they are allowed to use their own trademark.

Another plugin might want to disable the i18n_usage or late_escaping checks because they already use PHPCS for that and would be otherwise torpedoed by a massive amount of duplicate errors.

[swissspidy]

Not sure how to best implement this, doesn't seem to be as straightforward as I thought it would be.

@mukeshpanchal27 @felixarntz any thoughts?

[felixarntz]

@swissspidy It would certainly be possible, but we would need to be careful to follow the same pattern as other parameters so that it is centrally handled and works with UI and WP-CLI.

I think the foundation would need to be in Abstract_Check_Runner. There's a get_check_slugs() method with corresponding get_check_slugs_param() and set_check_slugs(). We would need to implement the same set of methods for the denylist, e.g. *_check_ignore_slugs*().

We could then retrieve this denylist via get_check_ignore_slugs() in Abstract_Check_Runner::get_checks_to_run(), and then we'd have two options:

• Either pass it as a new parameter in the Check_Repository::get_checks() call. This would technically be a breaking change as it breaks the Check_Repository interface, but if we think that's the best implementation, it would probably be fine, since I'm sure nobody has written their own Check_Repository yet at this point.
• Or we simply use the list after the Check_Repository::get_checks() call to remove those checks from the returned map. This would be completely trivial from a BC perspective, but is arguably less reliable as it wouldn't be implemented in the Check_Repository which is the foundation for retrieving checks.

[felixarntz]

@swissspidy I actually just started looking into a related improvement. If you give me 1 hour or so, I'll open a PR with a related simplification that will improve this.

[felixarntz]

@swissspidy The PR I implemented in #315 should make implementing this issue a bit more straightforward while improving the API for better separation of concerns.

My last "either or" consideration in #308 (comment) could then become a new Check_Collection::exclude( array $check_slugs ) method, simple yet centralized. LMKWYT.

[swissspidy]

Now that #315 is merged...

One issue that remains is that Check_Collection holds Check instances, but a Check instance doesn't have a slug. So such an exclusion is still not easily possible. The only place where slugs are known is in Abstract_Check_Runner::register_checks(). We should probably make the slug a class constant of Check and add a get_slug() method.

Even with that in place, implementing such an exclusion will be a bit cumbersome. From what I can see, I would need to add a new set_excluded_check_slugs() method to Abstract_Check_Runner, so that in Abstract_Check_Runner::get_checks_to_run() I could call Check_Collection::Collection::filter()

Between Plugin_Check_Command and Check_Collection there still is Abstract_Check_Runner which will need some adjustments for this. From what I can see, we could:

---

Aside: Check_Categories::filter_checks_by_categories() could now operate on a Check_Collection too

[felixarntz]

@swissspidy The entries in Check_Collection are actually keyed by slug. The include() method already uses that, so you could take the same approach for a new exclude() method.

I think the approach I outlined in the beginning of #308 (comment) would be most straightforward. While it's a bit cumbersome to have to introduce 3 methods, they are very simple to implement, and that approach is currently needed for all the things to be wired correctly and properly support both AJAX and CLI in its possible configurations.

[swissspidy]

The entries in Check_Collection are actually keyed by slug. The include() method already uses that, so you could take the same approach for a new exclude() method.

Ah, I see! Then IMHO a new exclude() method is overkill, I can just call filter() as long as we're passing ARRAY_FILTER_USE_BOTH to array_filter

[felixarntz]

I don't think it's overkill. We already have include(), and exclude() fits perfectly with it. It's a much simpler API than having to provide a custom filter callback (plus we don't currently use ARRAY_FILTER_USE_BOTH).