Are changes to the constructor really BC breaks?

[ondrejmirtes]

Hi, I've got these messages:

[BC] CHANGED: Default parameter value for parameter $nextAutoIndex of PHPStan\Type\Constant\ConstantArrayType#__construct() changed from 0 to array (
  0 => 0,
)
[BC] CHANGED: The parameter $nextAutoIndex of PHPStan\Type\Constant\ConstantArrayType#__construct() changed from int to a non-contravariant int|array
[BC] CHANGED: The parameter $nextAutoIndex of PHPStan\Type\Constant\ConstantArrayType#__construct() changed from int to int|array

This is how the PHP diff looks like:

 	/**
 	 * @api
 	 * @param array<int, ConstantIntegerType|ConstantStringType> $keyTypes
 	 * @param array<int, Type> $valueTypes
+	 * @param non-empty-list<int>|int $nextAutoIndexes
 	 * @param int[] $optionalKeys
 	 */
 	public function __construct(
 		private array $keyTypes,
 		private array $valueTypes,
-		private int $nextAutoIndex = 0,
+		int|array $nextAutoIndexes = [0],
 		private array $optionalKeys = [],
 	)
 	{
 		assert(count($keyTypes) === count($valueTypes));
 
+		if (is_int($nextAutoIndexes)) {
+			$nextAutoIndexes = [$nextAutoIndexes];
+		}
+
+		$this->nextAutoIndexes = $nextAutoIndexes;
+

I'm widening the type of the parameter, and if the argument is omitted from the call, or if the old type is passed to the parameter, the result is the same.

I don't see how this is a BC break, but I'm open to be corrected :) Thanks.

[Ocramius]

Probably need an exclusion for constructors, since in PHP they can be overridden

[ondrejmirtes]

Only abstract ones and those coming from interfaces can't :)

[Ocramius]

Actually, default value changes are most likely still a BC break, can't remember context though 🤔

As for type, it seems like a bug to me

[Ocramius]

Ah, brain fart: of course child classes cannot reduce the type of a parameter, so that's what is tripping this analysis.

We really just need to exclude constructors, like we do exclude methods for final classes, because the BC break is valid for inheritance, just not for __construct()

[zluiten]

Since the introduction of named arguments I can understand why the example in the top description can be a BC break.

But what is not a BC break is something like this. Or am I being stupid?

- final class GapDetection
+ final class GapDetection implements GapDetector
{
}

final class ConsumingClass
{
    public function __construct(
-        private GapDection $gapDection,
+        private GapDector $gapDection,
    ) {
    }
}

See f.e. prooph/pdo-event-store#260

[Ocramius]

@zluiten it shouldn't be a BC break, but I can see how the analysis may choke on it, if it can't find the inheritance in the before/after checks.

Can you try and expand this test with your example, please?

• https://github.com/Roave/BackwardCompatibilityCheck/blob/4eef6eaa785049bbcac0f29e80454ce5702d3d83/test/unit/DetectChanges/BCBreak/FunctionBased/ParameterTypeContravarianceChangedTest.php
• https://github.com/Roave/BackwardCompatibilityCheck/blob/4eef6eaa785049bbcac0f29e80454ce5702d3d83/src/DetectChanges/BCBreak/FunctionBased/ParameterTypeContravarianceChanged.php

Your error indicates:

Error: The parameter $gapDetection of Prooph\EventStore\Pdo\Projection\PdoEventStoreProjector#__construct() changed from Prooph\EventStore\Pdo\Projection\GapDetection|null to a non-contravariant Prooph\EventStore\Pdo\Projection\GapDetector|null

We need to get better at adding BC check identifiers, but I think your reported BC issue comes from this specific check.

[Ocramius]

I think this check specifically:

BackwardCompatibilityCheck/src/DetectChanges/BCBreak/FunctionBased/ParameterTypeContravarianceChanged.php

Lines 48 to 51 in 4eef6ea

	$fromType = $fromParameter->getType();
	$toType = $toParameter->getType();
	if (($this->typeIsContravariant)($fromType, $toType)) {

Is comparing the before/after classes with different inheritance in place.

It's probably something that TypeIsContravariant and TypeIsCovariant need to be expanded to handle