fix: make p:upgrade survive its own dependency swap - #5705
Open
svaningelgem wants to merge 5 commits into
Open
Conversation
The self-upgrade has been documented as inoperable for years. The cause is that the command replaces vendor/ underneath the PHP process running it and then keeps calling into the framework: composer install swaps the autoloader and every class under vendor/, but PHP cannot unload classes already held in memory, so every step after it ran against a mix of old and new definitions. Re-requiring bootstrap/app.php could not fix that, because the container is not what is stale. The upgrade is now split across two processes. The first runs from the installed code and stops once the new code is on disk; the second is a fresh 'artisan p:upgrade --finalize', so it boots the code it is about to migrate. Alongside that: - Every external command's exit status is now checked. All eleven were discarded before, so a 404 on the download unpacked nothing, ran migrations against the old tree, and still reported a successful upgrade. - Maintenance mode is entered before the archive is unpacked rather than after, so new code is never served against the old schema. - --user and --group were only ever tested for null and their values never used, so every run chowned to www-data. They are honoured now, and detection also works when the command runs non-interactively. - The PHP version guard printed an error and then carried on regardless. It is replaced by composer check-platform-reqs against the manifests inside the downloaded archive, so an unsupported PHP version or a missing extension is caught while the Panel is still online and untouched. - The archive is downloaded to a temporary file and can be verified with --checksum, rather than piped straight into tar where a truncated transfer overwrote the installation with nothing to fall back on. - chown -R targets '.' rather than '*', which silently skipped every dotfile, .env included. - Pre-flight checks for the required binaries, a writable tree, free disk space and a reachable database run before anything is touched. - A failure once the Panel is offline leaves it in maintenance mode on purpose and prints how to resume, rather than exposing a half-upgraded tree.
Contributor
|
Hi as part of our contributing guide I'm going to need to see this in a bit of a different format before I review the pr. |
The docblocks explained the reasoning at review length rather than describing the methods. Cut to a line or two each; the rationale belongs in the pull request, not in every header.
upgrade() read as one long body with the sequence buried in it. The phases are now named calls: preflight, downloadArchive, replaceInstallation. preflight throws like everything else instead of returning an error string. The progress bar came back with it. It cannot span the handover, so each process draws its own: seven steps in the first half, four when the download is skipped, six in --finalize. Both abort paths clear it before printing.
Author
Fair pushback. I read it before posting, but I noticed some things I'd still change. Changes done and it's now much closer to how I prefer to see my own code. Thanks for reminding me not to always trust bots 😉 |
Adds seventeen cases: the --finalize half (with the Artisan commands it calls stubbed out, so no migrated database is needed), ownership defaults and a failing chown, checksum match and absence, the platform requirement check passing, failing and being skipped when the archive has no manifests, an unreachable database, a missing binary, a read-only tree, both interactive confirmations, the production composer flags, and --url and --release. UpgradeCommand goes from 57.59% to 97.47% of lines and 60.43% to 97.84% of branches. What is left is the disk space guard, which cannot be provoked without a seam in the command, and one side of each posix_* function_exists ternary, which is dead on whichever platform the suite happens to run on.
ext-posix is a hard requirement in composer.json and only exists on Unix, so function_exists() around posix_getpwuid and posix_getgrgid can never be false anywhere the Panel runs. The guard was dead code hiding a fallback that would never fire, and 1.0-develop already calls posix_getpwuid unguarded. Coverage of the command reaches 98.70% of lines and 99.25% of branches; only the free disk space guard is left, which cannot be provoked without adding a seam to the command.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The upgrade guide currently tells people not to use
p:upgrade("The self-upgrade is currently in-operable due to issues with some dependencies we make use of") and lists manual steps that are the same steps this command already runs.composer installreplaces the autoloader and every class undervendor/while the command that started it is still running, and PHP cannot unload classes it has already loaded.Every
$this->call()after that line runs against a mix of the definitions in memory and the files now on disk.The
require bootstrap/app.phpon1.0-developrebuilds the container, but the container is not the stale part.So the upgrade is now split over two processes.
The first runs from the installed code and stops once the new code is on disk.
The second is a fresh
artisan p:upgrade --finalizethat boots the code it is about to migrate.composer.jsonalready does this for the same reason, running@php artisan package:discoveras its own process inpost-autoload-dump.The rest is fallout from actually reading the command:
$process->run()results were discarded (ref: Self Update fails because files are not downloaded #3159).artisan downran after the archive was unpacked. Now it runs before, so new code is never served against the old schema.--userand--groupwere only tested withis_null()and their values never assigned, so--user=nginxstill producedchown -R www-data:www-data.$this->error()and then carried on with noreturn.composer check-platform-reqs: Composer evaluates its own constraints and catches missing extensions too. A hardcoded8.2.0would have passed on PHP 8.4, wherecomposer installthen fails, sincerequireis^8.2 || ^8.3.tarchown -Rtargets.instead of*, which skipped every dotfile including.env.Not addressed: unpacking over the top still never deletes files removed upstream. Fixing that means release directories and an atomic symlink swap, which would restructure the webroot of every existing installation. The manual guide has the same gap.
Tests cover the command end to end: both halves, the pre-flight failures, checksum match and mismatch, the platform check passing and failing, the interactive prompts, and --url and --release. That is 98.7% of lines and 99.25% of branches; what is left is the free disk space guard, which cannot be provoked without adding a seam to the command.