Fix PoolOptimizer should consider disjunctive MultiConstraints #10579
Conversation
Toflar
changed the title
|
|
||
| if ($constraint instanceof MultiConstraint && $constraint->isDisjunctive()) { | ||
| foreach ($constraint->getConstraints() as $sub) { | ||
| $expanded = array_merge($expanded, $this->expandDisjunctiveMultiConstraints($sub)); |
There was a problem hiding this comment.
Is the recursion here needed vs simply $expanded[] = $sub;? I don't think we ever create nested disjunctive constraints but maybe I'm overlooking something.
There was a problem hiding this comment.
That's one of my questions ;) I don't know if it's even possible to define something like (^2.0 || ^3.0) || (^4.1 || ^2.1)?
There was a problem hiding this comment.
It technically is possible I think, although very uncommon. But we do currently allow multiconstraints to be arbitrarily nested.
There was a problem hiding this comment.
I'll try to work on another testcase then. I want to have this covered.
There was a problem hiding this comment.
Brackets are not allowed in version constraints so I don't know how I could create this. I also tried in an actual project and there was no case 🤔
There was a problem hiding this comment.
It already is. I just left a comment there to maybe give our future selves an idea where to start debugging if something like this happens. Might want to remove the TODO or rephrase but I think you have to decide how you'd like to have that note :)
There was a problem hiding this comment.
Ah indeed, compact constraint will never have nested disjunction in the output. Disjunction is only used on top level to basically list all the intervals.
There was a problem hiding this comment.
With that knowledge in mind, we can also drop the hint (and the time-consuming test if you want 😢) 😉 Just let me know what to do :)
There was a problem hiding this comment.
Sorry had missed that compact was called :( I'd remove it then and maybe just leave a comment that because of the compact that can't exist there?
There was a problem hiding this comment.
No prob, it helped us find out that it's not a use case so that's good :) This PR is now finished then, I guess.
|
Thanks for the fix, it makes sense I think. I suppose the perf regression isn't too bad as this kind of constraints isn't so widespread? |
|
I could not see any regression, no. I actually tested that :) According to blackfire there are other parts that require far more time which is something I want to investigate but that was already the case before this PR. |
|
I cannot comment in implementation, but I tested the fix on example |
|
Failing test again unrelated. |
|
Thanks! |
Fixes #10558
The problem is that we're currently only generating one "dependency constraint group" for disjunctive
MultiConstraintinstances. This means that for^2.14 || ^3.3we only keep one package that matches this constraint. However, this can go bad as e.g.3.3.0goes missing in favor of2.14.0for--prefer-lowestwhich might cause an unresolvable set of dependencies whereas3.3.0would have actually led to a solution.If a
MultiConstraintis disjunctive, we need to create individual groups for every sub constraint.