From 686c0a76f41e8affc36127c1878f870e6af751ca Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Sun, 26 Jul 2026 23:09:46 +0200 Subject: [PATCH 1/2] Improvements after review --- internal/ini/ini.go | 34 +++++- internal/ini/ini_test.go | 45 ++++++++ internal/installer/extension.go | 66 ++++++++---- internal/installer/extension_test.go | 109 ++++++++++++++++++- internal/installer/iniconfig.go | 111 ++++++++++++++----- internal/installer/install.go | 47 +++++++- internal/installer/install_test.go | 155 +++++++++++++++++++++++++++ internal/installer/uninstall.go | 9 ++ internal/installer/uninstall_test.go | 49 +++++++++ internal/manifest/manifest.go | 11 ++ internal/php/parse_test.go | 3 +- internal/php/php.go | 11 +- internal/platform/arch.go | 17 +++ internal/platform/arch_test.go | 23 ++++ 14 files changed, 638 insertions(+), 52 deletions(-) diff --git a/internal/ini/ini.go b/internal/ini/ini.go index 81adcc7..0ae7362 100644 --- a/internal/ini/ini.go +++ b/internal/ini/ini.go @@ -19,12 +19,21 @@ var AllowedModes = []string{"off", "debug"} // rewritten content and the list of removed lines (trimmed of any trailing CR) // for reporting. func StripXdebugLoaders(content string) (string, []string) { + return stripZendLoaders(content, referencesXdebug) +} + +// stripZendLoaders removes every `zend_extension=` directive whose value matches, +// whether the line is active or commented out, returning the rewritten content +// and the removed lines (trimmed of any trailing CR) for reporting. Only +// zend_extension is considered, because both xdebug and the php-debugger +// extension load solely via it. +func stripZendLoaders(content string, matches func(value string) bool) (string, []string) { lines := strings.Split(content, "\n") kept := make([]string, 0, len(lines)) var removed []string for _, ln := range lines { _, key, value, ok := parseDirective(ln) - if ok && key == "zend_extension" && referencesXdebug(value) { + if ok && key == "zend_extension" && matches(value) { removed = append(removed, strings.TrimRight(ln, "\r")) continue } @@ -60,6 +69,21 @@ func CommentExtensionLoaders(content string) (string, []string) { return strings.Join(lines, "\n"), commented } +// StripPhpDebuggerLoaders removes every `zend_extension=` directive that loads +// the php-debugger extension (its value references the php-debugger .so), whether +// active or commented, returning the rewritten content and the removed lines. +// Like xdebug, the extension only loads via zend_extension, so `extension=` lines +// are left alone. +// +// It is applied when copying an existing php's config onto the self-contained +// debugger interpreter, which already has the debugger compiled in: loading the +// standalone extension on top would register the module twice. Unlike +// CommentExtensionLoaders this runs regardless of ABI match, because the built-in +// debugger supersedes the extension in every case. +func StripPhpDebuggerLoaders(content string) (string, []string) { + return stripZendLoaders(content, referencesDebugger) +} + // DisallowedModes returns the de-duplicated set of xdebug.mode tokens present in // xdebug.mode directives (active or commented out) that are not in AllowedModes, // preserving first-seen order. It is empty if xdebug.mode is absent or already @@ -180,6 +204,14 @@ func referencesXdebug(value string) bool { return strings.Contains(strings.ToLower(unquote(value)), "xdebug") } +// referencesDebugger reports whether a loader value points at the php-debugger +// extension. Its .so is named php-debugger-*, while the module reports as +// php_debugger; accept either spelling to be safe. +func referencesDebugger(value string) bool { + v := strings.ToLower(unquote(value)) + return strings.Contains(v, "php-debugger") || strings.Contains(v, "php_debugger") +} + func parseModeList(value string) []string { var out []string for _, p := range strings.Split(unquote(value), ",") { diff --git a/internal/ini/ini_test.go b/internal/ini/ini_test.go index 3b6e57e..50406ce 100644 --- a/internal/ini/ini_test.go +++ b/internal/ini/ini_test.go @@ -119,6 +119,51 @@ func TestCommentExtensionLoaders(t *testing.T) { } } +func TestStripPhpDebuggerLoaders(t *testing.T) { + tests := []struct { + name string + in string + want string + wantRemoved []string + }{ + { + name: "removes the php-debugger zend_extension loader", + in: "zend_extension=/usr/lib/php/ext/php-debugger-php8.3-nts-linux-x86_64.so\nmemory_limit=256M\n", + want: "memory_limit=256M\n", + wantRemoved: []string{"zend_extension=/usr/lib/php/ext/php-debugger-php8.3-nts-linux-x86_64.so"}, + }, + { + name: "accepts the underscore spelling and removes commented loaders too", + in: "; zend_extension=php_debugger.so\n", + want: "", + wantRemoved: []string{"; zend_extension=php_debugger.so"}, + }, + { + name: "ignores extension= (loads only via zend_extension)", + in: "extension=php-debugger.so\n", + want: "extension=php-debugger.so\n", + wantRemoved: nil, + }, + { + name: "leaves unrelated loaders alone", + in: "zend_extension=xdebug.so\nextension=mysqli.so\n", + want: "zend_extension=xdebug.so\nextension=mysqli.so\n", + wantRemoved: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, removed := StripPhpDebuggerLoaders(tt.in) + if got != tt.want { + t.Errorf("content = %q, want %q", got, tt.want) + } + if !reflect.DeepEqual(removed, tt.wantRemoved) { + t.Errorf("removed = %#v, want %#v", removed, tt.wantRemoved) + } + }) + } +} + func TestDisallowedModes(t *testing.T) { tests := []struct { name string diff --git a/internal/installer/extension.go b/internal/installer/extension.go index e5a6b6c..912a2e4 100644 --- a/internal/installer/extension.go +++ b/internal/installer/extension.go @@ -45,19 +45,26 @@ func InstallExtension(ctx context.Context, opts Options) error { if err != nil { return fmt.Errorf("querying php at %s: %w", path, err) } - if existing.ExtensionDir == "" { - return fmt.Errorf("php at %s reports no extension_dir; cannot install the extension", path) - } - // Nothing to do if the debugger is already present (e.g. our own - // interpreter), unless forced (as by `update`). + // Nothing to do if the debugger is already available, unless forced (as by + // `update`). This covers two cases: the php on PATH already loads the + // extension, or it is our own interpreter with the debugger compiled in + // (both report the module via `php -m`). if !opts.Force { if has, _ := php.HasModule(ctx, path, php.DebuggerModule); has { - opts.logf("%s already has the %s module; nothing to do.", path, php.DebuggerModule) + if isOurInterpreter(path, layout.Root) { + opts.logf("php at %s is the php-debugger interpreter, which already includes the debugger built in; nothing to do.", path) + } else { + opts.logf("php at %s already loads the %s extension; nothing to do.", path, php.DebuggerModule) + } return nil } } + if existing.ExtensionDir == "" { + return fmt.Errorf("php at %s reports no extension_dir; cannot install the extension", path) + } + client := opts.Client if client == nil { client = release.NewClient() @@ -66,19 +73,33 @@ func InstallExtension(ctx context.Context, opts Options) error { if err != nil { return err } + + // The extension must match the architecture the existing php runs as, which + // can differ from the host (e.g. an Intel php under Rosetta on Apple Silicon). + // Fall back to the host arch only if php did not report its machine. + arch := p.Arch + if existing.Machine != "" { + a, err := platform.ArchFromMachine(existing.Machine) + if err != nil { + return fmt.Errorf("determining architecture of php at %s: %w", path, err) + } + arch = a + } + target := platform.Platform{OS: p.OS, Arch: arch} + asset, err := release.SelectAsset(rel.Assets, release.Selector{ Kind: release.Extension, Series: existing.Series, ZTS: existing.ZTS, - OS: p.OS, - Arch: p.Arch, + OS: target.OS, + Arch: target.Arch, }) if err != nil { return err } opts.logf("Installing php-debugger extension for php %s (%s) %s, release %s", - existing.Series, threading(existing.ZTS), p, rel.TagName) + existing.Series, threading(existing.ZTS), target, rel.TagName) tmpDir, err := os.MkdirTemp("", "php-debugger-ext-") if err != nil { @@ -105,7 +126,10 @@ func InstallExtension(ctx context.Context, opts Options) error { opts.logf(" copied %s", soDst) // --- disable any real xdebug in the existing ini files --- - if err := stripXdebugFromExisting(existing, rb, opts); err != nil { + // Back the originals up (under the install root) so uninstall can restore the + // user's xdebug config, since these are their live files, not copies. + iniBackups, err := stripXdebugFromExisting(existing, layout.BackupsDir(), rb, opts) + if err != nil { rb.run() return fmt.Errorf("updating existing ini files; reverted: %w", err) } @@ -138,13 +162,14 @@ func InstallExtension(ctx context.Context, opts Options) error { } m.InstallRoot = layout.Root m.SetExtension(manifest.Extension{ - Series: existing.Series, - PHPVersion: existing.Version, - ZTS: existing.ZTS, - ReleaseTag: rel.TagName, - SoPath: soDst, - IniPath: iniPath, - InstalledAt: opts.clock(), + Series: existing.Series, + PHPVersion: existing.Version, + ZTS: existing.ZTS, + ReleaseTag: rel.TagName, + SoPath: soDst, + IniPath: iniPath, + InstalledAt: opts.clock(), + ConfigBackups: iniBackups, }) if err := m.Save(layout.ManifestPath()); err != nil { rb.run() @@ -158,14 +183,15 @@ func InstallExtension(ctx context.Context, opts Options) error { // stripXdebugFromExisting disables a real xdebug in the existing php's ini files // (in place) so it does not conflict with the debugger's simulated one. It reuses // the shared ini rules (see rewriteIniFiles) without commenting other loaders — -// the existing php can load its own extensions. -func stripXdebugFromExisting(existing *php.Info, rb *rollback, opts Options) error { +// the existing php can load its own extensions. Modified files are backed up +// under backupDir and returned so uninstall can restore the user's xdebug config. +func stripXdebugFromExisting(existing *php.Info, backupDir string, rb *rollback, opts Options) ([]manifest.FileBackup, error) { var files []string if existing.Ini.LoadedFile != "" { files = append(files, existing.Ini.LoadedFile) } files = append(files, existing.Ini.AdditionalFiles...) - return sanitizeInPlace(files, rb, opts) + return sanitizeInPlace(files, backupDir, rb, opts) } // enableExtension writes a loader that enables the debugger extension: a diff --git a/internal/installer/extension_test.go b/internal/installer/extension_test.go index ee37e74..0368344 100644 --- a/internal/installer/extension_test.go +++ b/internal/installer/extension_test.go @@ -5,6 +5,8 @@ import ( "context" "errors" "fmt" + "net/http" + "net/http/httptest" "os" "path/filepath" "runtime" @@ -20,6 +22,13 @@ import ( // the loader ini exists AND simulateLoad is true — mimicking dynamic loading via // the enabled extension. It reports the given extension_dir and ini locations. func fakeExistingPHPForExt(version, extDir, loadedFile, scanDir string, simulateLoad bool) string { + return fakeExistingPHPForExtArch(version, "x86_64", extDir, loadedFile, scanDir, simulateLoad) +} + +// fakeExistingPHPForExtArch is fakeExistingPHPForExt with an explicit machine +// architecture reported by php_uname("m") (used to exercise Rosetta-style hosts +// where the php process arch differs from the host's native arch). +func fakeExistingPHPForExtArch(version, machine, extDir, loadedFile, scanDir string, simulateLoad bool) string { series := version if i := strings.LastIndex(version, "."); i >= 0 { series = version[:i] @@ -39,11 +48,11 @@ case "$1" in --ini) echo "Loaded Configuration File: \"%s\"" echo "Scan for additional .ini files in: \"%s\"" ;; - -r) printf 'version=%s\nseries=%s\nzts=0\nextension_dir=%s\n' ;; + -r) printf 'version=%s\nseries=%s\nzts=0\nmachine=%s\nextension_dir=%s\n' ;; *) : ;; esac exit 0 -`, version, sim, loader, loadedFile, scanDir, version, series, extDir) +`, version, sim, loader, loadedFile, scanDir, version, series, machine, extDir) } func TestInstallExtensionNoPHP(t *testing.T) { @@ -194,3 +203,99 @@ func TestInstallExtensionAlreadyPresent(t *testing.T) { t.Errorf("expected 'nothing to do' message, got: %s", out.String()) } } + +// When the php on PATH is our own interpreter (with the debugger compiled in), +// installing the extension is a no-op with a message naming the interpreter. +func TestInstallExtensionSkipsOwnInterpreter(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + root := filepath.Join(home, ".local", "share", "php-debugger") + + // Place a fake php that reports the debugger module *under our install root*. + ownBin := filepath.Join(root, "8.3", "bin") + writeExec(t, filepath.Join(ownBin, "php"), fakePHP("8.3.7", true, "", "")) + t.Setenv("PATH", ownBin) // php.Detect() finds our interpreter + + env := linuxUserEnv(home) + var out bytes.Buffer + // No release client: if the code tried to download, it would fail — proving + // we returned before touching the network. + err := InstallExtension(context.Background(), Options{Scope: platform.User, Out: &out, Env: &env}) + if err != nil { + t.Fatalf("expected a no-op, got: %v", err) + } + if !strings.Contains(out.String(), "interpreter") || !strings.Contains(out.String(), "nothing to do") { + t.Errorf("expected an interpreter-specific message, got: %s", out.String()) + } + // No extension should be recorded. + if _, err := os.Stat(filepath.Join(root, "manifest.json")); !os.IsNotExist(err) { + t.Error("no manifest should be written when nothing is installed") + } +} + +// newFakeExtReleaseServer serves a release whose extension assets exist for both +// architectures of the given os token, so a test can assert which one is chosen. +func newFakeExtReleaseServer(t *testing.T, osTok string) *httptest.Server { + t.Helper() + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/releases/latest"): + fmt.Fprintf(w, `{"tag_name":"9.9.9","assets":[ + {"name":"php-debugger-php8.3-nts-%s-x86_64.so","browser_download_url":%q,"size":%d}, + {"name":"php-debugger-php8.3-nts-%s-arm64.so","browser_download_url":%q,"size":%d} + ]}`, osTok, srv.URL+"/dl/x86_64", len(fakeSO), osTok, srv.URL+"/dl/arm64", len(fakeSO)) + case strings.HasPrefix(r.URL.Path, "/dl/"): + w.Write([]byte(fakeSO)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + return srv +} + +// On an Apple-Silicon host running an Intel php under Rosetta, the extension must +// match the php process's architecture (x86_64), not the host's native arm64. +func TestInstallExtensionMatchesPHPArch(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + extDir := t.TempDir() + scanDir := filepath.Join(t.TempDir(), "conf.d") + + binDir := filepath.Join(t.TempDir(), "bin") + // php reports itself as x86_64 even though the host env below is arm64. + writeExec(t, filepath.Join(binDir, "php"), + fakeExistingPHPForExtArch("8.3.7", "x86_64", extDir, "", scanDir, true)) + t.Setenv("PATH", binDir) + + srv := newFakeExtReleaseServer(t, "macos") + client := release.NewClient() + client.BaseURL = srv.URL + + // Host is Apple Silicon (macos/arm64). + env := platform.Env{ + OS: platform.MacOS, Arch: platform.Arm64, Home: home, + Getenv: func(string) string { return "" }, + } + + var out bytes.Buffer + err := InstallExtension(context.Background(), Options{ + Scope: platform.User, AssumeYes: true, Out: &out, Client: client, Env: &env, + }) + if err != nil { + t.Fatalf("InstallExtension: %v\n%s", err, out.String()) + } + + // The x86_64 extension (matching php) must be chosen, not the host's arm64. + if _, err := os.Stat(filepath.Join(extDir, "php-debugger-php8.3-nts-macos-x86_64.so")); err != nil { + t.Errorf("expected x86_64 extension to be installed (matching php arch): %v", err) + } + if _, err := os.Stat(filepath.Join(extDir, "php-debugger-php8.3-nts-macos-arm64.so")); !os.IsNotExist(err) { + t.Error("arm64 extension should not have been installed on a Rosetta php") + } +} diff --git a/internal/installer/iniconfig.go b/internal/installer/iniconfig.go index f65eeed..4eee1eb 100644 --- a/internal/installer/iniconfig.go +++ b/internal/installer/iniconfig.go @@ -5,9 +5,11 @@ import ( "os" "path/filepath" "sort" + "strconv" "strings" "github.com/php-debugger/installer/internal/ini" + "github.com/php-debugger/installer/internal/manifest" "github.com/php-debugger/installer/internal/php" ) @@ -22,33 +24,47 @@ type iniRewriteOptions struct { // loaders. Used for the interpreter (a self-contained build cannot load // foreign .so files); not for the extension (the existing php loads its own). commentOtherLoaders bool + // stripDebuggerLoader removes any existing php-debugger extension loader. Used + // for the interpreter (it has the debugger compiled in, so the standalone + // extension must not also load) regardless of ABI match. + stripDebuggerLoader bool // skipUnchanged avoids writing (and registering undo for) files the rules // leave untouched. Used for in-place edits of an existing php's config. skipUnchanged bool + // backupDir, when set, makes each in-place edit save the file's original + // contents there first, returned as manifest.FileBackup entries so uninstall + // can restore them (e.g. bringing back a stripped xdebug). Used only by the + // extension flow, which mutates the user's real ini files. + backupDir string } // rewriteIniFiles applies the shared ini rules to a set of (src -> dst) pairs: // strip xdebug loaders, optionally comment other extension loaders, and — after // a single confirmation covering all files — sanitize disallowed xdebug.mode // tokens. It registers undo steps on rb and returns the destination files -// written. This is the one place the xdebug/ini handling lives; the interpreter -// and extension flows differ only via iniRewriteOptions. -func rewriteIniFiles(pairs []configPair, cfg iniRewriteOptions, rb *rollback, opts Options) ([]string, error) { +// written plus any original-content backups made (see iniRewriteOptions.backupDir). +// This is the one place the xdebug/ini handling lives; the interpreter and +// extension flows differ only via iniRewriteOptions. +func rewriteIniFiles(pairs []configPair, cfg iniRewriteOptions, rb *rollback, opts Options) ([]string, []manifest.FileBackup, error) { stripModes, err := decideStripModes(pairs, opts) if err != nil { - return nil, err + return nil, nil, err } var written []string + var backups []manifest.FileBackup for _, pr := range pairs { data, err := os.ReadFile(pr.src) if err != nil { - return written, fmt.Errorf("reading ini %s: %w", pr.src, err) + return written, backups, fmt.Errorf("reading ini %s: %w", pr.src, err) + } + content, removedXdebug := ini.StripXdebugLoaders(string(data)) + var removedDebugger, commentedOther []string + if cfg.stripDebuggerLoader { + content, removedDebugger = ini.StripPhpDebuggerLoaders(content) } - content, removedLoaders := ini.StripXdebugLoaders(string(data)) - var commentedLoaders []string if cfg.commentOtherLoaders { - content, commentedLoaders = ini.CommentExtensionLoaders(content) + content, commentedOther = ini.CommentExtensionLoaders(content) } if stripModes { content, _, _ = ini.SanitizeXdebugMode(content) @@ -57,24 +73,53 @@ func rewriteIniFiles(pairs []configPair, cfg iniRewriteOptions, rb *rollback, op continue } + // Persist the original contents (for uninstall) before overwriting. + if cfg.backupDir != "" { + bpath, err := saveIniBackup(cfg.backupDir, pr.dst, data, len(backups)) + if err != nil { + return written, backups, err + } + rb.add(func() error { return removeIfExists(bpath) }) + backups = append(backups, manifest.FileBackup{OriginalPath: pr.dst, BackupPath: bpath}) + } + if err := registerFileRestore(pr.dst, rb, 0o644); err != nil { - return written, err + return written, backups, err } if err := os.MkdirAll(filepath.Dir(pr.dst), 0o755); err != nil { - return written, fmt.Errorf("creating config dir: %w", err) + return written, backups, fmt.Errorf("creating config dir: %w", err) } if err := os.WriteFile(pr.dst, []byte(content), 0o644); err != nil { - return written, fmt.Errorf("writing ini %s: %w", pr.dst, err) + return written, backups, fmt.Errorf("writing ini %s: %w", pr.dst, err) } written = append(written, pr.dst) - opts.logf(" %s%s", pr.dst, loaderNote(len(removedLoaders), len(commentedLoaders))) + opts.logf(" %s%s", pr.dst, loaderNote(len(removedXdebug), len(removedDebugger), len(commentedOther))) + } + return written, backups, nil +} + +// saveIniBackup writes the original contents of originalPath into backupDir under +// a unique, traceable name, returning the backup path. +func saveIniBackup(backupDir, originalPath string, data []byte, index int) (string, error) { + if err := os.MkdirAll(backupDir, 0o755); err != nil { + return "", fmt.Errorf("creating backup dir: %w", err) + } + bpath := filepath.Join(backupDir, "ini-"+strconv.Itoa(index)+"-"+filepath.Base(originalPath)) + if err := os.WriteFile(bpath, data, 0o644); err != nil { + return "", fmt.Errorf("backing up %s: %w", originalPath, err) } - return written, nil + return bpath, nil } // copyConfig copies the existing interpreter's ini files into the new // interpreter's compiled-in config path (so the new php loads the same // configuration), sanitizing each on the way. Returns the destination files. +// +// Other (non-xdebug) extension loaders are commented out only when the new +// interpreter has a different ABI from the one it replaces — a foreign .so built +// for a different PHP version or thread-safety cannot load and would error on +// every run. When the replacement is the same PHP series and thread-safety, those +// extensions load fine, so their loaders are left intact. func copyConfig(existing, target *php.Info, rb *rollback, opts Options) ([]string, error) { pairs := interpreterConfigPairs(existing, target) if len(pairs) == 0 { @@ -83,15 +128,30 @@ func copyConfig(existing, target *php.Info, rb *rollback, opts Options) ([]strin } return nil, nil } - return rewriteIniFiles(pairs, iniRewriteOptions{commentOtherLoaders: true}, rb, opts) + sameABI := existing.Series == target.Series && existing.ZTS == target.ZTS + if sameABI { + opts.logf(" (same PHP %s %s as before; keeping existing extension loaders)", + target.Series, threading(target.ZTS)) + } + // The debugger loader is always stripped: the interpreter has it built in, so + // loading a standalone php-debugger extension on top would double-register it. + // No backupDir: this writes copies into the new interpreter's config path and + // never modifies the user's originals, so there is nothing to restore later. + written, _, err := rewriteIniFiles(pairs, iniRewriteOptions{ + commentOtherLoaders: !sameABI, + stripDebuggerLoader: true, + }, rb, opts) + return written, err } // sanitizeInPlace applies the xdebug ini rules to the given existing ini files // in place (used by the extension flow to disable a real xdebug). Only files the -// rules change are touched. -func sanitizeInPlace(files []string, rb *rollback, opts Options) error { - _, err := rewriteIniFiles(inPlacePairs(files), iniRewriteOptions{skipUnchanged: true}, rb, opts) - return err +// rules change are touched. Each modified file's original contents are saved +// under backupDir and returned so uninstall can restore them. +func sanitizeInPlace(files []string, backupDir string, rb *rollback, opts Options) ([]manifest.FileBackup, error) { + _, backups, err := rewriteIniFiles(inPlacePairs(files), + iniRewriteOptions{skipUnchanged: true, backupDir: backupDir}, rb, opts) + return backups, err } // interpreterConfigPairs maps the existing main php.ini to the new interpreter's @@ -172,14 +232,17 @@ func removeIfExists(path string) error { } // loaderNote summarizes what happened to extension loaders in a rewritten ini -// file, e.g. " (removed 1 xdebug loader(s), commented 2 extension loader(s))". -func loaderNote(removed, commented int) string { +// file, e.g. " (removed 1 xdebug loader(s), removed 1 php-debugger loader(s))". +func loaderNote(removedXdebug, removedDebugger, commentedOther int) string { var parts []string - if removed > 0 { - parts = append(parts, fmt.Sprintf("removed %d xdebug loader(s)", removed)) + if removedXdebug > 0 { + parts = append(parts, fmt.Sprintf("removed %d xdebug loader(s)", removedXdebug)) + } + if removedDebugger > 0 { + parts = append(parts, fmt.Sprintf("removed %d php-debugger loader(s)", removedDebugger)) } - if commented > 0 { - parts = append(parts, fmt.Sprintf("commented %d extension loader(s)", commented)) + if commentedOther > 0 { + parts = append(parts, fmt.Sprintf("commented %d extension loader(s)", commentedOther)) } if len(parts) == 0 { return "" diff --git a/internal/installer/install.go b/internal/installer/install.go index 59d9365..e1e32e6 100644 --- a/internal/installer/install.go +++ b/internal/installer/install.go @@ -142,6 +142,13 @@ func InstallInterpreter(ctx context.Context, opts Options) error { return err } + // If our own interpreter for this exact version (series + threading) is + // already active with the debugger, there is nothing to do. `update` sets + // Force to reinstall against the latest release regardless. + if !opts.Force && alreadyProvided(ctx, opts, layout.Root, series) { + return nil + } + // Detect a pre-existing interpreter before we change anything. Ignore one // that is our own previous install (avoid backing up our own symlink). existing := detectExisting(ctx, opts, layout.Root) @@ -299,6 +306,29 @@ func InstallInterpreter(ctx context.Context, opts Options) error { return nil } +// alreadyProvided reports whether the php currently on PATH is our own +// interpreter for this exact version (series + threading) with the debugger — in +// which case installing again would be a no-op. It logs an informative message +// when so. A normal php that merely loads our extension does NOT count: it +// returns false so the interpreter install proceeds (and disables that +// extension). Callers gate this on !Force. +func alreadyProvided(ctx context.Context, opts Options, root, series string) bool { + path, err := php.Detect() + if err != nil || !isOurInterpreter(path, root) { + return false + } + info, err := php.Query(ctx, path) + if err != nil || info.Series != series || info.ZTS != opts.ZTS { + return false + } + if has, _ := php.HasModule(ctx, path, php.DebuggerModule); !has { + return false + } + opts.logf("php %s (%s) with the debugger is already installed and active at %s; nothing to do.", + series, threading(opts.ZTS), path) + return true +} + // detectExisting finds a pre-existing php on PATH and queries it. It returns nil // if none is found, if it cannot be queried, or if it is our own previous // install under root (which must not be treated as a foreign interpreter). @@ -307,7 +337,7 @@ func detectExisting(ctx context.Context, opts Options, root string) *php.Info { if err != nil { return nil } - if resolved, e := filepath.EvalSymlinks(path); e == nil && isUnderRoot(resolved, root) { + if isOurInterpreter(path, root) { return nil // our own previously-installed interpreter } info, err := php.Query(ctx, path) @@ -339,6 +369,21 @@ func chooseLinkDir(existing *php.Info, layout platform.Layout, override string) return binDir, false, nil } +// isOurInterpreter reports whether the php at path is one we installed (its real +// path, after resolving symlinks, lives under our install root). Both sides are +// canonicalized so symlinked path prefixes (e.g. macOS /var -> /private/var) do +// not cause a false negative. +func isOurInterpreter(path, root string) bool { + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return false + } + if r, err := filepath.EvalSymlinks(root); err == nil { + root = r + } + return isUnderRoot(resolved, root) +} + func isUnderRoot(path, root string) bool { rel, err := filepath.Rel(root, path) if err != nil { diff --git a/internal/installer/install_test.go b/internal/installer/install_test.go index 46809bd..dc7ae47 100644 --- a/internal/installer/install_test.go +++ b/internal/installer/install_test.go @@ -237,6 +237,161 @@ func TestInstallInterpreterReplacesExisting(t *testing.T) { } } +// When the new interpreter has the same PHP series and thread-safety as the one +// it replaces, foreign extensions built for that ABI still load, so their loaders +// are left intact (only xdebug is stripped). +func TestInstallInterpreterSameABIKeepsLoaders(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + + // New interpreter is 8.3 NTS (see newFakeReleaseServer's asset name). + newCfg := filepath.Join(t.TempDir(), "newcfg") + newScan := filepath.Join(newCfg, "conf.d") + srv := newFakeReleaseServer(t, fakePHP("8.3.12", true, newCfg, newScan)) + + // Pre-existing php is the SAME series (8.3) and NTS. + existBin := filepath.Join(t.TempDir(), "bin") + existIni := t.TempDir() + existConfd := filepath.Join(existIni, "conf.d") + writeExec(t, filepath.Join(existBin, "php"), existingPHPScript("8.3.4", + filepath.Join(existIni, "php.ini"), existConfd, "")) + if err := os.WriteFile(filepath.Join(existIni, "php.ini"), + []byte("zend_extension=xdebug.so\nextension=mysqli.so\nmemory_limit=128M\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(existConfd, 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", existBin) + + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + var out bytes.Buffer + err := InstallInterpreter(context.Background(), Options{ + Scope: platform.User, AssumeYes: true, Out: &out, Client: client, Env: &env, + }) + if err != nil { + t.Fatalf("InstallInterpreter: %v\n--- output ---\n%s", err, out.String()) + } + + mainIni, err := os.ReadFile(filepath.Join(newCfg, "php.ini")) + if err != nil { + t.Fatalf("main php.ini not copied: %v", err) + } + s := string(mainIni) + // xdebug is always stripped. + if strings.Contains(s, "xdebug.so") { + t.Errorf("xdebug loader not stripped:\n%s", s) + } + // The mysqli loader is left active (not commented), because it loads fine. + if !strings.Contains(s, "\nextension=mysqli.so") && !strings.HasPrefix(s, "extension=mysqli.so") { + t.Errorf("same-ABI: mysqli loader should stay active, not be commented:\n%s", s) + } + if strings.Contains(s, "; extension=mysqli.so") { + t.Errorf("same-ABI: mysqli loader should not be commented:\n%s", s) + } +} + +// When our own interpreter for the same version (series + threading) is already +// active with the debugger, installing again is a no-op. +func TestInstallInterpreterAlreadyProvided(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + root := filepath.Join(home, ".local", "share", "php-debugger") + + // A fake php that reports the debugger, installed *under our root* (8.3 NTS, + // matching what newFakeReleaseServer offers). + ownBin := filepath.Join(root, "8.3", "bin") + writeExec(t, filepath.Join(ownBin, "php"), fakePHP("8.3.7", true, "", "")) + t.Setenv("PATH", ownBin) + + srv := newFakeReleaseServer(t, fakePHP("8.3.7", true, "", "")) + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + var out bytes.Buffer + err := InstallInterpreter(context.Background(), Options{ + Scope: platform.User, Out: &out, Client: client, Env: &env, + }) + if err != nil { + t.Fatalf("expected a no-op, got: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "nothing to do") { + t.Errorf("expected 'nothing to do' message, got: %s", out.String()) + } + if _, err := os.Stat(filepath.Join(root, "manifest.json")); !os.IsNotExist(err) { + t.Error("no manifest should be written for a no-op install") + } +} + +// When a normal php already loads our standalone extension, installing the +// interpreter proceeds but the copied config disables that extension loader — the +// interpreter has the debugger built in, so the .so must not also load. Other +// same-ABI loaders stay active. +func TestInstallInterpreterDisablesExistingDebuggerExtension(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + + newCfg := filepath.Join(t.TempDir(), "newcfg") + newScan := filepath.Join(newCfg, "conf.d") + srv := newFakeReleaseServer(t, fakePHP("8.3.12", true, newCfg, newScan)) + + // Pre-existing php is the SAME series (8.3 NTS) and its ini loads our + // extension plus an unrelated one. + existBin := filepath.Join(t.TempDir(), "bin") + existIni := t.TempDir() + existConfd := filepath.Join(existIni, "conf.d") + writeExec(t, filepath.Join(existBin, "php"), existingPHPScript("8.3.4", + filepath.Join(existIni, "php.ini"), existConfd, "")) + soLoader := "zend_extension=/usr/lib/php/ext/php-debugger-php8.3-nts-linux-x86_64.so" + if err := os.WriteFile(filepath.Join(existIni, "php.ini"), + []byte(soLoader+"\nextension=mysqli.so\nmemory_limit=128M\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(existConfd, 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", existBin) + + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + var out bytes.Buffer + err := InstallInterpreter(context.Background(), Options{ + Scope: platform.User, AssumeYes: true, Out: &out, Client: client, Env: &env, + }) + if err != nil { + t.Fatalf("InstallInterpreter: %v\n%s", err, out.String()) + } + + mainIni, err := os.ReadFile(filepath.Join(newCfg, "php.ini")) + if err != nil { + t.Fatalf("main php.ini not copied: %v", err) + } + s := string(mainIni) + // The php-debugger extension loader must be stripped entirely. + if strings.Contains(s, soLoader) { + t.Errorf("debugger extension loader should be removed:\n%s", s) + } + // Same ABI: the unrelated mysqli loader stays active. + if strings.Contains(s, "; extension=mysqli.so") { + t.Errorf("same-ABI: mysqli loader should stay active:\n%s", s) + } + if !strings.Contains(s, "\nextension=mysqli.so") && !strings.HasPrefix(s, "extension=mysqli.so") { + t.Errorf("same-ABI: mysqli loader should stay present and active:\n%s", s) + } +} + func TestInstallInterpreterRollbackOnMissingModule(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("fake php is a /bin/sh script") diff --git a/internal/installer/uninstall.go b/internal/installer/uninstall.go index 9ec57b4..fc676c6 100644 --- a/internal/installer/uninstall.go +++ b/internal/installer/uninstall.go @@ -155,6 +155,15 @@ func uninstallExtension(opts Options, layout platform.Layout, m *manifest.Manife return fmt.Errorf("removing %s: %w", ext.SoPath, err) } } + // Restore the ini files we modified in place at install (bringing back any + // xdebug we disabled). This runs last so it is authoritative over the loader + // removal above for a file that held both. + for _, cb := range ext.ConfigBackups { + if err := restoreBackup(cb.BackupPath, cb.OriginalPath); err != nil { + return fmt.Errorf("restoring %s: %w", cb.OriginalPath, err) + } + opts.logf("Restored %s.", cb.OriginalPath) + } m.ClearExtension() if err := finalizeManifest(layout, m); err != nil { return fmt.Errorf("saving manifest: %w", err) diff --git a/internal/installer/uninstall_test.go b/internal/installer/uninstall_test.go index 16fd2e6..4488b24 100644 --- a/internal/installer/uninstall_test.go +++ b/internal/installer/uninstall_test.go @@ -203,6 +203,55 @@ func TestUninstallExtension(t *testing.T) { } } +// Installing the extension strips xdebug from the existing php's ini; uninstalling +// must restore it (the ini is the user's live file, not a copy). +func TestUninstallExtensionRestoresXdebug(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + extDir := t.TempDir() + iniDir := t.TempDir() + scanDir := filepath.Join(iniDir, "conf.d") + loadedFile := filepath.Join(iniDir, "php.ini") + original := "zend_extension=xdebug.so\nxdebug.mode=develop,debug\nmemory_limit=100M\n" + if err := os.WriteFile(loadedFile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + binDir := filepath.Join(t.TempDir(), "bin") + writeExec(t, filepath.Join(binDir, "php"), + fakeExistingPHPForExt("8.3.7", extDir, loadedFile, scanDir, true)) + t.Setenv("PATH", binDir) + + srv := newFakeReleaseServer(t, "unused") + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + if err := InstallExtension(context.Background(), Options{ + Scope: platform.User, AssumeYes: true, Out: &bytes.Buffer{}, Client: client, Env: &env, + }); err != nil { + t.Fatalf("install extension: %v", err) + } + // After install, xdebug is gone and the mode is sanitized. + if b, _ := os.ReadFile(loadedFile); strings.Contains(string(b), "xdebug.so") || strings.Contains(string(b), "develop") { + t.Fatalf("install should have stripped xdebug:\n%s", b) + } + + if err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env}, + false, false, "", false); err != nil { + t.Fatalf("uninstall extension: %v", err) + } + // After uninstall, the ini is byte-for-byte the user's original (xdebug back). + got, err := os.ReadFile(loadedFile) + if err != nil { + t.Fatal(err) + } + if string(got) != original { + t.Errorf("ini not restored to original.\n got: %q\nwant: %q", got, original) + } +} + func TestUninstallNothing(t *testing.T) { env := linuxUserEnv(t.TempDir()) err := Uninstall(context.Background(), Options{Scope: platform.User, Env: &env}, diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index 448f466..2d2446f 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -64,6 +64,17 @@ type Extension struct { SoPath string `json:"soPath"` // installed .so/.dll path IniPath string `json:"iniPath"` // ini file holding the loader line InstalledAt time.Time `json:"installedAt"` + // ConfigBackups are existing ini files this install modified in place (e.g. to + // remove xdebug), backed up so uninstall can restore them to their original + // contents. + ConfigBackups []FileBackup `json:"configBackups,omitempty"` +} + +// FileBackup records a file that was modified in place, so its original contents +// can be restored on uninstall. +type FileBackup struct { + OriginalPath string `json:"originalPath"` // the file that was modified + BackupPath string `json:"backupPath"` // where its original contents were saved } // New returns an empty manifest for the given install root and bin directory. diff --git a/internal/php/parse_test.go b/internal/php/parse_test.go index 4b10252..3c5ee4a 100644 --- a/internal/php/parse_test.go +++ b/internal/php/parse_test.go @@ -6,12 +6,13 @@ import ( ) func TestParseKeyVals(t *testing.T) { - out := "version=8.3.10\nseries=8.3\nzts=0\nextension_dir=/usr/lib/php/20230831\n" + out := "version=8.3.10\nseries=8.3\nzts=0\nmachine=x86_64\nextension_dir=/usr/lib/php/20230831\n" kv := parseKeyVals(out) want := map[string]string{ "version": "8.3.10", "series": "8.3", "zts": "0", + "machine": "x86_64", "extension_dir": "/usr/lib/php/20230831", } if !reflect.DeepEqual(kv, want) { diff --git a/internal/php/php.go b/internal/php/php.go index 45060ab..523e458 100644 --- a/internal/php/php.go +++ b/internal/php/php.go @@ -36,14 +36,18 @@ type Info struct { Version string // full version, e.g. "8.3.10" Series string // major.minor, e.g. "8.3" ZTS bool // thread-safe build + Machine string // php_uname("m"): the arch the php process runs as ExtensionDir string // ini_get("extension_dir") Ini IniPaths } // infoScript prints the interpreter facts as key=value lines. Using -r keeps the -// output locale-independent (unlike parsing -v / -i prose). -const infoScript = `printf("version=%s\nseries=%s\nzts=%d\nextension_dir=%s\n",` + - ` PHP_VERSION, PHP_MAJOR_VERSION.".".PHP_MINOR_VERSION, PHP_ZTS, ini_get("extension_dir"));` +// output locale-independent (unlike parsing -v / -i prose). Machine comes from +// php_uname("m") — the architecture the php *process* runs as (e.g. "x86_64" for +// an Intel php running under Rosetta on Apple Silicon), which is what a matching +// extension must be built for. +const infoScript = `printf("version=%s\nseries=%s\nzts=%d\nmachine=%s\nextension_dir=%s\n",` + + ` PHP_VERSION, PHP_MAJOR_VERSION.".".PHP_MINOR_VERSION, PHP_ZTS, php_uname("m"), ini_get("extension_dir"));` // Detect returns the path to the php interpreter on PATH, or ErrNotFound. func Detect() (string, error) { @@ -96,6 +100,7 @@ func Query(ctx context.Context, binary string) (*Info, error) { info.Version = kv["version"] info.Series = kv["series"] info.ZTS = kv["zts"] == "1" + info.Machine = kv["machine"] info.ExtensionDir = kv["extension_dir"] iniOut, err := run(ctx, binary, "--ini") diff --git a/internal/platform/arch.go b/internal/platform/arch.go index abecdac..ed15ba9 100644 --- a/internal/platform/arch.go +++ b/internal/platform/arch.go @@ -1,11 +1,28 @@ package platform import ( + "fmt" "os/exec" "strconv" "strings" ) +// ArchFromMachine maps a `uname -m`-style machine name — as reported by +// php_uname("m") — to a release architecture token. This is the architecture the +// PHP *process* runs as, which is what its matching extension must be built for: +// an Intel php running under Rosetta 2 on Apple Silicon reports "x86_64", so it +// needs the x86_64 extension, not the host's native arm64 one. +func ArchFromMachine(machine string) (Arch, error) { + switch strings.ToLower(strings.TrimSpace(machine)) { + case "x86_64", "amd64", "x64": + return X8664, nil + case "arm64", "aarch64": + return Arm64, nil + default: + return "", fmt.Errorf("unrecognized machine architecture %q", machine) + } +} + // detectNativeArch upgrades a detected architecture to the host's *native* // architecture when they differ due to emulation. Concretely: an x86_64 build of // this tool running on Apple Silicon (under Rosetta 2) reports GOARCH=amd64, but diff --git a/internal/platform/arch_test.go b/internal/platform/arch_test.go index dae4623..33fdd74 100644 --- a/internal/platform/arch_test.go +++ b/internal/platform/arch_test.go @@ -64,3 +64,26 @@ func TestDetectNativeArch(t *testing.T) { }) } } + +func TestArchFromMachine(t *testing.T) { + ok := map[string]Arch{ + "x86_64": X8664, + "amd64": X8664, + "x64": X8664, + "X86_64": X8664, + " x86_64": X8664, + "arm64": Arm64, + "aarch64": Arm64, + "ARM64": Arm64, + } + for in, want := range ok { + if got, err := ArchFromMachine(in); err != nil || got != want { + t.Errorf("ArchFromMachine(%q) = %s, %v; want %s", in, got, err, want) + } + } + for _, in := range []string{"", "i386", "ppc64", "riscv64"} { + if _, err := ArchFromMachine(in); err == nil { + t.Errorf("ArchFromMachine(%q) should error", in) + } + } +} From bb8afb735e0ac4c77c90d4550a30c3c6bca13672 Mon Sep 17 00:00:00 2001 From: Carlos Granados Date: Mon, 27 Jul 2026 13:11:56 +0200 Subject: [PATCH 2/2] Improvements after Codex review --- README.md | 8 +- internal/installer/backup.go | 51 ++++- internal/installer/backup_test.go | 118 +++++++++++ internal/installer/extension.go | 62 +++++- internal/installer/extension_test.go | 233 +++++++++++++++++++++ internal/installer/iniconfig.go | 27 ++- internal/installer/install.go | 97 ++++++++- internal/installer/install_test.go | 297 +++++++++++++++++++++++++++ internal/installer/uninstall.go | 119 ++++++++--- internal/installer/uninstall_test.go | 251 ++++++++++++++++++++++ internal/installer/update.go | 4 + internal/installer/update_test.go | 43 ++++ 12 files changed, 1255 insertions(+), 55 deletions(-) create mode 100644 internal/installer/backup_test.go diff --git a/README.md b/README.md index b7e1094..4c9097a 100644 --- a/README.md +++ b/README.md @@ -21,10 +21,10 @@ Windows (PowerShell): powershell -c "irm https://github.com/php-debugger/installer/releases/latest/download/install.ps1 | iex" ``` -The script detects your OS/arch, downloads the right archive, and installs the -binary (into the current directory by default; set `INSTALL_DIR` to change it, or -`VERSION` to pin a release). Because it fetches with `curl`/`wget` rather than a -browser, the binary is **not quarantined**, so macOS Gatekeeper doesn't block it. +The script detects your OS/arch, downloads the latest archive, and installs the +binary (into the current directory by default; set `INSTALL_DIR` to change it). +Because it fetches with `curl`/`wget` rather than a browser, the binary is **not +quarantined**, so macOS Gatekeeper doesn't block it. ### Download a prebuilt binary diff --git a/internal/installer/backup.go b/internal/installer/backup.go index 59f1c8d..1ae765b 100644 --- a/internal/installer/backup.go +++ b/internal/installer/backup.go @@ -9,18 +9,32 @@ import ( // backupExisting moves the existing interpreter at srcPath into backupDir under a // unique name, preserving whatever it is (a real binary or a symlink). It falls // back to copy+remove if the move crosses filesystems. Returns the backup path. -func backupExisting(srcPath, backupDir, key string, nowNanos int64) (string, error) { +// +// The destination is reserved with CreateTemp so its name is guaranteed unique: +// one install can back up several files under the same key (e.g. a Windows php.exe +// and php.cmd sharing an active slot), and a deterministic or timestamp-based name +// could collide and let one backup silently overwrite another. The original's +// basename is kept in the name for traceability. +func backupExisting(srcPath, backupDir, key string) (string, error) { if err := os.MkdirAll(backupDir, 0o755); err != nil { return "", fmt.Errorf("creating backup dir: %w", err) } - dst := filepath.Join(backupDir, fmt.Sprintf("php-%s-%d", key, nowNanos)) + f, err := os.CreateTemp(backupDir, "php-"+key+"-*-"+filepath.Base(srcPath)) + if err != nil { + return "", fmt.Errorf("reserving backup path for %s: %w", srcPath, err) + } + dst := f.Name() + f.Close() + // Move the original into the reserved path (os.Rename atomically replaces the + // empty placeholder on both Unix and Windows). if err := os.Rename(srcPath, dst); err == nil { return dst, nil } - // Cross-device or other rename failure: copy the resolved binary, then remove - // the original. - if err := copyFile(srcPath, dst, 0o755); err != nil { + // Cross-device or other rename failure: copy into the reserved path, then remove + // the original. copyNode preserves a symlink as a symlink (matching rename). + if err := copyNode(srcPath, dst, 0o755); err != nil { + os.Remove(dst) return "", fmt.Errorf("backing up %s: %w", srcPath, err) } if err := os.Remove(srcPath); err != nil { @@ -30,7 +44,8 @@ func backupExisting(srcPath, backupDir, key string, nowNanos int64) (string, err return dst, nil } -// restoreBackup moves a backup back to its original path. +// restoreBackup moves a backup back to its original path, preserving a symlink as +// a symlink whether it moves (rename) or falls back to copy across filesystems. func restoreBackup(backupPath, originalPath string) error { if err := os.MkdirAll(filepath.Dir(originalPath), 0o755); err != nil { return err @@ -38,8 +53,30 @@ func restoreBackup(backupPath, originalPath string) error { if err := os.Rename(backupPath, originalPath); err == nil { return nil } - if err := copyFile(backupPath, originalPath, 0o755); err != nil { + if err := copyNode(backupPath, originalPath, 0o755); err != nil { return err } return os.Remove(backupPath) } + +// copyNode copies src to dst for the cross-filesystem move fallbacks. A symlink is +// recreated as a symlink (its target is not dereferenced); a regular file has its +// contents copied with perm. +func copyNode(src, dst string, perm os.FileMode) error { + fi, err := os.Lstat(src) + if err != nil { + return err + } + if fi.Mode()&os.ModeSymlink != 0 { + target, err := os.Readlink(src) + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { + return fmt.Errorf("creating %s: %w", filepath.Dir(dst), err) + } + _ = os.Remove(dst) // os.Symlink fails if dst already exists + return os.Symlink(target, dst) + } + return copyFile(src, dst, perm) +} diff --git a/internal/installer/backup_test.go b/internal/installer/backup_test.go new file mode 100644 index 0000000..5d71644 --- /dev/null +++ b/internal/installer/backup_test.go @@ -0,0 +1,118 @@ +package installer + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// copyNode backs the cross-filesystem move fallback. A symlink must survive as a +// symlink (target not dereferenced), matching the rename path it stands in for. +func TestCopyNodePreservesSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need privileges on Windows") + } + dir := t.TempDir() + realTarget := filepath.Join(dir, "real-php") + if err := os.WriteFile(realTarget, []byte("BINARY"), 0o755); err != nil { + t.Fatal(err) + } + src := filepath.Join(dir, "php") // symlink -> real-php + if err := os.Symlink(realTarget, src); err != nil { + t.Fatal(err) + } + + dst := filepath.Join(dir, "backup", "php") + if err := copyNode(src, dst, 0o755); err != nil { + t.Fatalf("copyNode: %v", err) + } + + fi, err := os.Lstat(dst) + if err != nil { + t.Fatalf("lstat dst: %v", err) + } + if fi.Mode()&os.ModeSymlink == 0 { + t.Fatal("dst should be a symlink, not a dereferenced regular file") + } + got, err := os.Readlink(dst) + if err != nil || got != realTarget { + t.Errorf("symlink target = %q (err %v), want %q", got, err, realTarget) + } +} + +func TestCopyNodeCopiesRegularFile(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "bin") + if err := os.WriteFile(src, []byte("CONTENTS"), 0o755); err != nil { + t.Fatal(err) + } + dst := filepath.Join(dir, "sub", "bin") + if err := copyNode(src, dst, 0o755); err != nil { + t.Fatalf("copyNode: %v", err) + } + if isLink, _ := isSymlinkNode(dst); isLink { + t.Error("regular file should not become a symlink") + } + if b, err := os.ReadFile(dst); err != nil || string(b) != "CONTENTS" { + t.Errorf("dst contents = %q (err %v), want CONTENTS", b, err) + } +} + +// Two files backed up under the same key in one install (e.g. a Windows php.exe +// and php.cmd) must get distinct backup paths — even sharing a basename — so one +// never overwrites the other. +func TestBackupExistingUniquePaths(t *testing.T) { + dir := t.TempDir() + backups := filepath.Join(dir, "backups") + + // Two distinct sources sharing a basename ("php"), backed up under one key. + srcA := filepath.Join(dir, "a", "php") + srcB := filepath.Join(dir, "b", "php") + if err := os.MkdirAll(filepath.Dir(srcA), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(srcB), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(srcA, []byte("AAA"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(srcB, []byte("BBB"), 0o755); err != nil { + t.Fatal(err) + } + + pa, err := backupExisting(srcA, backups, "8.3") + if err != nil { + t.Fatal(err) + } + pb, err := backupExisting(srcB, backups, "8.3") + if err != nil { + t.Fatal(err) + } + + if pa == pb { + t.Fatalf("backup paths collided: %q", pa) + } + if data, _ := os.ReadFile(pa); string(data) != "AAA" { + t.Errorf("backup A content = %q, want AAA", data) + } + if data, _ := os.ReadFile(pb); string(data) != "BBB" { + t.Errorf("backup B content = %q, want BBB", data) + } + // Both sources were moved into their backups. + if _, err := os.Stat(srcA); !os.IsNotExist(err) { + t.Error("srcA should have been moved") + } + if _, err := os.Stat(srcB); !os.IsNotExist(err) { + t.Error("srcB should have been moved") + } +} + +func isSymlinkNode(path string) (bool, error) { + fi, err := os.Lstat(path) + if err != nil { + return false, err + } + return fi.Mode()&os.ModeSymlink != 0, nil +} diff --git a/internal/installer/extension.go b/internal/installer/extension.go index 912a2e4..e292cbf 100644 --- a/internal/installer/extension.go +++ b/internal/installer/extension.go @@ -116,12 +116,22 @@ func InstallExtension(ctx context.Context, opts Options) error { rb := &rollback{} // --- copy the extension into extension_dir --- - soDst := filepath.Join(existing.ExtensionDir, asset.Name) + // Resolve a relative extension_dir to an absolute path. PHP may report one + // (notably Windows configs default to "ext"), which it resolves against the PHP + // installation directory — not the installer's working directory. Installing to + // the raw relative value would drop the .so under the current directory and + // record a CWD-relative loader path that only loads when php runs from here. + extDir := resolveExtensionDir(existing.ExtensionDir, path) + if extDir != existing.ExtensionDir { + opts.logf("Note: php reports a relative extension_dir (%q); installing into %s.", + existing.ExtensionDir, extDir) + } + soDst := filepath.Join(extDir, asset.Name) if err := registerFileRestore(soDst, rb, 0o755); err != nil { return err } if err := installFile(dlPath, soDst); err != nil { - return fmt.Errorf("installing extension into %s: %w", existing.ExtensionDir, err) + return fmt.Errorf("installing extension into %s: %w", extDir, err) } opts.logf(" copied %s", soDst) @@ -161,6 +171,13 @@ func InstallExtension(ctx context.Context, opts Options) error { return err } m.InstallRoot = layout.Root + // Preserve the user's original ini backups across reinstalls/updates. A re-run + // (e.g. `update`, which forces past the already-installed guard) typically finds + // xdebug already stripped, so stripXdebugFromExisting produces no new backups; + // the prior extension record still holds the true pre-strip originals, so carry + // those forward for any file we did not freshly back up this run — otherwise + // uninstall could no longer restore the user's xdebug configuration. + iniBackups = preserveBackups(m.Extension, iniBackups) m.SetExtension(manifest.Extension{ Series: existing.Series, PHPVersion: existing.Version, @@ -194,6 +211,47 @@ func stripXdebugFromExisting(existing *php.Info, backupDir string, rb *rollback, return sanitizeInPlace(files, backupDir, rb, opts) } +// preserveBackups merges the ini backups created this run (fresh) with those from +// a prior extension record (if any). A freshly-captured backup wins for its own +// file — it holds that file's pre-strip contents from this run. For files not +// touched this run (the common update case, where xdebug was already stripped at +// first install), the prior backup is kept, since it holds the user's true +// original contents. Returns fresh unchanged when there is nothing to preserve. +func preserveBackups(prior *manifest.Extension, fresh []manifest.FileBackup) []manifest.FileBackup { + if prior == nil || len(prior.ConfigBackups) == 0 { + return fresh + } + freshByPath := make(map[string]bool, len(fresh)) + for _, b := range fresh { + freshByPath[b.OriginalPath] = true + } + merged := append([]manifest.FileBackup(nil), fresh...) + for _, b := range prior.ConfigBackups { + if !freshByPath[b.OriginalPath] { + merged = append(merged, b) + } + } + return merged +} + +// resolveExtensionDir returns an absolute extension directory. PHP sometimes +// reports a relative extension_dir (notably Windows' default "ext"), which it +// resolves against the PHP installation directory rather than the current working +// directory. We mirror that by resolving it against the php binary's directory +// (following symlinks to the real install location), so the .so is installed +// alongside PHP's other extensions and the loader references it by an absolute +// path. An already-absolute dir is returned unchanged. +func resolveExtensionDir(extDir, phpPath string) string { + if filepath.IsAbs(extDir) { + return extDir + } + base := filepath.Dir(phpPath) + if resolved, err := filepath.EvalSymlinks(phpPath); err == nil { + base = filepath.Dir(resolved) + } + return filepath.Join(base, extDir) +} + // enableExtension writes a loader that enables the debugger extension: a // dedicated ini in the scan dir if there is one, otherwise appended to the main // php.ini. Returns the ini file path. Registers undo on rb. diff --git a/internal/installer/extension_test.go b/internal/installer/extension_test.go index 0368344..b963e7b 100644 --- a/internal/installer/extension_test.go +++ b/internal/installer/extension_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" "github.com/php-debugger/installer/internal/manifest" @@ -235,6 +236,238 @@ func TestInstallExtensionSkipsOwnInterpreter(t *testing.T) { } } +// mutableExtServer serves a release with a single extension asset whose tag can +// change between requests, to simulate a newer release becoming available. +type mutableExtServer struct { + mu sync.Mutex + tag string + URL string +} + +func newMutableExtServer(t *testing.T, tag string) *mutableExtServer { + t.Helper() + ms := &mutableExtServer{tag: tag} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ms.mu.Lock() + tag := ms.tag + ms.mu.Unlock() + switch { + case strings.HasSuffix(r.URL.Path, "/releases/latest"): + fmt.Fprintf(w, `{"tag_name":%q,"assets":[ + {"name":"php-debugger-php8.3-nts-linux-x86_64.so","browser_download_url":%q,"size":%d} + ]}`, tag, ms.URL+"/dl/ext", len(fakeSO)) + case r.URL.Path == "/dl/ext": + w.Write([]byte(fakeSO)) + default: + http.NotFound(w, r) + } + })) + ms.URL = srv.URL + t.Cleanup(srv.Close) + return ms +} + +func (ms *mutableExtServer) set(tag string) { + ms.mu.Lock() + ms.tag = tag + ms.mu.Unlock() +} + +// Regression: updating the extension must not lose the original ini backups. The +// update re-runs InstallExtension (forced), which finds xdebug already stripped +// and so produces no new backups; the manifest must retain the first install's +// backups so a later uninstall can still restore the user's xdebug config. +func TestUpdateExtensionPreservesXdebugBackup(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + extDir := t.TempDir() + iniDir := t.TempDir() + scanDir := filepath.Join(iniDir, "conf.d") + loadedFile := filepath.Join(iniDir, "php.ini") + original := "zend_extension=xdebug.so\nxdebug.mode=develop,debug\nmemory_limit=100M\n" + if err := os.WriteFile(loadedFile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + binDir := filepath.Join(t.TempDir(), "bin") + writeExec(t, filepath.Join(binDir, "php"), + fakeExistingPHPForExt("8.3.7", extDir, loadedFile, scanDir, true)) + t.Setenv("PATH", binDir) + + ms := newMutableExtServer(t, "1.0.0") + client := release.NewClient() + client.BaseURL = ms.URL + env := linuxUserEnv(home) + opts := Options{Scope: platform.User, AssumeYes: true, Out: &bytes.Buffer{}, Client: client, Env: &env} + + if err := InstallExtension(context.Background(), opts); err != nil { + t.Fatalf("install extension: %v", err) + } + manifestPath := filepath.Join(home, ".local", "share", "php-debugger", "manifest.json") + m, _ := manifest.Load(manifestPath) + if m.Extension == nil || len(m.Extension.ConfigBackups) == 0 { + t.Fatalf("first install should record ini backups, got %+v", m.Extension) + } + + // A newer release appears; update the extension. + ms.set("2.0.0") + var out bytes.Buffer + uo := opts + uo.Out = &out + if err := Update(context.Background(), uo, false, true); err != nil { + t.Fatalf("Update: %v\n%s", err, out.String()) + } + + // The backups must survive the update (bug: they were nil'd out). + m, _ = manifest.Load(manifestPath) + if m.Extension == nil || m.Extension.ReleaseTag != "2.0.0" { + t.Fatalf("extension not updated: %+v", m.Extension) + } + if len(m.Extension.ConfigBackups) == 0 { + t.Fatal("update lost the ini backups; uninstall could no longer restore xdebug") + } + + // End-to-end: uninstall after update must restore the user's original ini. + if err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env}, + false, false, "", false); err != nil { + t.Fatalf("uninstall: %v", err) + } + got, err := os.ReadFile(loadedFile) + if err != nil { + t.Fatal(err) + } + if string(got) != original { + t.Errorf("ini not restored after update+uninstall.\n got: %q\nwant: %q", got, original) + } +} + +// Regression: a php that reports a relative extension_dir (e.g. Windows' "ext") +// must have the .so installed under the PHP install dir, with an absolute loader +// path — not relative to the installer's working directory. +func TestInstallExtensionResolvesRelativeExtensionDir(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + t.Chdir(t.TempDir()) // contain (and make detectable) any CWD-relative write + home := t.TempDir() + scanDir := filepath.Join(t.TempDir(), "conf.d") + + binDir := filepath.Join(t.TempDir(), "bin") + // php reports a RELATIVE extension_dir ("ext"). + writeExec(t, filepath.Join(binDir, "php"), + fakeExistingPHPForExtArch("8.3.7", "x86_64", "ext", "", scanDir, true)) + t.Setenv("PATH", binDir) + + srv := newFakeReleaseServer(t, "unused") + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + var out bytes.Buffer + if err := InstallExtension(context.Background(), Options{ + Scope: platform.User, AssumeYes: true, Out: &out, Client: client, Env: &env, + }); err != nil { + t.Fatalf("InstallExtension: %v\n%s", err, out.String()) + } + + // The .so lands under the php install dir's ext/, addressed absolutely. + phpReal, err := filepath.EvalSymlinks(filepath.Join(binDir, "php")) + if err != nil { + t.Fatal(err) + } + wantSo := filepath.Join(filepath.Dir(phpReal), "ext", "php-debugger-php8.3-nts-linux-x86_64.so") + if _, err := os.Stat(wantSo); err != nil { + t.Errorf("extension not installed at resolved absolute path %s: %v", wantSo, err) + } + // Nothing should have been written relative to the working directory. + if _, err := os.Stat("ext"); err == nil { + t.Error("extension dir was created relative to CWD") + } + // The loader references the .so by that absolute path. + loader := filepath.Join(scanDir, "99-php-debugger.ini") + if lb, _ := os.ReadFile(loader); !strings.Contains(string(lb), "zend_extension="+wantSo) { + t.Errorf("loader should reference absolute .so path %s:\n%s", wantSo, lb) + } + // The manifest records the absolute .so path (so uninstall removes the right file). + m, _ := manifest.Load(filepath.Join(home, ".local", "share", "php-debugger", "manifest.json")) + if m.Extension == nil || m.Extension.SoPath != wantSo { + t.Errorf("manifest SoPath = %+v, want %s", m.Extension, wantSo) + } +} + +// Regression: a forced extension reinstall that creates a fresh ini backup and +// then rolls back (verification fails) must NOT delete the prior run's backup, +// which the persisted manifest still references. Unique backup paths guarantee the +// fresh backup can't collide with the old one. +func TestForcedReinstallRollbackKeepsPriorIniBackup(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + extDir := t.TempDir() + iniDir := t.TempDir() + scanDir := filepath.Join(iniDir, "conf.d") + loadedFile := filepath.Join(iniDir, "php.ini") + original := "zend_extension=xdebug.so\nxdebug.mode=develop,debug\nmemory_limit=100M\n" + if err := os.WriteFile(loadedFile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + phpPath := filepath.Join(t.TempDir(), "bin", "php") + writeExec(t, phpPath, fakeExistingPHPForExt("8.3.7", extDir, loadedFile, scanDir, true)) + t.Setenv("PATH", filepath.Dir(phpPath)) + + srv := newFakeReleaseServer(t, "unused") + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + opts := Options{Scope: platform.User, AssumeYes: true, Out: &bytes.Buffer{}, Client: client, Env: &env} + + // First install: strips xdebug and records backup B1 holding the original. + if err := InstallExtension(context.Background(), opts); err != nil { + t.Fatalf("install: %v", err) + } + manifestPath := filepath.Join(home, ".local", "share", "php-debugger", "manifest.json") + m, _ := manifest.Load(manifestPath) + if m.Extension == nil || len(m.Extension.ConfigBackups) == 0 { + t.Fatalf("expected a config backup, got %+v", m.Extension) + } + b1 := m.Extension.ConfigBackups[0].BackupPath + if data, err := os.ReadFile(b1); err != nil || !strings.Contains(string(data), "xdebug.so") { + t.Fatalf("B1 should hold the original xdebug ini: %q err=%v", data, err) + } + + // The user re-adds xdebug (so a forced reinstall strips again → a fresh backup), + // and the reinstall is made to FAIL at verification so it rolls back. + if err := os.WriteFile(loadedFile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + writeExec(t, phpPath, fakeExistingPHPForExt("8.3.7", extDir, loadedFile, scanDir, false)) + + fo := opts + fo.Force = true + if err := InstallExtension(context.Background(), fo); err == nil { + t.Fatal("expected forced reinstall to fail at verification (so it rolls back)") + } + + // The prior backup B1 must survive the rollback (unique paths ⇒ no collision). + if _, err := os.Stat(b1); err != nil { + t.Errorf("prior backup B1 was deleted by the rollback: %v", err) + } + // The persisted (unchanged) manifest still references B1 and can restore xdebug. + m2, _ := manifest.Load(manifestPath) + if m2.Extension == nil || len(m2.Extension.ConfigBackups) == 0 || + m2.Extension.ConfigBackups[0].BackupPath != b1 { + t.Fatalf("manifest should still reference B1: %+v", m2.Extension) + } + if err := restoreBackup(b1, loadedFile); err != nil { + t.Fatalf("restore from B1 failed: %v", err) + } + if got, _ := os.ReadFile(loadedFile); string(got) != original { + t.Errorf("restore did not reproduce the original ini:\n%s", got) + } +} + // newFakeExtReleaseServer serves a release whose extension assets exist for both // architectures of the given os token, so a test can assert which one is chosen. func newFakeExtReleaseServer(t *testing.T, osTok string) *httptest.Server { diff --git a/internal/installer/iniconfig.go b/internal/installer/iniconfig.go index 4eee1eb..0915c59 100644 --- a/internal/installer/iniconfig.go +++ b/internal/installer/iniconfig.go @@ -5,7 +5,6 @@ import ( "os" "path/filepath" "sort" - "strconv" "strings" "github.com/php-debugger/installer/internal/ini" @@ -75,7 +74,7 @@ func rewriteIniFiles(pairs []configPair, cfg iniRewriteOptions, rb *rollback, op // Persist the original contents (for uninstall) before overwriting. if cfg.backupDir != "" { - bpath, err := saveIniBackup(cfg.backupDir, pr.dst, data, len(backups)) + bpath, err := saveIniBackup(cfg.backupDir, pr.dst, data) if err != nil { return written, backups, err } @@ -99,16 +98,30 @@ func rewriteIniFiles(pairs []configPair, cfg iniRewriteOptions, rb *rollback, op } // saveIniBackup writes the original contents of originalPath into backupDir under -// a unique, traceable name, returning the backup path. -func saveIniBackup(backupDir, originalPath string, data []byte, index int) (string, error) { +// a unique, traceable name, returning the backup path. The name is made unique +// (via CreateTemp) rather than deterministic so a fresh backup never collides with +// one from a previous run: otherwise a forced reinstall could overwrite — and its +// rollback delete — a backup file the persisted manifest still points at, breaking +// a later uninstall restore. +func saveIniBackup(backupDir, originalPath string, data []byte) (string, error) { if err := os.MkdirAll(backupDir, 0o755); err != nil { return "", fmt.Errorf("creating backup dir: %w", err) } - bpath := filepath.Join(backupDir, "ini-"+strconv.Itoa(index)+"-"+filepath.Base(originalPath)) - if err := os.WriteFile(bpath, data, 0o644); err != nil { + // Pattern keeps the origin file's name visible: "ini--php.ini". + f, err := os.CreateTemp(backupDir, "ini-*-"+filepath.Base(originalPath)) + if err != nil { + return "", fmt.Errorf("backing up %s: %w", originalPath, err) + } + if _, err := f.Write(data); err != nil { + f.Close() + os.Remove(f.Name()) + return "", fmt.Errorf("backing up %s: %w", originalPath, err) + } + if err := f.Close(); err != nil { + os.Remove(f.Name()) return "", fmt.Errorf("backing up %s: %w", originalPath, err) } - return bpath, nil + return f.Name(), nil } // copyConfig copies the existing interpreter's ini files into the new diff --git a/internal/installer/install.go b/internal/installer/install.go index e1e32e6..22e442f 100644 --- a/internal/installer/install.go +++ b/internal/installer/install.go @@ -225,21 +225,39 @@ func InstallInterpreter(ctx context.Context, opts Options) error { } // --- back up and replace an existing interpreter at its location --- - var backup *manifest.Backup + var backups []manifest.Backup if replaceExisting { opts.logf("Backing up existing interpreter at %s ...", existing.Path) backupPath, err := backupExisting(existing.Path, layout.BackupsDir(), - platform.VersionDirName(series, opts.ZTS), time.Now().UnixNano()) + platform.VersionDirName(series, opts.ZTS)) if err != nil { rb.run() return fmt.Errorf("backing up existing interpreter; rolled back: %w", err) } origPath := existing.Path rb.add(func() error { return restoreBackup(backupPath, origPath) }) - backup = &manifest.Backup{OriginalPath: origPath, BackupPath: backupPath, CreatedAt: opts.clock()} + backups = append(backups, manifest.Backup{OriginalPath: origPath, BackupPath: backupPath, CreatedAt: opts.clock()}) maybeWarnManaged(opts, existing) } + // --- preserve any un-backed-up file at the activation path(s) --- + // Activate replaces whatever occupies the active `php` path, and on Windows it + // materializes one of php.exe / php.cmd and removes the other. Back up anything + // there we have not already preserved, so activation cannot destroy it + // irrecoverably — a foreign php off PATH, one whose query failed (clean-host + // fallback), a real sibling next to a replaced interpreter (a php.cmd beside the + // detected php.exe), or a user-created symlink pointing at some other php. This + // runs even after replacing a detected interpreter: that file is already moved + // aside, so it is skipped here. Only a symlink that is our own active entry + // (resolves into our install root) is skipped — everything else is preserved. + bs, err := preserveDestination(activeCandidatePaths(p.OS, linkDir), layout.BackupsDir(), + platform.VersionDirName(series, opts.ZTS), layout.Root, rb, opts) + if err != nil { + rb.run() + return fmt.Errorf("backing up interpreter at the activation path; rolled back: %w", err) + } + backups = append(backups, bs...) + // --- activate (symlink into the chosen bin dir) --- prevTarget, _, hadPrev := platform.ReadActive(linkDir, "php") activePath, kind, err := platform.Activate(linkDir, "php", binTarget) @@ -291,8 +309,8 @@ func InstallInterpreter(ctx context.Context, opts Options) error { ConfigFiles: configFiles, }) m.SetActive(key) - if backup != nil { - m.SetBackup(key, *backup) + for i, b := range backups { + m.SetBackup(backupKey(key, i), b) } if err := m.Save(layout.ManifestPath()); err != nil { rb.run() @@ -407,6 +425,75 @@ func isDir(p string) bool { return err == nil && fi.IsDir() } +// shouldPreserve reports whether the file at an activation candidate path must be +// backed up before activation overwrites it. A regular file always is (a foreign +// interpreter or shim). A symlink is preserved too — unless it is our own active +// entry, proven by resolving into our install root — because a user-created symlink +// to another php would otherwise be replaced with no way to restore it on +// uninstall. Missing paths and other node types (dirs, sockets) are left alone. +func shouldPreserve(path, root string) bool { + fi, err := os.Lstat(path) + if err != nil { + return false + } + if fi.Mode().IsRegular() { + return true + } + if fi.Mode()&os.ModeSymlink != 0 { + return !isOurInterpreter(path, root) + } + return false +} + +// activeCandidatePaths lists the paths where activation may materialize the active +// `php`, so the installer can preserve any real file it is about to replace. On +// Windows activation writes either a php.exe symlink or a php.cmd shim (removing +// the other), so both are candidates; elsewhere it is just php. It mirrors +// platform.Activate's naming but keys off the target OS (not runtime.GOOS) so the +// backup decision is made for the platform being installed to. +func activeCandidatePaths(osID platform.OS, binDir string) []string { + if osID == platform.Windows { + return []string{ + filepath.Join(binDir, "php.exe"), + filepath.Join(binDir, "php.cmd"), + } + } + return []string{filepath.Join(binDir, "php")} +} + +// preserveDestination backs up every candidate file that must not be lost to +// activation (see shouldPreserve), registering a rollback restore for each and +// returning the recorded backups. On Windows both php.exe and php.cmd can exist and +// activation clobbers/removes both forms, so both are preserved — not just the first +// found. root identifies our install so our own active symlink is not backed up. +func preserveDestination(candidates []string, backupDir, key, root string, rb *rollback, opts Options) ([]manifest.Backup, error) { + var backups []manifest.Backup + for _, p := range candidates { + if !shouldPreserve(p, root) { + continue + } + opts.logf("Backing up existing interpreter at %s ...", p) + backupPath, err := backupExisting(p, backupDir, key) + if err != nil { + return backups, err + } + orig := p + rb.add(func() error { return restoreBackup(backupPath, orig) }) + backups = append(backups, manifest.Backup{OriginalPath: orig, BackupPath: backupPath, CreatedAt: opts.clock()}) + } + return backups, nil +} + +// backupKey names a manifest backup entry. The first keeps the bare version key +// (the common single-backup case); extras get a suffix so multiple displaced files +// sharing one active slot (e.g. a Windows php.exe and php.cmd) coexist in the map. +func backupKey(versionKey string, index int) string { + if index == 0 { + return versionKey + } + return fmt.Sprintf("%s#%d", versionKey, index) +} + func threading(zts bool) string { if zts { return "zts" diff --git a/internal/installer/install_test.go b/internal/installer/install_test.go index dc7ae47..b2e12ec 100644 --- a/internal/installer/install_test.go +++ b/internal/installer/install_test.go @@ -140,6 +140,298 @@ func TestInstallInterpreterCleanHost(t *testing.T) { } } +// Regression: a real php sitting in the selected bin dir but not on PATH (so it +// is never detected) must be backed up before Activate overwrites it — otherwise +// installing would destroy a foreign interpreter with no way to restore it. +func TestInstallInterpreterBacksUpUndetectedInterpreterAtDest(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + isolatePATH(t) // nothing is detectable on PATH -> treated as a clean host + home := t.TempDir() + + // A real (non-symlink) php already lives in the scope bin dir, off PATH. + foreign := filepath.Join(home, ".local", "bin", "php") + const sentinel = "#!/bin/sh\necho REAL-FOREIGN-PHP\n" + writeExec(t, foreign, sentinel) + + srv := newFakeReleaseServer(t, fakePHP("8.3.7", true, "", "")) + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + if err := InstallInterpreter(context.Background(), Options{ + Scope: platform.User, Out: &bytes.Buffer{}, Client: client, Env: &env, PHPVersion: "8.3", + }); err != nil { + t.Fatalf("install: %v", err) + } + + // The destination is now our symlink, and the foreign binary is backed up. + if isLink, _ := platform.IsSymlink(foreign); !isLink { + t.Error("active php should be our symlink after install") + } + root := filepath.Join(home, ".local", "share", "php-debugger") + m, _ := manifest.Load(filepath.Join(root, "manifest.json")) + _, b, ok := anyBackup(m) + if !ok { + t.Fatal("the overwritten foreign php should have been backed up") + } + if b.OriginalPath != foreign { + t.Errorf("backup OriginalPath = %q, want %q", b.OriginalPath, foreign) + } + if data, err := os.ReadFile(b.BackupPath); err != nil || string(data) != sentinel { + t.Errorf("backup does not hold the original foreign php: %q err=%v", data, err) + } + + // Uninstall restores the foreign php byte-for-byte. + if err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env}, + false, false, "", false); err != nil { + t.Fatalf("uninstall: %v", err) + } + if isLink, _ := platform.IsSymlink(foreign); isLink { + t.Error("restored foreign php should be a real file, not a symlink") + } + if data, err := os.ReadFile(foreign); err != nil || string(data) != sentinel { + t.Errorf("foreign php not restored: %q err=%v", data, err) + } +} + +// End-to-end: a user-created symlink at the activation path pointing at another +// php (not detected on PATH) must be backed up on install and restored on +// uninstall — not silently replaced and lost. +func TestInstallInterpreterBacksUpForeignSymlinkAtDest(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + isolatePATH(t) // the foreign symlink is off PATH -> not detected + home := t.TempDir() + + // A user symlink in the scope bin dir pointing at some other php. + foreignPhp := filepath.Join(t.TempDir(), "other-php") + writeExec(t, foreignPhp, "#!/bin/sh\necho OTHER\n") + link := filepath.Join(home, ".local", "bin", "php") + if err := os.MkdirAll(filepath.Dir(link), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(foreignPhp, link); err != nil { + t.Fatal(err) + } + + srv := newFakeReleaseServer(t, fakePHP("8.3.7", true, "", "")) + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + if err := InstallInterpreter(context.Background(), Options{ + Scope: platform.User, Out: &bytes.Buffer{}, Client: client, Env: &env, PHPVersion: "8.3", + }); err != nil { + t.Fatalf("install: %v", err) + } + + // Our interpreter is now active, and the foreign symlink was backed up. + root := filepath.Join(home, ".local", "share", "php-debugger") + binTarget := filepath.Join(root, "8.3", "bin", "php") + if tgt, _ := os.Readlink(link); tgt != binTarget { + t.Errorf("active php should point at our interpreter, got %q", tgt) + } + m, _ := manifest.Load(filepath.Join(root, "manifest.json")) + _, b, ok := anyBackup(m) + if !ok { + t.Fatal("the foreign symlink should have been backed up") + } + if b.OriginalPath != link { + t.Errorf("backup OriginalPath = %q, want %q", b.OriginalPath, link) + } + + // Uninstall restores the user's foreign symlink, pointing at their php. + if err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env}, + false, false, "", false); err != nil { + t.Fatalf("uninstall: %v", err) + } + if isLink, _ := isSymlinkNode(link); !isLink { + t.Error("restored path should be a symlink, not our removed entry") + } + if tgt, err := os.Readlink(link); err != nil || tgt != foreignPhp { + t.Errorf("foreign symlink not restored: %q err=%v", tgt, err) + } +} + +// Windows activation may write php.cmd (shim) instead of php.exe, so both must be +// candidates for the destination backup — otherwise a real php.cmd is clobbered. +func TestActiveCandidatePaths(t *testing.T) { + unix := activeCandidatePaths(platform.Linux, "/bin") + if len(unix) != 1 || filepath.Base(unix[0]) != "php" { + t.Errorf("unix candidates = %v, want [/bin/php]", unix) + } + win := activeCandidatePaths(platform.Windows, `C:\bin`) + hasExe, hasCmd := false, false + for _, p := range win { + switch filepath.Base(p) { + case "php.exe": + hasExe = true + case "php.cmd": + hasCmd = true + } + } + if !hasExe || !hasCmd { + t.Errorf("windows candidates must include php.exe and php.cmd, got %v", win) + } +} + +// A real php.cmd shim at the destination (the case the Unix test cannot exercise) +// must be backed up before activation and restored on rollback. +func TestPreserveDestinationBacksUpShim(t *testing.T) { + dir := t.TempDir() + backups := filepath.Join(dir, "backups") + exe := filepath.Join(dir, "php.exe") // absent + cmd := filepath.Join(dir, "php.cmd") // a real, foreign shim + const shim = "@echo off\r\n\"C:\\real\\php.exe\" %*\r\n" + if err := os.WriteFile(cmd, []byte(shim), 0o755); err != nil { + t.Fatal(err) + } + + rb := &rollback{} + got, err := preserveDestination([]string{exe, cmd}, backups, "8.3", filepath.Join(dir, "root"), rb, Options{Out: &bytes.Buffer{}}) + if err != nil { + t.Fatalf("preserveDestination: %v", err) + } + if len(got) != 1 || got[0].OriginalPath != cmd { + t.Fatalf("expected only php.cmd to be backed up, got %+v", got) + } + b := got[0] + // The original is moved aside so activation can write freely. + if _, err := os.Stat(cmd); !os.IsNotExist(err) { + t.Error("php.cmd should have been moved to the backup") + } + if data, err := os.ReadFile(b.BackupPath); err != nil || string(data) != shim { + t.Errorf("backup does not hold the original shim: %q err=%v", data, err) + } + // Rollback restores it byte-for-byte. + rb.run() + if data, err := os.ReadFile(cmd); err != nil || string(data) != shim { + t.Errorf("php.cmd not restored on rollback: %q err=%v", data, err) + } +} + +// If both Windows activation forms (php.exe and php.cmd) are real files, BOTH must +// be backed up — activation clobbers one and removes the other, so backing up only +// the first would still lose one without recovery. +func TestPreserveDestinationBacksUpBothWindowsForms(t *testing.T) { + dir := t.TempDir() + backups := filepath.Join(dir, "backups") + exe := filepath.Join(dir, "php.exe") + cmd := filepath.Join(dir, "php.cmd") + if err := os.WriteFile(exe, []byte("EXE"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cmd, []byte("CMD"), 0o755); err != nil { + t.Fatal(err) + } + + rb := &rollback{} + got, err := preserveDestination([]string{exe, cmd}, backups, "8.3", filepath.Join(dir, "root"), rb, Options{Out: &bytes.Buffer{}}) + if err != nil { + t.Fatalf("preserveDestination: %v", err) + } + if len(got) != 2 { + t.Fatalf("both real files should be backed up, got %d: %+v", len(got), got) + } + + // Each original was moved aside into a backup holding its own contents. + want := map[string]string{"php.exe": "EXE", "php.cmd": "CMD"} + for _, b := range got { + name := filepath.Base(b.OriginalPath) + if data, err := os.ReadFile(b.BackupPath); err != nil || string(data) != want[name] { + t.Errorf("%s backup content = %q (err %v), want %q", name, data, err, want[name]) + } + if _, err := os.Stat(b.OriginalPath); !os.IsNotExist(err) { + t.Errorf("%s should have been moved aside", name) + } + } + + // Rollback restores both, byte-for-byte. + rb.run() + if data, _ := os.ReadFile(exe); string(data) != "EXE" { + t.Error("php.exe not restored on rollback") + } + if data, _ := os.ReadFile(cmd); string(data) != "CMD" { + t.Error("php.cmd not restored on rollback") + } +} + +// Our own active entry — a symlink resolving into the install root — is skipped: +// uninstall reconstructs it from the manifest, and it holds no user data. +func TestPreserveDestinationSkipsOwnSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need privileges on Windows") + } + dir := t.TempDir() + root := filepath.Join(dir, "share", "php-debugger") + target := filepath.Join(root, "8.3", "bin", "php") // under our install root + writeExec(t, target, fakePHP("8.3.7", true, "", "")) + link := filepath.Join(dir, "bin", "php") + if err := os.MkdirAll(filepath.Dir(link), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + + rb := &rollback{} + got, err := preserveDestination([]string{link}, filepath.Join(dir, "backups"), "8.3", root, rb, Options{Out: &bytes.Buffer{}}) + if err != nil { + t.Fatalf("preserveDestination: %v", err) + } + if len(got) != 0 { + t.Errorf("our own active symlink should not be backed up, got %+v", got) + } + if isLink, _ := isSymlinkNode(link); !isLink { + t.Error("our symlink should be left in place") + } +} + +// A foreign symlink (to a php outside our install root) that the user created at +// the activation path MUST be backed up — otherwise activation replaces it and +// uninstall cannot restore the user's original. +func TestPreserveDestinationBacksUpForeignSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlinks need privileges on Windows") + } + dir := t.TempDir() + root := filepath.Join(dir, "share", "php-debugger") // our install root (unrelated) + foreignTarget := filepath.Join(dir, "opt", "other-php") + writeExec(t, foreignTarget, "#!/bin/sh\necho OTHER\n") + link := filepath.Join(dir, "bin", "php") // user symlink -> foreign php + if err := os.MkdirAll(filepath.Dir(link), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(foreignTarget, link); err != nil { + t.Fatal(err) + } + + rb := &rollback{} + got, err := preserveDestination([]string{link}, filepath.Join(dir, "backups"), "8.3", root, rb, Options{Out: &bytes.Buffer{}}) + if err != nil { + t.Fatalf("preserveDestination: %v", err) + } + if len(got) != 1 || got[0].OriginalPath != link { + t.Fatalf("foreign symlink should be backed up, got %+v", got) + } + // It was moved aside (so activation can write) and the backup is still a symlink + // to the user's original target. + if _, err := os.Lstat(link); !os.IsNotExist(err) { + t.Error("foreign symlink should have been moved to the backup") + } + if tgt, err := os.Readlink(got[0].BackupPath); err != nil || tgt != foreignTarget { + t.Errorf("backup should preserve the symlink target: %q err=%v", tgt, err) + } + // Rollback restores the user's symlink pointing at their php. + rb.run() + if tgt, err := os.Readlink(link); err != nil || tgt != foreignTarget { + t.Errorf("foreign symlink not restored on rollback: %q err=%v", tgt, err) + } +} + func TestInstallInterpreterReplacesExisting(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("fake php is a /bin/sh script") @@ -202,6 +494,11 @@ func TestInstallInterpreterReplacesExisting(t *testing.T) { if !ok { t.Fatal("no backup recorded for replaced interpreter") } + // Exactly one backup: preserveDestination runs after the replace but must skip + // the already-moved detected interpreter rather than double-recording it. + if len(m.Backups) != 1 { + t.Errorf("expected exactly one backup, got %d: %v", len(m.Backups), m.Backups) + } if b.OriginalPath != link { t.Errorf("backup OriginalPath = %q, want %q", b.OriginalPath, link) } diff --git a/internal/installer/uninstall.go b/internal/installer/uninstall.go index fc676c6..c2eefff 100644 --- a/internal/installer/uninstall.go +++ b/internal/installer/uninstall.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "github.com/php-debugger/installer/internal/manifest" @@ -72,20 +73,14 @@ func uninstallInterpreter(opts Options, layout platform.Layout, m *manifest.Mani return fmt.Errorf("interpreter %q is not installed", key) } wasActive := m.Active() == key + m.RemoveInterpreter(key) // in-memory; also clears active if it was active - // Remove the copied config files (and prune the dirs they lived in if now - // empty), then the versioned directory. - for _, f := range it.ConfigFiles { - if err := removeIfExists(f); err != nil { - return fmt.Errorf("removing config %s: %w", f, err) - } - } - pruneEmptyConfigDirs(it.ConfigFiles) - if err := os.RemoveAll(it.Dir); err != nil { - return fmt.Errorf("removing %s: %w", it.Dir, err) - } - m.RemoveInterpreter(key) // also clears active if it was active - + // Re-point the active `php` (or restore a backed-up original) BEFORE deleting + // anything on disk. This is the step that can fail, so doing it first leaves the + // interpreter and manifest untouched — and fully recoverable — if it does. + // reassignActive works off the remaining variants (the key is already gone from + // the map) and does not depend on the directory removed below. + var consumedBackups []string if wasActive { binDir := m.BinDir if binDir == "" { @@ -93,50 +88,103 @@ func uninstallInterpreter(opts Options, layout platform.Layout, m *manifest.Mani binDir = filepath.Dir(it.Dir) } } - if err := reassignActive(opts, m, env, binDir); err != nil { + cb, err := reassignActive(opts, m, env, binDir) + if err != nil { return err } + consumedBackups = cb } + // Persist the consistent state (variant removed, active reassigned) before the + // destructive cleanup, so a later removal failure can only leave orphan files — + // never a manifest that points at an already-deleted interpreter directory. if err := finalizeManifest(layout, m); err != nil { return fmt.Errorf("saving manifest: %w", err) } + + // Only now that the manifest no longer references them, delete the backup files + // the restore copied into place. Deferring until after the commit keeps a failed + // save (or a partial restore) recoverable: the backups survive until the manifest + // that would need them for a retry is gone. + for _, p := range consumedBackups { + _ = removeIfExists(p) + } + + // Remove the copied config files (pruning now-empty dirs) and the versioned + // directory. The active `php` no longer points into it. + for _, f := range it.ConfigFiles { + if err := removeIfExists(f); err != nil { + return fmt.Errorf("removing config %s: %w", f, err) + } + } + pruneEmptyConfigDirs(it.ConfigFiles) + if err := os.RemoveAll(it.Dir); err != nil { + return fmt.Errorf("removing %s: %w", it.Dir, err) + } + opts.logf("Uninstalled interpreter php %s (%s).", it.Series, threading(it.ZTS)) return nil } // reassignActive decides what `php` points at after the active interpreter was -// removed: activate the highest remaining variant, or restore a backed-up -// original, or remove the symlink. -func reassignActive(opts Options, m *manifest.Manifest, env platform.Env, binDir string) error { +// removed: activate the highest remaining variant, or restore backed-up +// original(s), or remove the symlink. When it restores backups it returns their +// backup-file paths so the caller can delete them once the manifest is committed. +func reassignActive(opts Options, m *manifest.Manifest, env platform.Env, binDir string) ([]string, error) { if remaining := m.InterpreterKeys(); len(remaining) > 0 { newKey := highestKey(remaining) nit, _ := m.Interpreter(newKey) target := filepath.Join(nit.Dir, "bin", phpBinaryName(env.OS)) if _, _, err := platform.Activate(binDir, "php", target); err != nil { - return fmt.Errorf("activating php %s: %w", newKey, err) + return nil, fmt.Errorf("activating php %s: %w", newKey, err) } m.SetActive(newKey) opts.logf("Switched active php -> %s (%s).", nit.Series, threading(nit.ZTS)) - return nil + return nil, nil } - // No variants left: restore the displaced original if we have one. - if bkey, b, ok := anyBackup(m); ok { + // No variants left: restore every displaced original we backed up (a single + // active `php` can have two materializations on Windows — php.exe and php.cmd). + // Restores are COPIES that leave the backup files intact, so a partial failure is + // safely retryable: the on-disk manifest still references every backup and every + // backup file still exists, so a re-run restores them all again. The backup files + // are consumed only after the manifest commit (by the caller). Removing the active + // entry must precede the restores (a backup often targets that path); if a restore + // then fails and nothing has been restored yet, re-activate the entry we removed + // so the user is not left without a php. Once a restore has succeeded we do not + // re-activate — that would clobber the restored file (and on Windows drop its + // sibling), and the restored original already provides a working php. + if keys := backupKeys(m); len(keys) > 0 { + prevTarget, _, hadPrev := platform.ReadActive(binDir, "php") _ = platform.RemoveActive(binDir, "php") - if err := restoreBackup(b.BackupPath, b.OriginalPath); err != nil { - return fmt.Errorf("restoring backup to %s: %w", b.OriginalPath, err) + restoredAny := false + for _, bkey := range keys { + b, _ := m.Backup(bkey) + if err := copyNode(b.BackupPath, b.OriginalPath, 0o755); err != nil { + if !restoredAny && hadPrev { + _, _, _ = platform.Activate(binDir, "php", prevTarget) + } + return nil, fmt.Errorf("restoring backup to %s: %w", b.OriginalPath, err) + } + restoredAny = true } - m.RemoveBackup(bkey) - opts.logf("Restored the original interpreter at %s.", b.OriginalPath) - return nil + // All restored: drop the backups from the manifest and hand the backup files + // to the caller to delete after it persists this state. + var consumed []string + for _, bkey := range keys { + b, _ := m.Backup(bkey) + consumed = append(consumed, b.BackupPath) + m.RemoveBackup(bkey) + opts.logf("Restored the original interpreter at %s.", b.OriginalPath) + } + return consumed, nil } if err := platform.RemoveActive(binDir, "php"); err != nil { - return fmt.Errorf("removing active php: %w", err) + return nil, fmt.Errorf("removing active php: %w", err) } opts.logf("Removed the active php entry.") - return nil + return nil, nil } func uninstallExtension(opts Options, layout platform.Layout, m *manifest.Manifest) error { @@ -201,8 +249,7 @@ func removeExtensionLoader(iniPath, soPath string) error { return os.WriteFile(iniPath, []byte(result), 0o644) } -// anyBackup returns any backup recorded in the manifest (there is at most one -// meaningful one: the interpreter displaced when we first took over a location). +// anyBackup returns any backup recorded in the manifest, if one exists. func anyBackup(m *manifest.Manifest) (string, manifest.Backup, bool) { for k := range m.Backups { b, _ := m.Backup(k) @@ -211,6 +258,18 @@ func anyBackup(m *manifest.Manifest) (string, manifest.Backup, bool) { return "", manifest.Backup{}, false } +// backupKeys returns the recorded backup keys in a stable order (the bare version +// key sorts before its "#N" extras), so multiple displaced files are restored +// deterministically, primary first. +func backupKeys(m *manifest.Manifest) []string { + keys := make([]string, 0, len(m.Backups)) + for k := range m.Backups { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // highestKey returns the variant key with the highest PHP version. func highestKey(keys []string) string { best := keys[0] diff --git a/internal/installer/uninstall_test.go b/internal/installer/uninstall_test.go index 4488b24..ac9523e 100644 --- a/internal/installer/uninstall_test.go +++ b/internal/installer/uninstall_test.go @@ -87,6 +87,257 @@ func TestUninstallInterpreterRestoresBackup(t *testing.T) { } } +// Regression: if reassigning the active php (here, restoring a backup) fails, the +// uninstall must not have already deleted the interpreter's files — the manifest +// must still point at an interpreter directory that still exists, so the state is +// recoverable rather than dangling. +func TestUninstallInterpreterKeepsFilesWhenReassignFails(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + srv := newFakeReleaseServer(t, fakePHP("8.3.7", true, "", "")) + + // A pre-existing php so the install records a restorable backup. + existBin := filepath.Join(t.TempDir(), "bin") + existPhp := filepath.Join(existBin, "php") + existIni := t.TempDir() + writeExec(t, existPhp, existingPHPScript("8.2.9", filepath.Join(existIni, "php.ini"), + filepath.Join(existIni, "conf.d"), "")) + if err := os.WriteFile(filepath.Join(existIni, "php.ini"), []byte("memory_limit=64M\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", existBin) + + client := release.NewClient() + client.BaseURL = srv.URL + env := linuxUserEnv(home) + + if err := InstallInterpreter(context.Background(), Options{ + Scope: platform.User, AssumeYes: true, Out: &bytes.Buffer{}, Client: client, Env: &env, + }); err != nil { + t.Fatalf("install: %v", err) + } + root := filepath.Join(home, ".local", "share", "php-debugger") + manifestPath := filepath.Join(root, "manifest.json") + versionDir := filepath.Join(root, "8.3") + + // Sabotage the recorded backup so restoreBackup fails: point OriginalPath under + // a regular file, so MkdirAll of its parent errors out. + blocker := filepath.Join(t.TempDir(), "blocker") + if err := os.WriteFile(blocker, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + m, _ := manifest.Load(manifestPath) + b, _ := m.Backup("8.3") + b.OriginalPath = filepath.Join(blocker, "php") // parent is a file -> unrestoreable + m.SetBackup("8.3", b) + if err := m.Save(manifestPath); err != nil { + t.Fatal(err) + } + + // Uninstalling the active (only) variant must attempt the backup restore, fail, + // and leave everything intact. + err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env}, + false, false, "", false) + if err == nil { + t.Fatal("expected uninstall to fail when backup restore fails") + } + + // The version dir must NOT have been deleted... + if _, statErr := os.Stat(versionDir); statErr != nil { + t.Errorf("version dir should survive a failed reassignment: %v", statErr) + } + // ...and the on-disk manifest must still reference the interpreter. + m2, _ := manifest.Load(manifestPath) + if _, ok := m2.Interpreter("8.3"); !ok { + t.Error("manifest should still list the interpreter after a failed uninstall") + } + // ...and, crucially, the active `php` must still exist and point at the + // still-installed interpreter (a failed restore must not leave the user with no php). + link := filepath.Join(existBin, "php") + if _, statErr := os.Lstat(link); statErr != nil { + t.Errorf("active php should still exist after a failed uninstall: %v", statErr) + } + binTarget := filepath.Join(versionDir, "bin", "php") + if tgt, _ := os.Readlink(link); tgt != binTarget { + t.Errorf("active php -> %q, want %q", tgt, binTarget) + } + // And it should still run and report the debugger. + if has, err := phpHasDebugger(link); err != nil || !has { + t.Errorf("restored active php should run and report the debugger: has=%v err=%v", has, err) + } +} + +// Uninstalling the active interpreter must restore every displaced backup, not +// just one — a Windows active slot can have both a php.exe and a php.cmd backed up. +func TestUninstallRestoresMultipleBackups(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + root := filepath.Join(home, ".local", "share", "php-debugger") + versionDir := filepath.Join(root, "8.3") + writeExec(t, filepath.Join(versionDir, "bin", "php"), fakePHP("8.3.7", true, "", "")) + + backupsDir := filepath.Join(root, "backups") + if err := os.MkdirAll(backupsDir, 0o755); err != nil { + t.Fatal(err) + } + b1 := filepath.Join(backupsDir, "php-8.3-exe") + b2 := filepath.Join(backupsDir, "php-8.3-cmd") + if err := os.WriteFile(b1, []byte("EXE-ORIG"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(b2, []byte("CMD-ORIG"), 0o755); err != nil { + t.Fatal(err) + } + + origDir := filepath.Join(t.TempDir(), "bin") + orig1 := filepath.Join(origDir, "php.exe") + orig2 := filepath.Join(origDir, "php.cmd") + + m := manifest.New(root, origDir) + m.SetInterpreter("8.3", manifest.Interpreter{Series: "8.3", PHPVersion: "8.3.7", Dir: versionDir}) + m.SetActive("8.3") + m.SetBackup("8.3", manifest.Backup{OriginalPath: orig1, BackupPath: b1}) + m.SetBackup("8.3#1", manifest.Backup{OriginalPath: orig2, BackupPath: b2}) + if err := m.Save(filepath.Join(root, "manifest.json")); err != nil { + t.Fatal(err) + } + + env := linuxUserEnv(home) + if err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env}, + true, false, "", false); err != nil { + t.Fatalf("uninstall: %v", err) + } + + // Both displaced originals restored to their contents. + if data, err := os.ReadFile(orig1); err != nil || string(data) != "EXE-ORIG" { + t.Errorf("php.exe not restored: %q err=%v", data, err) + } + if data, err := os.ReadFile(orig2); err != nil || string(data) != "CMD-ORIG" { + t.Errorf("php.cmd not restored: %q err=%v", data, err) + } + // Backups consumed and version dir removed. + m2, _ := manifest.Load(filepath.Join(root, "manifest.json")) + if len(m2.Backups) != 0 { + t.Errorf("backups should be cleared after restore, got %v", m2.Backups) + } + if _, err := os.Stat(versionDir); !os.IsNotExist(err) { + t.Error("version dir should be removed") + } +} + +// Regression: with two backups (Windows php.exe + php.cmd), if the first restore +// succeeds and a later one fails, the uninstall must stay recoverable — no backup +// file consumed, the on-disk manifest unchanged — so a retry completes cleanly. +func TestUninstallMultiBackupPartialFailureIsRetryable(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + root := filepath.Join(home, ".local", "share", "php-debugger") + versionDir := filepath.Join(root, "8.3") + writeExec(t, filepath.Join(versionDir, "bin", "php"), fakePHP("8.3.7", true, "", "")) + + backupsDir := filepath.Join(root, "backups") + if err := os.MkdirAll(backupsDir, 0o755); err != nil { + t.Fatal(err) + } + b1 := filepath.Join(backupsDir, "php-8.3-exe") + b2 := filepath.Join(backupsDir, "php-8.3-cmd") + if err := os.WriteFile(b1, []byte("EXE-ORIG"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(b2, []byte("CMD-ORIG"), 0o755); err != nil { + t.Fatal(err) + } + + binDir := filepath.Join(t.TempDir(), "bin") + if err := os.MkdirAll(binDir, 0o755); err != nil { + t.Fatal(err) + } + orig1 := filepath.Join(binDir, "php.exe") // 8.3 -> restoreable + // 8.3#1's parent is a regular file, so restoring it fails until we fix it. + blocker := filepath.Join(t.TempDir(), "blk") + if err := os.WriteFile(blocker, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + orig2 := filepath.Join(blocker, "php.cmd") // 8.3#1 -> unrestoreable (for now) + + m := manifest.New(root, binDir) + m.SetInterpreter("8.3", manifest.Interpreter{Series: "8.3", PHPVersion: "8.3.7", Dir: versionDir}) + m.SetActive("8.3") + m.SetBackup("8.3", manifest.Backup{OriginalPath: orig1, BackupPath: b1}) + m.SetBackup("8.3#1", manifest.Backup{OriginalPath: orig2, BackupPath: b2}) + if err := m.Save(filepath.Join(root, "manifest.json")); err != nil { + t.Fatal(err) + } + + env := linuxUserEnv(home) + uOpts := Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env} + + // First attempt: 8.3 restores, 8.3#1 fails. + if err := Uninstall(context.Background(), uOpts, true, false, "", false); err == nil { + t.Fatal("expected uninstall to fail on the sabotaged second restore") + } + + // The first restore did happen (copy), proving we got past 8.3... + if data, err := os.ReadFile(orig1); err != nil || string(data) != "EXE-ORIG" { + t.Errorf("first backup should have been restored: %q err=%v", data, err) + } + // ...yet nothing was consumed: both backup files survive and the on-disk manifest + // still references both backups and the interpreter, so a retry can complete. + if _, err := os.Stat(b1); err != nil { + t.Errorf("backup b1 must not be consumed on partial failure: %v", err) + } + if _, err := os.Stat(b2); err != nil { + t.Errorf("backup b2 must survive: %v", err) + } + m2, _ := manifest.Load(filepath.Join(root, "manifest.json")) + if _, ok := m2.Interpreter("8.3"); !ok { + t.Error("manifest should still list the interpreter after a failed uninstall") + } + if len(m2.Backups) != 2 { + t.Errorf("manifest should still reference both backups, got %v", m2.Backups) + } + if _, err := os.Stat(versionDir); err != nil { + t.Errorf("version dir should survive a failed uninstall: %v", err) + } + + // Remove the obstruction and retry: now it completes. + if err := os.Remove(blocker); err != nil { + t.Fatal(err) + } + if err := Uninstall(context.Background(), uOpts, true, false, "", false); err != nil { + t.Fatalf("retry uninstall: %v", err) + } + if data, err := os.ReadFile(orig1); err != nil || string(data) != "EXE-ORIG" { + t.Errorf("php.exe not restored on retry: %q err=%v", data, err) + } + if data, err := os.ReadFile(orig2); err != nil || string(data) != "CMD-ORIG" { + t.Errorf("php.cmd not restored on retry: %q err=%v", data, err) + } + m3, _ := manifest.Load(filepath.Join(root, "manifest.json")) + if len(m3.Backups) != 0 { + t.Errorf("backups should be cleared after retry, got %v", m3.Backups) + } + if _, ok := m3.Interpreter("8.3"); ok { + t.Error("interpreter should be gone after a successful retry") + } + if _, err := os.Stat(versionDir); !os.IsNotExist(err) { + t.Error("version dir should be removed after a successful retry") + } + // Backup files are consumed only after the successful commit. + if _, err := os.Stat(b1); !os.IsNotExist(err) { + t.Error("b1 should be deleted after a successful uninstall") + } + if _, err := os.Stat(b2); !os.IsNotExist(err) { + t.Error("b2 should be deleted after a successful uninstall") + } +} + func TestUninstallActiveReassignsToOtherVariant(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("fake php is a /bin/sh script") diff --git a/internal/installer/update.go b/internal/installer/update.go index 58f9f52..534c019 100644 --- a/internal/installer/update.go +++ b/internal/installer/update.go @@ -90,6 +90,10 @@ func updateInterpreter(ctx context.Context, opts Options, m *manifest.Manifest, io.BinDir = m.BinDir io.Client = client io.preloadedRelease = rel + // Force past the "already provided" short-circuit: the active php is our own + // interpreter for this series, so alreadyProvided would otherwise skip the + // reinstall (it does not compare release tags) and leave the old binary. + io.Force = true return InstallInterpreter(ctx, io) } diff --git a/internal/installer/update_test.go b/internal/installer/update_test.go index b9df95c..6a67e8d 100644 --- a/internal/installer/update_test.go +++ b/internal/installer/update_test.go @@ -98,6 +98,49 @@ func TestUpdateInterpreter(t *testing.T) { } } +// Regression: when the installer's own interpreter is active and on PATH, update +// must still reinstall against a newer release. The "already provided" check in +// InstallInterpreter matches on series+threading (not release tag), so update has +// to force past it — otherwise it silently no-ops and leaves the old binary/tag. +func TestUpdateInterpreterActiveOnPATH(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake php is a /bin/sh script") + } + home := t.TempDir() + // Put the install's bin dir on PATH so the active php is discoverable, the way + // it is after a real install (unlike the isolated-PATH tests above). + binDir := filepath.Join(home, ".local", "bin") + t.Setenv("PATH", binDir) + + ms := newMutableServer(t, "1.0.0", fakePHP("8.3.7", true, "", "")) + opts, manifestPath := installBaseInterpreter(t, home, ms) + + // Sanity: the active php is now on PATH and reports the debugger. + if has, err := phpHasDebugger(filepath.Join(binDir, "php")); err != nil || !has { + t.Fatalf("active php not usable on PATH: has=%v err=%v", has, err) + } + + // A newer release appears. + ms.set("2.0.0", fakePHP("8.3.9", true, "", "")) + + var out bytes.Buffer + uo := opts + uo.Out = &out + uo.PHPVersion = "" + if err := Update(context.Background(), uo, false, false); err != nil { + t.Fatalf("Update: %v\n%s", err, out.String()) + } + + m, _ := manifest.Load(manifestPath) + it, _ := m.Interpreter("8.3") + if it.ReleaseTag != "2.0.0" { + t.Errorf("ReleaseTag = %q, want 2.0.0 (update no-opped past the already-provided check)", it.ReleaseTag) + } + if it.PHPVersion != "8.3.9" { + t.Errorf("PHPVersion = %q, want 8.3.9", it.PHPVersion) + } +} + func TestUpdateAlreadyLatest(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("fake php is a /bin/sh script")