Fixed MultiConstraint with MatchAllConstraint

[Toflar]

I'm not sure if this is correct but I'm fairly sure the current behaviour is incorrect :D

If I create a MultiConstraint with any instance of MatchAllConstraint, I currently get a MultiConstraint back which seems strange.

>= 2.5.0 AND * should return either a MatchAllConstraint or a MatchNonConstraint. Same goes for OR. I'm just confused at the moment as to which one has to return what but I'm proposing it anyway :D

[Toflar]

Don't understand phpstan, none of the methods in this test have any return type.

[Seldaek]

I kinda wonder if this is worth fixing from a performance standpoint, it adds a loop on every multi constraint for a case which will most likely almost never happen as it's really nonsense constraints.. And it's not like it was broken before, it just returns a slightly optimized version now.

[Toflar]

I just noticed because I did $constraint instanceof MatchAllConstraint instead of $constraint->matches(new MatchAllConstraint()). Of course the first variant failed which is why I created the PR.

[leofeyer]

This change is causing a Must provide at least two constraints for a MultiConstraint error, because now that there is an unset($constraints[$k]), the array can become empty after the existing \count($constraints) checks.

        if (0 === \count($constraints)) {
            return new MatchAllConstraint();
        }

        if (1 === \count($constraints)) {
             return $constraints[0];
        }

        foreach ($constraints as $k => $constraint) {
            if ($constraint instanceof MatchAllConstraint) {
                if (!$conjunctive) {
                    return new MatchAllConstraint();
                }
                unset($constraints[$k]);
            }
        }

        // Due to the new unset() above, $constraints can have 0 or 1 member(s) here again,
        // so the checks from above might have to be moved here or copied here.

        $optimized = self::optimizeConstraints($constraints, $conjunctive);

[Seldaek]

Ah yes.. https://github.com/composer/semver/commit/84ce71b983a9463e55438e10d2cbbbba9073aac3 should fix this I believe

[leofeyer]

Unfortunately, it does not: PHP Warning: Undefined array key 0 in vendor/composer/semver/src/Constraint/MultiConstraint.php on line 238

return reset($constraints) fixes the issue for me, but I‘m not sure if it is correct.

[Seldaek]

Yeah reset works, but unfortunately in more complex cases this will still blow up in optimizeConstraint, we'd have to call array_values always there to make sure we have a packed array without gaps for the for loop to work.

I think I will just revert this and done. It was already not really worth it to begin with, but it's only getting worse.

[leofeyer]

Thank you. 👍