Use less verbose Comparator::compareSchema()

[trompette]

Q	A
Type	improvement
BC Break	no
Fixed issues	none

Summary

While working on #4698, I noticed a small inconsistency when comparing schemas with Comparator.

This PR uses an existing shortcut everywhere instead of a verbose alternative.

[morozov]

This is the only static Comparator method, so I'd rather replace it with a non-static one for consistency with the rest of the API and deprecated the compare() method since its purpose is unclear from the name.

Please also see #4659 (comment).

[trompette]

Looking at the code and how it is used, it is quite odd the other static diffing methods are not present. Specially given the Comparator object does not have any state.

As mentionned in #4659 (comment), if the goal is to rename all methods anyways, maybe we could introduce a new Differ class with the following public API:

class Differ
{
    public static function compareSchemas(Schema $s1, Schema $s2): SchemaDiff;
    public static function compareSequence(Sequence $s1, Sequence $s2): bool;
    public static function compareTable(Table $t1, Table $t2): TableDiff|false;
    public static function compareForeignKey(ForeignKeyConstraint $k1, ForeignKeyConstraint $k2): bool;
    public static function compareColumn(Column $c1, Column $c2): string[]; // could also be ColumnDiff
    public static function compareIndex(Index $i1, Index $i2): bool;
}

(Now that I see the API, maybe the methods returning a boolean should be renamed for clarity or moved to a new class.)

What do you think ?

[morozov]

"Differ" is a bit odd name, I'd prefer the same class name: the comparator compares things and returns a diff – looks like a solid naming foundation to me. As for the specific method names, I think they should contain the plural form of the object (e.g. compareColumns()).

IIRC, we'll need to only migrate compareSchemas() from being static to non-static which doesn't require introduction of a new class. We can deprecate non-static calls to the method (which PHP allows) and make it non-static in the next major release.

Oh, and I don't think static methods is a good idea. This way, it will be impossible to evolve the API further, e.g. split it into multiple classes, define interfaces, pass it as a dependency, etc.

[trompette]

"Differ" sounded great from my suburb of Paris, France... 😉

I did not think about evolving the Comparator API further, you are quite right!

[morozov]

"Differ" sounded great from my suburb of Paris, France... 😉

I think that would be perfect in the Java world where they smash an -er on top of any noun :-)

[trompette]

Let's close this PR. I will open another one to make Comparator API more consistent.