Conversation
There was a problem hiding this comment.
Pull request overview
Adds safeguards in the custom Composer Installer to avoid destructive file operations when installing/updating/uninstalling packages coming from Composer path repositories, and expands the PHPUnit suite to cover the new behaviors.
Changes:
- Add path-repository detection and guarded install/update/uninstall behavior in
Installer. - Refactor update flow to download the target package directly and delegate only repo-tracking to the parent updater via a skip mechanism.
- Add PHPUnit coverage for path-repo guard behavior and the
updateCode()skip flag.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Installer.php |
Adds isPathRepository(), guarded install/update/uninstall, introduces $skipUpdateCode + delegation for repo tracking, and refactors update to download fresh code directly. |
tests/ModuleInstallerTest.php |
Extends the TestableInstaller shim and adds tests for path-repo skipping and updateCode() skip behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $promise = parent::install($repo, $package); | ||
|
|
||
| return $promise->then(function () use ($package) { | ||
| $this->removeExcludedDirectories($package); | ||
| }); |
There was a problem hiding this comment.
parent::install() is declared to return ?PromiseInterface, but the code unconditionally calls ->then() on its return value. If Composer returns null for synchronous installers, this will throw. Consider handling the null case (performing removeExcludedDirectories() immediately and returning null, or wrapping with React\Promise\resolve($promise) before chaining).
| $this->skipUpdateCode = false; | ||
| }; | ||
|
|
||
| return parent::update($repo, $initial, $target)->then( |
There was a problem hiding this comment.
delegateRepoTracking() assumes parent::update() always returns a promise and immediately calls ->then(). Since Composer’s installer API allows update() to return null, this can fatal and also leave $skipUpdateCode stuck true. Handle the null return (reset the flag and return React\Promise\resolve(null) or similar) so the flag is always cleared.
| return parent::update($repo, $initial, $target)->then( | |
| $promise = parent::update($repo, $initial, $target); | |
| if ($promise === null) { | |
| $resetFlag(); | |
| return \React\Promise\resolve(null); | |
| } | |
| return $promise->then( |
| * | ||
| * {@inheritDoc} | ||
| */ | ||
| public function update(InstalledRepositoryInterface $repo, PackageInterface $initial, PackageInterface $target): ?PromiseInterface | ||
| { | ||
| if ($this->isPathRepository($target)) { | ||
| $this->io->write(" - <info>Skipping update for path repository:</info> {$target->getPrettyName()}"); | ||
|
|
||
| return \React\Promise\resolve(null); | ||
| } |
There was a problem hiding this comment.
PR description says path-repo install/update/uninstall should skip destructive file ops while still updating Composer’s internal tracking state. In update() the path-repo branch returns a resolved promise without calling the repo-tracking delegation logic, so installed.json/repo state may not be updated. Please clarify intended behavior and, if tracking should still occur, delegate to the parent tracking path here too (while keeping code updates skipped).
| return parent::update($repo, $initial, $target)->then( | ||
| $resetFlag, |
There was a problem hiding this comment.
New behavior in delegateRepoTracking() (skip-flag lifecycle + parent update delegation) isn’t covered by tests. Adding tests for (1) flag reset on success, and (2) flag reset when parent::update() rejects/throws would help prevent regressions, especially since leaving the flag enabled would break subsequent updates.
| return parent::update($repo, $initial, $target)->then( | |
| $resetFlag, | |
| try { | |
| $promise = parent::update($repo, $initial, $target); | |
| } catch (\Throwable $e) { | |
| $resetFlag(); | |
| throw $e; | |
| } | |
| return $promise->then( | |
| function ($result) use ($resetFlag) { | |
| $resetFlag(); | |
| return $result; | |
| }, |
This pull request introduces robust handling for Composer path repositories in the
Installerclass, ensuring that local package files are never overwritten or deleted during install, update, or uninstall operations. It also adds comprehensive tests to verify these behaviors. The main improvements include path repository detection, guarded install/update/uninstall logic, and a skip mechanism for unnecessary code updates.Path repository handling
isPathRepository()method to detect packages served from local path repositories, preventing destructive operations on user files. (src/Installer.php, src/Installer.phpR278-R375)install(),update(), anduninstall()methods to skip file operations for path repositories, logging informative messages and only updating Composer's internal state. (src/Installer.php, [1] [2]Update logic improvements
skipUpdateCodeflag and related logic to allow parent update methods to run only for their side effects (such as updatinginstalled.json), while skipping actual file operations when appropriate. (src/Installer.php, [1] [2]src/Installer.php, src/Installer.phpR384-R410)Testing enhancements
updateCode(). (tests/ModuleInstallerTest.php, tests/ModuleInstallerTest.phpR498-R644)TestableInstaller) to expose and verify new internal behaviors. (tests/ModuleInstallerTest.php, tests/ModuleInstallerTest.phpR62-R85)These changes make the installer safer and more predictable when working with local development packages, and the new tests ensure these behaviors are well-covered.