-
-
Notifications
You must be signed in to change notification settings - Fork 444
[Composer] Target lowest declared version for libraries in composer-based sets #8359
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,7 @@ public function resolvePackageVersion(string $packageName): ?string | |
| private function createInstalledPackages(array $packages): array | ||
| { | ||
| $packageConstraints = $this->resolvePackageConstraints(); | ||
| $isLibrary = $this->isLibrary(); | ||
| $installedPackages = []; | ||
|
|
||
| foreach ($packages as $package) { | ||
|
|
@@ -116,9 +117,15 @@ private function createInstalledPackages(array $packages): array | |
|
|
||
| $constraint = $packageConstraints[$name] ?? null; | ||
| if (is_string($constraint)) { | ||
| // the "installed.json" can be outdated, e.g. after a branch switch; | ||
| // in such case the "composer.json" constraint has a priority | ||
| $version = $this->matchConstraintVersion($version, $constraint) ?? $version; | ||
| if ($isLibrary) { | ||
| // a library must stay compatible with the lowest version it declares, | ||
| // regardless of which one happens to be installed locally | ||
| $version = $this->resolveConstraintLowestVersion($constraint) ?? $version; | ||
| } else { | ||
| // the "installed.json" can be outdated, e.g. after a branch switch; | ||
| // in such case the "composer.json" constraint has a priority | ||
| $version = $this->matchConstraintVersion($version, $constraint) ?? $version; | ||
| } | ||
| } | ||
|
|
||
| $installedPackages[$name] = new InstalledPackage($name, $version); | ||
|
|
@@ -127,6 +134,17 @@ private function createInstalledPackages(array $packages): array | |
| return $installedPackages; | ||
| } | ||
|
|
||
| /** | ||
| * A library declares a compatibility range in its "composer.json"; the version-specific rules must target the | ||
| * lowest declared version, not the one that happens to be installed locally. | ||
| */ | ||
| private function isLibrary(): bool | ||
| { | ||
| $projectComposerJson = $this->loadProjectComposerJson(); | ||
|
|
||
| return ($projectComposerJson['type'] ?? null) === 'library'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is wrong. It should be inverted to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "type" may be missing, most likely in projects, so we have to go with a default here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should consider something a project if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or in other words: the current solution does not fix the original issue. I have a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get your point, but if we do that, all projects will be upgraded only when someone fill the keyword explicitly in If a keyword is an issue, then other non-project keywords should be included as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I'm convinced there should be no check at all and the composer.json should always win. I always want to have to rules applied to "what is compatible", never ever to "what is installed".
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's exactly the point. If they specify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @Toflar here. |
||
| } | ||
|
|
||
| /** | ||
| * There is no vendor to read the installed versions from, so the constraints themselves are the only source | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "type": "library", | ||
| "require": { | ||
| "phpunit/phpunit": "^10.5 || ^11.0 || ^12.0", | ||
| "symfony/console": "^7.0" | ||
| }, | ||
| "require-dev": { | ||
| "nette/utils": "^3.2" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "packages": [ | ||
| { | ||
| "name": "phpunit/phpunit", | ||
| "version": "12.1.0", | ||
| "version_normalized": "12.1.0.0" | ||
| }, | ||
| { | ||
| "name": "symfony/console", | ||
| "version": "v7.2.0", | ||
| "version_normalized": "7.2.0.0" | ||
| }, | ||
| { | ||
| "name": "nette/utils", | ||
| "version": "v3.2.0", | ||
| "version_normalized": "3.2.0.0" | ||
| }, | ||
| { | ||
| "name": "webmozart/assert", | ||
| "version": "1.11.0", | ||
| "version_normalized": "1.11.0.0" | ||
| } | ||
| ] | ||
| } |
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 think library check is not needed, even on project, eg on "framework skeleton", phpunit range may exists to give user ability to use phpunit version based on specific php version.
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.
The goal is to give projects real latest versions (those in
installed.json),and to give packagest the lowest safest ones. As
^7.3can be 7.3.0 on a library, but 7.3.50 on final real project