Deprecate modified foreign keys in TableDiff

[morozov]

Constraints cannot be modified neither in the DBAL Schema API, nor in databases – they can be dropped and added. However, Comparator and TableDiff represent non-equal foreign key constraints with the same name as "modified".

This requires the consumer of the diff to implement additional processing of the "modified foreign keys" alongside the dropped and added ones:

dbal/src/Platforms/AbstractPlatform.php

Lines 1310 to 1312 in b2ac865

	foreach ($diff->getModifiedForeignKeys() as $foreignKey) {
	$sql[] = $this->getDropForeignKeySQL($foreignKey->getQuotedName($this), $tableNameSQL);
	}

dbal/src/Platforms/AbstractPlatform.php

Lines 1336 to 1338 in b2ac865

	foreach ($diff->getModifiedForeignKeys() as $foreignKey) {
	$sql[] = $this->getCreateForeignKeySQL($foreignKey, $tableNameSQL);
	}

dbal/src/Platforms/SQLitePlatform.php

Line 923 in b2ac865

foreach (array_merge($diff->getModifiedForeignKeys(), $diff->getAddedForeignKeys()) as $constraint) {

The changes

As part of the deprecation, the Comparator will stop passing $modifiedForeignKeys to the TableDiff constructor and instead will start including the dropped and added constraint to $droppedForeignKeys and $addedForeignKeys respectively. I don't think that this is a breaking change assuming that the consumer of the diff processes it in full (not just the modified keys).

I coulnd't find any Doctrine (other than DBAL) and other open-source code that would use TableDiff#getModifiedForeignKeys() at all.

If we believe the shape of the diff should remain intact, then instead of deprecating the method, we need to introduce a comparator configuration parameter for producing the diff the new way and then deprecating it. This would be overkill.

[greg0ire]

That's a convoluted way to check for 0, or am I missing something?

[morozov]

Yes, it checks if the array is empty. In general, the value under count() could be any Countable, and its count() could be negative, so checking for < 1 feels safer. But if it feels odd, we can agree on some other standard.

Personally for me, === 0 and !== 0 looks too strict (both from the standpoints of requiring a strict type match and a strict value match). The value is guaranteed to be of type int but isn't guaranteed to be non-negative.

This is all purely aesthetic (or it's "defensive programming"?), so I'm fine with using whatever others are good with (except for === [] and !== []).

[greg0ire]

Why would you want to silently return for negative values? Semantically, I'd lean towards === [] , we want to detect if the value differs from the default value (regardless of what it is), right? I mean, we want to deprecate this parameter, don't we?

[morozov]

Semantically, I'd lean towards === [] ,

This is a PHP-ism. See #6621 (comment) for more details. Let's stick with count($array) === 0 for "is array empty?" and count($array) !== 0 for "is array not empty?".

[greg0ire]

Works for me

[greg0ire]

Should this property be deprecated, BTW?

[morozov]

Is there a way to deprecate a promoted property? In #6728, in order to deprecate ForeignKeyConstraint#$options, I had to replace a promoted constructor parameter with a parameter and property.

But in this case, the property is private, nobody can use it outside of the class anyways.

[greg0ire]

Oh right, forget it then. And to answer your question, I thought it was possible to annotate them, but can't check right now.