EZP-27743: Rename Criterion abstract class to Matcher

[jacek-foremski]

Question	Answer
JIRA issue	EZP-27743
Bug/Improvement	Improvement
New feature	no
Target version	master
BC breaks	yes
Tests pass	yes
Doc needed	no

This is the first in series of PRs which goal is to clean up the code related to Criterions and introduce proper sub-interfaces for Criterion Interface (one for logical operators and another one for attributes or matchers, as I propose to call them).
Quote from the JIRA ticket:

This task aims at making interfaces related to Criterions more clear. The main reason for this task was the fact that currently LogicalOperators do not implement CriterionInterface, but extend Criterion abstract class instead. This makes CriterionInterface not used almost at all, and it leads to CriterionInterface|LogicalOperator return type hint combo when it would make sense to have only CriterionInterface instead (see #2035).
After this task is complete, there will exist two abstract classes implementing the CriterionInterface - one for the LogicalOperators, and one for the criterions that match the content based on some conditions (called Matchers). This would make it clear which abstract class should the Criterion extend and it also would make it easy to typehint (CriterionInterface used for the cases when either LogicalOperators or Matchers can be expected).

This PR takes care of the following step of this task:

Criterion abstract class renamed to Matcher. The name Criterion (as in CriterionInterface) is better associated with all of the criterions including LogicalOperator. Therefore the new name for the criterions used to match content based on some conditions was necessary. I used the name Matcher, so when moved to Criterion\Matcher namespace it (in my opinion) most clearly represents what it is used for.

After this PR is merged, I will create another one to change classes from kernel so they will use the new name instead of the deprecated class.
The full list of step that I plan to perform can be found in the related JIRA issue (EZP-27743).

Original PR: #2078 (requested to be split into smaller parts).

TODO:

• Implement feature / fix a bug.
• Implement tests (skipped).
• Fix new code according to Coding Standards ($ composer fix-cs).
• Ask for Code Review.

[andrerom]

Side: We might want to backport this change to btw, so bundles can be adapted to work across 6.x/7.x/8.x (6.x will still be supported once 8.0 comes out).

Wdyt @alongosz?

[MarioBlazek]

Are you sure that Matcher is adequate name ?

[jacek-foremski]

Java uses Attribute name for something similar (although not exactly the same): https://docs.oracle.com/html/E10684_08/oracle/adf/view/rich/model/AttributeCriterion.html. I think that it is also problematic.
I chose Matcher since when it is put inside Criterion\Matcher namespace, then, in my opinion, it clearly states what it does (it matches based on some conditions).
That being said, I am aware that the name is not perfect, but it is the best I came up with. So all suggestions are welcome.

[andrerom]

@MarioBlazek Did you managed to hear from @pspanja if he had any thought on this? If he remembers past naming we have referred to this as?

[pspanja]

It is not a "matcher" since it's not matching anything. Instead, this is about defining rules for matching, so I find the name "criterion" to be fitting. Matching is really job of the search backend.

Logical operator is just a special type of rule that works by combining/modifying other rules and that is already clear.

As for the CriterionInterface, it actually looks like a leftover. It's ignored in both API and SPI, for example:

• https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Values/Content/Query.php#L49
• https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/SearchService.php#L151
• https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/SPI/Search/Handler.php#L45

So if going for a BC break, I would rather remove the interface. It will probably be a break in name only.

[jacek-foremski]

I don't think that staying with only an abstract class Criterion, which is extended by LogicalOperator, is a good idea. This has a multiple bad smells which I'm trying to fix by making it implementing a CriterionInterface instead:

• LogicalOperator doesn't use the contructor from the Criterion class, it overwrites it completely: https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Values/Content/Query/Criterion/LogicalOperator.php#L34
• As a result, LogicalOperator doesn't use the getValueTypeCheckCallback private function from Criterion class: https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Values/Content/Query/Criterion.php#L123
• LogicalOperator inherits createFromQueryBuilder function from Criterion class, which looks plain wrong in LogicalOperator's scope (it passes three arguments to the constructor, when LogicalOperator has only one and it is different one): https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Values/Content/Query/Criterion.php#L149
• LogicalOperator declares its own attribute and I don't think the attributes from Criterion class make much sense in LogicalOperator's scope: https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Values/Content/Query/Criterion/LogicalOperator.php#L25.

To fix the above, I wanted to use CriterionInterface, which both LogicalOperator and Criterion(Matcher) abstract classes would implement.
We can rename Criterion to Attribute instead of Matcher (or something else completely), or rename the CriterionInterface, but I don't think that removal of CriterionInterface completely would fix the current problems.

On a side note, we say that the Criterions "match" the Content in the comments (for example https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/API/Repository/Values/Content/Query/Criterion/LanguageCode.php#L17), so that's where I got the idea. But I agree, it's not the best of names. I probably lean towards Attribute, but I would love to hear some opinions on that.

[pspanja]

Then the problem being fixed here is LogicalOperator not using implementation from the base class?

[emodric]

In any case, how would renaming Criterion to Matcher help anyone? It is just creating a BC break for the sake of breaking BC.

Developers are already using Criterion ALL over their code (meaning projects, 3rd party libs and bundles and so on), and no one is complaining about the name, so switching to the new name will suddenly make most of 3rd party code obsolete.

[alongosz]

Side: We might want to backport this change to btw, so bundles can be adapted to work across
6.x/7.x/8.x (6.x will still be supported once 8.0 comes out).
Wdyt @alongosz?

@andrerom New classes in favor of the old ones can be backported as forward compatibility, but we cannot deprecate in 6.x (unless we plan to release 6.14 which we do not :) )

[alongosz]

Overall, while I understand the point of changing name to avoid confusion where it comes to LogicalOperator not extending Criterion but implementing CriterionInterface only, in this case I don't think it's the best course of action. Criterion is a very good name here.

As discussed internally with @natanael89 I'd rather:

• remove extends Criterion from LogicalOperator and replace it with implements CriterionInterface and implement explicitly required contract,
• keep extends Criterion on other Criterions as the name and implementations on abstract Criterion still fit,
• document that LogicalOperator is a special kind of Criterion that does not rely on abstract Criterion but implements specific contract.

[jacek-foremski]

As @alongosz said, we decided to leave the current naming and focus on fixing the architecture. Therefore this PR will be closed. I will open another one with other fixes soon.