chore(legacy): remove the legacy PHP installer and self-update - #186
pjcdawkins wants to merge 6 commits into
Conversation
The legacy CLI now only runs through the Go wrapper's embedded PHP binary, so the pre-5.x installer and self-update code paths are unused. Removed: - dist/ (installer.php, manifest.json, dev-build-index.php), the installer test and scripts, and .platform.app.yaml (dev-build hosting) - self:update, self:release and self:stats, with SelfUpdater, SelfUpdateChecker and ManifestStrategy - the installer_url, manifest_url, github_repo, release_branch and updates.* legacy config keys - the padraic/phar-updater and padraic/humbug_get_contents dependencies - the installer config check in self:build - the "Legacy installer" README section The https://platform.sh/cli/installer and manifest.json URLs are hosted separately and are not affected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 2 minor points
🔍 Full review · 30 files reviewed
🔵 Minor points
legacy/config-defaults.yaml:399— The PR removes theupdates.checkkey, butinternal/legacy/legacy.go:144still passesenvPrefix+"UPDATES_CHECK=0"to every legacy PHP process. The setting existed to keepSelfUpdateCheckerquiet, and that class is now deleted, so the variable no longer affects anything. It also suggests the PHP layer still has an update check.legacy/src/Command/Self/SelfReleaseCommand.php:17—Platformsh\Cli\Util\VersionUtilwas only used byself:releaseand the self-update code. With both removed, nothing inlegacy/srccalls it; onlytests/Util/VersionUtilTest.phpdoes. The class and its test are now dead code that the PR left behind.
Verification
- Nothing under legacy/src, legacy/tests or legacy/resources still references SelfUpdateChecker, SelfUpdater, ManifestStrategy, or the removed config keys (installer_url, manifest_url, github_repo, release_branch, updates.*).
- phpstan-baseline.neon has no leftover entries for the deleted files, so phpstan's unmatched-ignore check will not fail on them.
- SelfBuildCommand is still complete after checkInstallerFile() is removed, and none of its imports were used only by that method.
- Application.php still uses
$containerafter the update-check block is removed; the LegacyMigration check on the next lines reads it. - No CI workflow, Makefile target or box config refers to legacy/dist or the deleted scripts/test/installer.sh and build-and-install.sh.
No tests were added. The deletions are covered by the ci.yml PHP job (php-cs-fixer, phpstan, PHPUnit via scripts/test/unit.sh, and a self:build phar build), and the Go test job covers the edits to the wrapper config. commands/abbreviation_test.go keeps self:update only as a stub fixture, so it does not depend on the real command.
Review details
- Commit: e3dc24a
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
VersionUtil was only used by self:release and the self-update code. Also note why the wrapper still sets UPDATES_CHECK=0 for the PHP CLI: wrapper processes spawned from it (e.g. ssh-cert:load via SSH config) inherit the variable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🔵
legacy/src/Command/Self/SelfReleaseCommand.php:17: Leaves unused code to maintain. (first raised)
Verification
- Nothing in the .php, .go, .yaml or .neon files still refers to
VersionUtil, so deleting it and its test leaves no dangling reference. internal/update.go:122readsEnvPrefix+"UPDATES_CHECK", so the env var on legacy.go line 145 still stops update checks in Go wrapper processes that the PHP CLI starts, as the new comment says.
This increment deletes an unused PHP utility and its test, and adds one comment. No test is added. The VersionUtil removal is covered by the PHPUnit and PHPStan runs on the legacy code; the comment changes no behaviour.
Review 2 of 10 for this pull request · View the full run
legacy/README.md described the archived standalone CLI (install, upgrade and usage for platformsh/cli). It is now a short developer note for the PHP layer, and absorbs the still-relevant parts of legacy/CONTRIBUTING.md, which pointed at the archived repository. The user config file and environment variable docs move to a new Configuration section in the root README, using the UPSUN_CLI_ prefix. This also fixes the SSH certificate variable name, which is AUTO_LOAD_SSH_CERT, not SSH_AUTO_LOAD_CERT. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
📋 PR Summary This PR removes the pre-5.x PHP installer and the PHP self-update code. The legacy CLI now only runs through the Go wrapper's embedded PHP binary, so these were no longer used. The related commands, services, config keys, Composer dependencies and tests are removed too, and the legacy PHP minimum goes up to 8.4. The docs move into a new Configuration section in the root README. The latest push stops the wrapper from setting Changes
|
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🔵
legacy/src/Command/Self/SelfReleaseCommand.php:17: Leaves unused code to maintain. (first raised)
Verification
- Every env var listed in the new README Configuration section matches the code: the Config.php override map for TOKEN, DEBUG, DISABLE_CACHE, DEFAULT_TIMEOUT, SESSION_ID and AUTO_LOAD_SSH_CERT, plus the direct reads in Application.php and SelfInstallCommand.
- The documented colour behaviour matches Application.php: CLICOLOR_FORCE=1 turns colour on, and NO_COLOR or CLICOLOR_FORCE=0 turns it off.
- The user config path
~/.upsun-cli/config.yamlcomes from Config.php, which reads getUserConfigDir()/config.yaml, and from user_config_dir.upsun-cliin upsun-cli.yaml. - The
make lint,make testandmake cleantargets and the.env-distfile cited in legacy/README.md all exist under legacy/. - No remaining file in the repo links to the deleted legacy/CONTRIBUTING.md.
This increment only changes documentation, so no tests cover it. I checked the documented env vars, paths and make targets by hand against legacy/src and legacy/Makefile.
Review 3 of 10 for this pull request · View the full run
The wrapper embeds PHP 8.4, so raise the minimum and the Composer platform version to match, and update the development Dockerfile. Locked dependency versions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🔵
legacy/src/Command/Self/SelfReleaseCommand.php:17: Leaves unused code to maintain. (first raised)
Verification
- composer.json require.php, config.platform.php and composer.lock platform/platform-overrides all consistently say 8.4.
- legacy/Dockerfile base image php:8.4-cli matches the php-version '8.4' set in ci.yml and release.yml.
- No locked dependency in composer.lock declares a PHP constraint that excludes 8.4 (only ^8.1 and ^8.2 remain).
- internal/legacy/legacy.go now documents UPDATES_CHECK=0 as suppressing update checks in wrapper processes the PHP CLI spawns.
No tests change in these commits. The ci.yml jobs that set up PHP use php-version '8.4' (lines 67 and 112), and the composer.lock platform settings were regenerated to match; I did not read those jobs or see their results.
Review 4 of 10 for this pull request · View the full run
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental (base moved) · 1 base file checked
Outstanding from earlier reviews:
- 🔵
legacy/src/Command/Self/SelfReleaseCommand.php:17: Leaves unused code to maintain. (first raised)
Verification
- The base fixture change only touches legacy/tests/data/apps/build-deps/php/_platform.app.yaml, which none of the files this PR removed refer to.
- legacy/tests/Local/BuildFlavor/DependenciesTest.php still points at tests/data/apps/build-deps, a directory this PR does not touch.
- internal/update.go:122 reads <PREFIX>UPDATES_CHECK, so the variable set at internal/legacy/legacy.go:145 still stops update checks in wrapper processes the PHP CLI starts.
This round has no PR diff. The base change is a fixture used by DependenciesTest, which runs with the legacy PHP unit tests (make test in legacy/). No PR test covers the fixture's interaction with this change, and none is needed.
Review 5 of 10 for this pull request · View the full run
It only disabled the PHP CLI's own update checker, which has been removed. The wrapper's update check is passive in the processes the PHP CLI may spawn (e.g. ssh-cert:load via SSH config): it prints only a notice, and not in quiet mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 1 still open
🔁 Incremental (head + base moved) · 2 files reviewed
Outstanding from earlier reviews:
- 🔵
legacy/src/Command/Self/SelfReleaseCommand.php:17: Leaves unused code to maintain. (first raised)
Verification
internal/update.go:122still gates the Go update check on<PREFIX>UPDATES_CHECK != "0", so the variable is still read by the Go side.- No other env var passed in
Exec(MIGRATE_CHECK, WRAPPED, etc.) is checked byshouldCheckForUpdate, so nothing else turns off the check in nested wrapper processes. - The base change (#187) only changes
LocalProject::getGitExcludePath/writeGitExcludeand tests, and none of the symbols this PR removes are involved.
This increment has no tests. No Go test checks the environment CLIWrapper.Exec builds, or checks that nested wrapper processes skip the update check. The Go test job (make test) will not catch the removal.
Review 6 of 10 for this pull request · View the full run
This branch has not been deployed
1 participant
The legacy CLI only runs through the Go wrapper's embedded PHP binary, so the pre-5.x installer (
curl -sS https://platform.sh/cli/installer | php) and the PHP self-update code are unused.Removed:
legacy/dist/(installer, manifest, dev-build index), the installer test and scripts, andlegacy/.platform.app.yamlself:update,self:releaseandself:stats, withSelfUpdater,SelfUpdateChecker,ManifestStrategyandVersionUtilinstaller_url,manifest_url,github_repo,release_branchandupdates.*legacy config keyspadraic/phar-updaterandpadraic/humbug_get_contentsdependenciesself:build(still used to build the embedded phar)self:updatefrom the wrapper'sdisabled_commands, and theUPDATES_CHECK=0the wrapper passed to the PHP CLIThe legacy PHP minimum is raised to 8.4 (in
composer.json, the Composer platform config and the dev Dockerfile), matching the embedded binary.Docs:
legacy/README.mddescribed the archived standalone CLI; it is now a short developer note, absorbinglegacy/CONTRIBUTING.md. The configuration and environment variable docs move to a new Configuration section in the root README (also fixingSSH_AUTO_LOAD_CERT→AUTO_LOAD_SSH_CERT).The
platform.sh/cli/installerandmanifest.jsonURLs are hosted separately and are not affected.🤖 Generated with Claude Code