Skip to content
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

Prevent auto-unlocked path repo packages from also unlocking their transitive deps when -w/-W is used #10157

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 51 additions & 24 deletions src/Composer/DependencyResolver/PoolBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ class PoolBuilder
/** @var array<string, array<PackageInterface>> */
private $skippedLoad = array();

/**
* Keeps a list of dependencies which are locked but were auto-unlocked as they are path repositories
*
* This half-unlocked state means the package itself will update but the UPDATE_LISTED_WITH_TRANSITIVE_DEPS*
* flags will not apply until the package really gets unlocked in some other way than being a path repo
*
* @var array<string, true>
*/
private $pathRepoUnlocked = array();

/**
* Keeps a list of dependencies which are root requirements, and as such
* have already their maximum required range loaded and can not be
Expand Down Expand Up @@ -155,12 +165,25 @@ public function buildPool(array $repositories, Request $request)

foreach ($request->getLockedRepository()->getPackages() as $lockedPackage) {
if (!$this->isUpdateAllowed($lockedPackage)) {
$request->lockPackage($lockedPackage);
$this->skippedLoad[$lockedPackage->getName()][] = $lockedPackage;
// remember which packages we skipped loading remote content for in this partial update
$this->skippedLoad[$lockedPackage->getName()][] = $lockedPackage;
foreach ($lockedPackage->getReplaces() as $link) {
$this->skippedLoad[$link->getTarget()][] = $lockedPackage;
}

// Path repo packages are never loaded from lock, to force them to always remain in sync
// unless symlinking is disabled in which case we probably should rather treat them like
// regular packages. We mark them specially so they can be reloaded fully including update propagation
// if they do get unlocked, but by default they are unlocked without update propagation.
if ($lockedPackage->getDistType() === 'path') {
$transportOptions = $lockedPackage->getTransportOptions();
if (!isset($transportOptions['symlink']) || $transportOptions['symlink'] !== false) {
$this->pathRepoUnlocked[$lockedPackage->getName()] = true;
continue;
}
}

$request->lockPackage($lockedPackage);
}
}
}
Expand All @@ -183,7 +206,7 @@ public function buildPool(array $repositories, Request $request)
|| $package->getRepository() instanceof PlatformRepository
|| StabilityFilter::isPackageAcceptable($this->acceptableStabilities, $this->stabilityFlags, $package->getNames(), $package->getStability())
) {
$this->loadPackage($request, $package, false);
$this->loadPackage($request, $repositories, $package, false);
} else {
$this->unacceptableFixedOrLockedPackages[] = $package;
}
Expand Down Expand Up @@ -333,7 +356,7 @@ private function markPackageNameForLoading(Request $request, $name, ConstraintIn
* @param RepositoryInterface[] $repositories
* @return void
*/
private function loadPackagesMarkedForLoading(Request $request, $repositories)
private function loadPackagesMarkedForLoading(Request $request, array $repositories)
{
foreach ($this->packagesToLoad as $name => $constraint) {
$this->loadedPackages[$name] = $constraint;
Expand All @@ -360,16 +383,17 @@ private function loadPackagesMarkedForLoading(Request $request, $repositories)
}
foreach ($result['packages'] as $package) {
$this->loadedPerRepo[$repoIndex][$package->getName()][$package->getVersion()] = $package;
$this->loadPackage($request, $package);
$this->loadPackage($request, $repositories, $package, !isset($this->pathRepoUnlocked[$package->getName()]));
}
}
}

/**
* @param bool $propagateUpdate
* @param RepositoryInterface[] $repositories
* @return void
*/
private function loadPackage(Request $request, BasePackage $package, $propagateUpdate = true)
private function loadPackage(Request $request, array $repositories, BasePackage $package, $propagateUpdate)
{
$index = $this->indexCounter++;
$this->packages[$index] = $package;
Expand Down Expand Up @@ -424,7 +448,7 @@ private function loadPackage(Request $request, BasePackage $package, $propagateU
$skippedRootRequires = $this->getSkippedRootRequires($request, $require);

if ($request->getUpdateAllowTransitiveRootDependencies() || !$skippedRootRequires) {
$this->unlockPackage($request, $require);
$this->unlockPackage($request, $repositories, $require);
$this->markPackageNameForLoading($request, $require, $linkConstraint);
} else {
foreach ($skippedRootRequires as $rootRequire) {
Expand All @@ -449,7 +473,7 @@ private function loadPackage(Request $request, BasePackage $package, $propagateU
$skippedRootRequires = $this->getSkippedRootRequires($request, $replace);

if ($request->getUpdateAllowTransitiveRootDependencies() || !$skippedRootRequires) {
$this->unlockPackage($request, $replace);
$this->unlockPackage($request, $repositories, $replace);
$this->markPackageNameForLoading($request, $replace, $link->getConstraint());
} else {
foreach ($skippedRootRequires as $rootRequire) {
Expand Down Expand Up @@ -526,16 +550,6 @@ private function getSkippedRootRequires(Request $request, $name)
*/
private function isUpdateAllowed(BasePackage $package)
{
// Path repo packages are never loaded from lock, to force them to always remain in sync
// unless symlinking is disabled in which case we probably should rather treat them like
// regular packages
if ($package->getDistType() === 'path') {
$transportOptions = $package->getTransportOptions();
if (!isset($transportOptions['symlink']) || $transportOptions['symlink'] !== false) {
return true;
}
}

foreach ($this->updateAllowList as $pattern => $void) {
$patternRegexp = BasePackage::packageNameToRegexp($pattern);
if (preg_match($patternRegexp, $package->getName())) {
Expand Down Expand Up @@ -577,18 +591,19 @@ private function warnAboutNonMatchingUpdateAllowList(Request $request)
* Reverts the decision to use a locked package if a partial update with transitive dependencies
* found that this package actually needs to be updated
*
* @param RepositoryInterface[] $repositories
* @param string $name
* @return void
*/
private function unlockPackage(Request $request, $name)
private function unlockPackage(Request $request, array $repositories, $name)
{
foreach ($this->skippedLoad[$name] as $packageOrReplacer) {
// if we unfixed a replaced package name, we also need to unfix the replacer itself
// as long as it was not unfixed yet
if ($packageOrReplacer->getName() !== $name && isset($this->skippedLoad[$packageOrReplacer->getName()])) {
$replacerName = $packageOrReplacer->getName();
if ($request->getUpdateAllowTransitiveRootDependencies() || (!$this->isRootRequire($request, $name) && !$this->isRootRequire($request, $replacerName))) {
$this->unlockPackage($request, $replacerName);
$this->unlockPackage($request, $repositories, $replacerName);

if ($this->isRootRequire($request, $replacerName)) {
$this->markPackageNameForLoading($request, $replacerName, new MatchAllConstraint);
Expand All @@ -604,14 +619,22 @@ private function unlockPackage(Request $request, $name)
}
}

unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name]);
if (isset($this->pathRepoUnlocked[$name])) {
foreach ($this->packages as $index => $package) {
if ($package->getName() === $name) {
$this->removeLoadedPackage($request, $repositories, $package, $index);
}
}
}

unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name], $this->pathRepoUnlocked[$name]);

// remove locked package by this name which was already initialized
foreach ($request->getLockedPackages() as $lockedPackage) {
if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) {
if (false !== $index = array_search($lockedPackage, $this->packages, true)) {
$request->unlockPackage($lockedPackage);
$this->removeLoadedPackage($request, $lockedPackage, $index);
$this->removeLoadedPackage($request, $repositories, $lockedPackage, $index);

// make sure that any requirements for this package by other locked or fixed packages are now
// also loaded, as they were previously ignored because the locked (now unlocked) package already
Expand All @@ -634,15 +657,19 @@ private function unlockPackage(Request $request, $name)
}

/**
* @param RepositoryInterface[] $repositories
* @param int $index
* @return void
*/
private function removeLoadedPackage(Request $request, BasePackage $package, $index)
private function removeLoadedPackage(Request $request, array $repositories, BasePackage $package, $index)
{
$repoIndex = array_search($package->getRepository(), $repositories, true);

unset($this->loadedPerRepo[$repoIndex][$package->getName()][$package->getVersion()]);
unset($this->packages[$index]);
if (isset($this->aliasMap[spl_object_hash($package)])) {
foreach ($this->aliasMap[spl_object_hash($package)] as $aliasIndex => $aliasPackage) {
$request->unlockPackage($aliasPackage);
unset($this->loadedPerRepo[$repoIndex][$aliasPackage->getName()][$aliasPackage->getVersion()]);
unset($this->packages[$aliasIndex]);
}
unset($this->aliasMap[spl_object_hash($package)]);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "mirrored/path-pkg",
"version": "2.0.0",
"require": {
"mirrored/transitive": "2.*",
"mirrored/transitive2": "2.*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
--TEST--
Partially updating with deps a root requirement which depends on packages in a symlinked path repo should load all available versions for the path repo packages' dependencies.

--REQUEST--
{
"require": {
"root/update": "*",
"symlinked/transitive3": "*",
"symlinked/transitive5": "*",
"symlinked/path-pkg-replace": "*"
},
"locked": [
{"name": "root/update", "version": "1.0.1", "require": {"symlinked/path-pkg": ">=1.0.1", "symlinked/replaced-pkg": "*"}},
{"name": "symlinked/transitive", "version": "1.0.0"},
{"name": "symlinked/transitive3", "version": "1.0.0", "replace": {"symlinked/transitive3-replaced": "1.0.0"}},
{
"name": "symlinked/path-pkg",
"version": "1.0.0",
"require": {
"symlinked/transitive": "1.*",
"symlinked/transitive3-replaced": "1.*",
"symlinked/transitive5-replaced": "1.*"
},
"dist": {"type": "path", "url": "./symlinked-path-repo-with-replaced-deps", "reference": "abcd"}, "transport-options": {}
},
{"name": "symlinked/transitive4", "version": "1.0.0"},
{"name": "symlinked/transitive5", "version": "1.0.0", "replace": {"symlinked/transitive5-replaced": "1.0.0"}},
{
"name": "symlinked/path-pkg-replace",
"version": "1.0.0",
"require": {
"symlinked/transitive3-replaced": "1.*",
"symlinked/transitive4": "1.*",
"symlinked/transitive5-replaced": "1.*"
},
"replace": {
"symlinked/replaced-pkg": "1.0.0"
},
"dist": {"type": "path", "url": "./symlinked-path-repo-replacer", "reference": "abcd"}, "transport-options": {}
}
],
"allowList": [
"root/update"
],
"allowTransitiveDeps": true
}

--FIXED--
[
]

--PACKAGE-REPOS--
[
{"type": "path", "url": "./symlinked-path-repo-with-replaced-deps"},
{"type": "path", "url": "./symlinked-path-repo-replacer"},
[
{"name": "root/update", "version": "1.0.4", "require": {"symlinked/path-pkg": ">=1.0.1", "symlinked/replaced-pkg": "*"}},
{"name": "symlinked/transitive", "version": "1.0.0"},
{"name": "symlinked/transitive", "version": "2.0.2"},
{"name": "symlinked/transitive3", "version": "1.0.0", "replace": {"symlinked/transitive3-replaced": "1.0.0"}},
{"name": "symlinked/transitive3", "version": "1.0.3", "replace": {"symlinked/transitive3-replaced": "1.0.3"}},
{"name": "symlinked/transitive3", "version": "2.0.4", "replace": {"symlinked/transitive3-replaced": "2.0.4"}},
{"name": "symlinked/transitive4", "version": "1.0.0"},
{"name": "symlinked/transitive4", "version": "2.0.2"},
{"name": "symlinked/transitive5", "version": "1.0.0", "replace": {"symlinked/transitive5-replaced": "1.0.0"}},
{"name": "symlinked/transitive5", "version": "1.0.3", "replace": {"symlinked/transitive5-replaced": "1.0.3"}},
{"name": "symlinked/transitive5", "version": "2.0.4", "replace": {"symlinked/transitive5-replaced": "2.0.4"}}
]
]

--EXPECT--
[
"root/update-1.0.4.0",
"symlinked/path-pkg-2.0.0.0",
"symlinked/path-pkg-replace-2.0.0.0",
"symlinked/transitive-2.0.2.0",
"symlinked/transitive3-1.0.0.0",
"symlinked/transitive3-1.0.3.0",
"symlinked/transitive3-2.0.4.0",
naderman marked this conversation as resolved.
Show resolved Hide resolved
"symlinked/transitive4-2.0.2.0",
"symlinked/transitive5-1.0.0.0",
"symlinked/transitive5-1.0.3.0",
"symlinked/transitive5-2.0.4.0"
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
--TEST--
Partially updating one root requirement with transitive deps fully updates transitive deps, and always updates symlinked path repos, but not the transitive deps of the path repos.

--REQUEST--
{
"require": {
"root/update": "*",
"symlinked/path-pkg": "*",
"mirrored/path-pkg": "*"
},
"locked": [
{"name": "root/update", "version": "1.0.1", "require": {"symlinked/transitive2": ">=1.0.1", "mirrored/transitive2": ">=1.0.1"}},
{"name": "symlinked/transitive", "version": "1.0.0"},
{"name": "symlinked/transitive2", "version": "1.0.0"},
{"name": "mirrored/transitive", "version": "1.0.0"},
{"name": "mirrored/transitive2", "version": "1.0.0"},
{
"name": "symlinked/path-pkg",
"version": "1.0.0",
"require": {
"symlinked/transitive": "1.*",
"symlinked/transitive2": "1.*"
},
"dist": {"type": "path", "url": "./symlinked-path-repo", "reference": "abcd"}, "transport-options": {}
},
{
"name": "mirrored/path-pkg",
"version": "1.0.0",
"require": {
"mirrored/transitive": "1.*",
"mirrored/transitive2": "1.*"
},
"dist": {"type": "path", "url": "./mirrored-path-repo", "reference": "abcd"}, "transport-options": {"symlink": false}
}
],
"allowList": [
"root/update"
],
"allowTransitiveDeps": true
}

--FIXED--
[
]

--PACKAGE-REPOS--
[
{"type": "path", "url": "./symlinked-path-repo"},
{"type": "path", "url": "./mirrored-path-repo", "options": {"symlink": false}},
[
{"name": "root/update", "version": "1.0.4", "require": {"symlinked/transitive2": ">=1.0.1", "mirrored/transitive2": ">=1.0.1"}},
{"name": "symlinked/transitive", "version": "1.0.0"},
{"name": "symlinked/transitive", "version": "1.0.1"},
{"name": "symlinked/transitive", "version": "2.0.2"},
{"name": "symlinked/transitive2", "version": "1.0.0"},
{"name": "symlinked/transitive2", "version": "1.0.3"},
{"name": "symlinked/transitive2", "version": "2.0.4"},
{"name": "mirrored/transitive", "version": "1.0.0"},
{"name": "mirrored/transitive", "version": "1.0.5"},
{"name": "mirrored/transitive", "version": "2.0.6"},
{"name": "mirrored/transitive2", "version": "1.0.0"},
{"name": "mirrored/transitive2", "version": "1.0.7"},
{"name": "mirrored/transitive2", "version": "2.0.8"}
]
]

--EXPECT--
[
"symlinked/transitive-1.0.0.0 (locked)",
"mirrored/transitive-1.0.0.0 (locked)",
"mirrored/path-pkg-1.0.0.0 (locked)",
"symlinked/path-pkg-2.0.0.0",
"root/update-1.0.4.0",
"symlinked/transitive2-1.0.3.0",
"symlinked/transitive2-2.0.4.0",
"mirrored/transitive2-1.0.0.0",
"mirrored/transitive2-1.0.7.0",
"mirrored/transitive2-2.0.8.0"
]

--EXPECT-OPTIMIZED--
[
"symlinked/transitive-1.0.0.0 (locked)",
"mirrored/transitive-1.0.0.0 (locked)",
"mirrored/path-pkg-1.0.0.0 (locked)",
"symlinked/path-pkg-2.0.0.0",
"root/update-1.0.4.0",
"symlinked/transitive2-2.0.4.0",
"mirrored/transitive2-1.0.7.0",
"mirrored/transitive2-2.0.8.0"
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "symlinked/path-pkg-replace",
"version": "2.0.0",
"require": {
"symlinked/transitive3-replaced": ">=1.0.3",
"symlinked/transitive4": "2.*",
"symlinked/transitive5-replaced": "2.*"
},
"replace": {
"symlinked/replaced-pkg": "2.0.0"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "symlinked/path-pkg",
"version": "2.0.0",
"require": {
"symlinked/transitive": "2.*",
"symlinked/transitive3-replaced": "2.*",
"symlinked/transitive5-replaced": ">=1.0.3"
}
}