Filters/ExactMatch: deprecate the getBlacklist() and getWhitelist() methods

[jrfnl]

Description

Deprecate the getBlacklist() and getWhitelist() methods and introduce the getBlockedFiles() ++getDisallowedFiles()++ and getAllowedFiles() replacement methods.

When available in child classes, the getBlockedFiles() ++getDisallowedFiles()++ and getAllowedFiles() methods will take precedence over the deprecated getBlacklist() and getWhitelist() methods.

Suggested changelog entry

• Issue 198: The abstract ExactMatch::getBlacklist() and ExactMatch::getWhitelist() methods are deprecated and will be removed in the 4.0 release.
  • In version 4.0, these methods will be replaced with abstract ExactMatch::getDisallowedFiles() and ExactMatch::getAllowedFiles() methods
  • To make Filters extending ExactMatch cross-version compatible with both PHP_CodeSniffer 3.9.0+ as well as 4.0+, implement the new getDisallowedFiles() and getAllowedFiles() methods.
    • When both the getDisallowedFiles() and getAllowedFiles() methods as well as the getBlacklist() and getWhitelist() are available, the new methods will take precedence over the old methods.

Related issues/external references

Fixes #198
Covered by the tests which were added in #201

[derrabus]

Can we replace the method with a non- abstract implementation, so downstream code won't need to implement a deprecated method?

[jrfnl]

@derrabus Thanks for the feedback.

I was struggling a little with whether to do that or not, as that would mean we can no longer enforce that at least one of each of getBlacklist/getBlockedFiles and getWhitelist/getAllowedFiles is required to be implemented and making the new methods abstract would introduce a breaking change in a minor.

Only way I can think of still safeguarding it, would be to remove all four method declarations from the ExactMatch class and using method_exists() checks and throwing an exception is any of these sets don't have a corresponding method, but removing the method declarations also makes it much less obvious what the class expects and would require significantly more documentation to be added to the class.

Not sure it's worth the overhead for the (hopefully) short period until the 4.0 release, especially considering that based on a simple code search, there are no external classes extending the ExactMatch class.

What do you think ?

[derrabus]

The abstract class could contain implementations of both methods calling each other, with a recursion detection. If a recursion is detected, you throw because in that case the downstream class hasn't implemented either of them.

[jrfnl]

@derrabus Hmm.. feels a bit like overengineering for something which isn't a real life issue anyway - see the impact analysis (same link I posted in the underlying issue)

The PHP native implements Serializable vs __serialize() conundrum also comes to mind.

Based on the code in this PR, the following situations are possible for custom Filter classes which extend ExactMatch:

1. Do nothing, i.e. only implements the old abstract methods.
   Impact:
   • Will continue to work fine without deprecation notices in combination with any PHPCS 3.x release.
   • Will break in combination with PHPCS 4.0 or higher.
2. Implement both the old and the new methods in whichever way they prefer (one pointing to the other or visa versa).
   Impact:
   • Will work cross-version with any PHPCS 3.x or 4.x release without notices.
   • Once support for PHPCS 3.x is dropped, the implementation of the "old" methods can be removed.
3. Implement only the "new" methods.
   Impact:
   • Will not work in combination with PHPCS 3.x.
   • Well, that would be one package which is ready early for PHPCS 4.x ;-)

[jrfnl]

There has been some discussion about the new function names in #198. Based on that discussion, the getBlockedFiles() method name has been replaced with getDisallowedFiles().

[stronk7]

I like current patch as is, basically.

• It will make things to be as they are now (abstract) in 4.x with the new methods.
• It's documented (in phpdocs, in release notes - it will, ....).
• it's easy to support both 3.x and 4.x if needed to.
• impact seems to be small, not requiring any sort of extra check or complexity.

So, my +0.5 :-)

[jrfnl]

If there are no other comments/feedback to this PR, I will merge it tomorrow.

[jrfnl]

Rebased without changes to be sure. Merging once the build passes.