Remove internal from comparator-related classes constructor

[VincentLanglet]

Q	A
Type	improvement
Fixed issues

1. The comparator constructor say

@internal The comparator can be only instantiated by a schema manager.

but when we're writing an own custom schemaManager, extending AbstractSchemaManager, we'll need still need a way with BC to instantiate a Comparator in order to implement/override the createComparator method.

2. When writing our custom Comparator, we should be able to implements compareSchemas or compareTable so we need a way with BC to instantiate a SchemaDiff/TableDiff/ColumnDiff classes (It doesn't forbid those class to be final @morozov)

3. Another reason to allows instantiate a SchemaDiff/TableDiff/ColumnDiff classes is to write tests when implementing our own Platform and we want to unit tests the methods getAlterSchemaSQL or getAlterTableSQL.

[derrabus]

@morozov: Do we want userland code to create custom Comparator implementations? If so, the constructor indeed should not be @internal. WDYT?

[morozov]

Most of the DBAL code that can be extended wasn't designed with maintainability in mind. The only extensible and maintainable API I can think of is the drivers.

I deliberately marked the Comparator constructors as as internal to allow us adding required parameters to the constructor without breaking the API.

Personally, I'd say that as long as extending those classes works, advanced users can do that and accept the risks of breaking changes.

[VincentLanglet]

I deliberately marked the Comparator constructors as as internal to allow us adding required parameters to the constructor without breaking the API.

I don't see the real benefit when AbstractSchemaManager::createComparator is already part of the API.

dbal/src/Schema/AbstractSchemaManager.php

Lines 997 to 1000 in 54d70d8

	public function createComparator(/* ComparatorConfig $config = new ComparatorConfig() */): Comparator
	{
	return new Comparator($this->platform, func_num_args() > 0 ? func_get_arg(0) : new ComparatorConfig());
	}

If you add a required parameter in the Comparator constructor, you'll still need to provide a default value in createComparator definition...

Also after 4 years, it could be considered that the constructor is mature enough to provide BC.
The platform seems to be enough for the implementation and if some extra config is needed the ComparatorConfig can add extra args in his constructor.

[morozov]

In this case, @internal means that this API was not design with extensibility in mind and what I'm being asked for is to annotate as if it was.

Also after 4 years, it could be considered that the constructor is mature enough to provide BC.

The same argument could be reversed. You can rely on the API w/o an explicit BC promise because you need that and it's been stable enough.

[VincentLanglet]

The same argument could be reversed. You can rely on the API w/o an explicit BC promise because you need that and it's been stable enough.

With this "reversed" argument, you could add @internal everywhere...

My main issue is the fact that you cannot promise the BC policy is a important blocker to write/expose an open source library then since I cannot be fully sure it won't break one day with a patch/minor bump (and I won't control such bump when requiring something like DBAL ^4.3

In this case, @internal means that this API was not design with extensibility in mind and what I'm being asked for is to annotate as if it was.

I understand it wasn't design this way at first. But is it a big deal to offer extensibility now ?
If you still don't want, the PR can be closed.

[morozov]

With this "reversed" argument, you could add @internal everywhere...

Honestly, this might be the right thing to do. Alternatively, we could document that public API with @api and consider everything else internal. The fact that a method is public (e.g. in order to be called by another class from the same library) doesn't mean that it's meant to be used externally.

I don't want to declare these internal APIs as public because they are not. It's as simple as that.