New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix PoolOptimizer should consider disjunctive MultiConstraints #10579
Merged
Seldaek
merged 8 commits into
composer:2.2
from
Toflar:fix/pool-optimizer-too-aggressive
Mar 12, 2022
Merged
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
b23177b
Failing test case
Toflar fa0eae9
Quickfix, needs more investigation
Toflar 073e115
Only disjunctive multi constraints are of interest
Toflar 160e1f4
More test coverage
Toflar 8263c96
CS
Toflar abbe9c9
CS
Toflar 9169458
Added a note and removed useless test case
Toflar 51c22aa
Simplify code
Seldaek File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
55 changes: 55 additions & 0 deletions
55
tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/complex-prefer-lowest.test
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
--TEST-- | ||
Test keeps package "package/b" in version 2.2.0 because for prefer-lowest either one might be relevant | ||
|
||
--REQUEST-- | ||
{ | ||
"require": { | ||
"package/a": "^1.0" | ||
}, | ||
"preferLowest": true | ||
} | ||
|
||
|
||
--POOL-BEFORE-- | ||
[ | ||
{ | ||
"name": "package/a", | ||
"version": "1.0.0", | ||
"require": { | ||
"package/b": "^1.0 || ^2.2" | ||
} | ||
}, | ||
{ | ||
"name": "package/b", | ||
"version": "1.0.0" | ||
}, | ||
{ | ||
"name": "package/b", | ||
"version": "1.0.1" | ||
}, | ||
{ | ||
"name": "package/b", | ||
"version": "2.2.0" | ||
} | ||
] | ||
|
||
|
||
--POOL-AFTER-- | ||
[ | ||
{ | ||
"name": "package/a", | ||
"version": "1.0.0", | ||
"require": { | ||
"package/b": "^1.0" | ||
} | ||
}, | ||
{ | ||
"name": "package/b", | ||
"version": "1.0.0" | ||
}, | ||
{ | ||
"name": "package/b", | ||
"version": "2.2.0" | ||
} | ||
] | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to work on another testcase then. I want to have this covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.