Updates after testing

README.md

@@ -92,8 +92,8 @@ php-debugger uninstall
 | --- | --- |
 | `install` | Install the debugger **interpreter** (default) or the **extension** (`-e`). |
 | `switch <version>` | Make an installed version active, installing it first if needed. |
-| `update` | Reinstall the active interpreter and/or extension against the latest release. |
-| `uninstall [version]` | Remove an interpreter (or the extension), restoring any backup. |
+| `update` | Reinstall whatever is installed (interpreter or extension) against the latest release. |
+| `uninstall [version]` | Remove the installed debugger (interpreter or extension), restoring any backup. |
 
 ### Flags
 
@@ -109,10 +109,14 @@ Global:
 - `-z, --zts` — thread-safe build (default: non-thread-safe; interpreter only).
 - `-e, --extension-only` — install only the extension into the current php.
 
-`update` / `uninstall`:
+`update` takes no flags — it updates whatever is installed (interpreter or
+extension), detected automatically.
 
-- `-e, --extension` / `-i, --interpreter` — pick the target when both are installed.
-- `uninstall` also takes an optional `<version>` and `-z, --zts` to target a variant.
+`uninstall`:
+
+- optional `<version>` and `-z, --zts` to target a specific interpreter variant. The
+  kind (interpreter or extension) is detected automatically — the two are never
+  installed at once, since installing the interpreter removes any extension.
 
 ## Install locations
 

internal/cli/uninstall.go

@@ -8,11 +8,6 @@ import (
 
 // uninstallOptions holds flags specific to the uninstall command.
 type uninstallOptions struct {
-	// Extension uninstalls the debugger extension.
-	Extension bool
-	// Interpreter uninstalls a debugger interpreter (optionally a specific
-	// version given as a positional argument).
-	Interpreter bool
 	// ZTS selects the thread-safe variant when a version is given.
 	ZTS bool
 }
@@ -22,10 +17,11 @@ func newUninstallCmd() *cobra.Command {
 
 	cmd := &cobra.Command{
 		Use:   "uninstall [version]",
-		Short: "Uninstall the interpreter or extension, restoring any backup",
-		Long: "Remove a debugger interpreter (optionally a specific version) or the\n" +
-			"debugger extension. If a backup of a previously replaced interpreter exists,\n" +
-			"it is restored.",
+		Short: "Uninstall the debugger, restoring any backup",
+		Long: "Remove the installed debugger. The interpreter and the extension are never\n" +
+			"installed at once, so the kind is detected automatically; pass a version to\n" +
+			"select a specific installed interpreter variant. If a backup of a previously\n" +
+			"replaced interpreter exists, it is restored.",
 		Args: cobra.MaximumNArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			var version string
@@ -37,14 +33,11 @@ func newUninstallCmd() *cobra.Command {
 				AssumeYes: globalOpts.Yes,
 				Out:       cmd.OutOrStdout(),
 				In:        cmd.InOrStdin(),
-			}, opts.Interpreter, opts.Extension, version, opts.ZTS)
+			}, version, opts.ZTS)
 		},
 	}
 
-	f := cmd.Flags()
-	f.BoolVarP(&opts.Extension, "extension", "e", false, "uninstall the debugger extension")
-	f.BoolVarP(&opts.Interpreter, "interpreter", "i", false, "uninstall the debugger interpreter")
-	f.BoolVarP(&opts.ZTS, "zts", "z", false, "select the thread-safe (ZTS) variant of the given version")
+	cmd.Flags().BoolVarP(&opts.ZTS, "zts", "z", false, "select the thread-safe (ZTS) variant of the given version")
 
 	return cmd
 }

internal/cli/update.go

@@ -6,37 +6,23 @@ import (
 	"github.com/spf13/cobra"
 )
 
-// updateOptions holds flags specific to the update command.
-type updateOptions struct {
-	// Extension updates the installed debugger extension.
-	Extension bool
-	// Interpreter updates the active debugger interpreter.
-	Interpreter bool
-}
-
 func newUpdateCmd() *cobra.Command {
-	opts := &updateOptions{}
-
 	cmd := &cobra.Command{
 		Use:   "update",
-		Short: "Update the installed interpreter or extension to the latest release",
-		Long: "Re-install the currently active interpreter (or the extension) against the\n" +
-			"latest release. Use --extension or --interpreter to disambiguate when both\n" +
-			"are installed.",
+		Short: "Update the installed debugger to the latest release",
+		Long: "Re-install whatever is installed — the active interpreter or the extension —\n" +
+			"against the latest release. The two are never installed at once, so the kind\n" +
+			"is detected automatically.",
 		Args: cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return installer.Update(cmd.Context(), installer.Options{
 				Scope:     platform.ScopeFromUserFlag(globalOpts.User),
 				AssumeYes: globalOpts.Yes,
 				Out:       cmd.OutOrStdout(),
 				In:        cmd.InOrStdin(),
-			}, opts.Interpreter, opts.Extension)
+			})
 		},
 	}
 
-	f := cmd.Flags()
-	f.BoolVarP(&opts.Extension, "extension", "e", false, "update the debugger extension")
-	f.BoolVarP(&opts.Interpreter, "interpreter", "i", false, "update the debugger interpreter")
-
 	return cmd
 }

internal/installer/backup.go

@@ -46,19 +46,41 @@ func backupExisting(srcPath, backupDir, key string) (string, error) {
 
 // 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.
+// The restored file keeps the backup's own permissions (which mirror the original
+// file's mode — see backupExisting/saveIniBackup), so a rename and the copy
+// fallback yield the same mode.
 func restoreBackup(backupPath, originalPath string) error {
 	if err := os.MkdirAll(filepath.Dir(originalPath), 0o755); err != nil {
 		return err
 	}
 	if err := os.Rename(backupPath, originalPath); err == nil {
 		return nil
 	}
-	if err := copyNode(backupPath, originalPath, 0o755); err != nil {
+	fi, err := os.Lstat(backupPath)
+	if err != nil {
+		return err
+	}
+	if err := copyNode(backupPath, originalPath, fi.Mode().Perm()); err != nil {
 		return err
 	}
 	return os.Remove(backupPath)
 }
 
+// copyBackup restores a backup to its original path by COPYING it (the backup file
+// is left in place, so the operation is retryable until a caller deletes it),
+// preserving the backup's own permissions (which mirror the original file's mode —
+// see saveIniBackup). A symlink is recreated as a symlink.
+func copyBackup(backupPath, originalPath string) error {
+	fi, err := os.Lstat(backupPath)
+	if err != nil {
+		return err
+	}
+	if err := os.MkdirAll(filepath.Dir(originalPath), 0o755); err != nil {
+		return err
+	}
+	return copyNode(backupPath, originalPath, fi.Mode().Perm())
+}
+
 // 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.

internal/installer/cleanup_test.go

@@ -72,7 +72,7 @@ func TestUninstallRemovesEmptyRoot(t *testing.T) {
 	}
 
 	if err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env},
-		false, false, "", false); err != nil {
+		"", false); err != nil {
 		t.Fatalf("uninstall: %v", err)
 	}
 	if _, err := os.Stat(root); !os.IsNotExist(err) {

internal/installer/extension.go

@@ -218,20 +218,10 @@ func stripXdebugFromExisting(existing *php.Info, backupDir string, rb *rollback,
 // 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 {
+	if prior == nil {
 		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
+	return mergeFileBackups(fresh, prior.ConfigBackups)
 }
 
 // resolveExtensionDir returns an absolute extension directory. PHP sometimes

internal/installer/extension_test.go

@@ -236,6 +236,76 @@ func TestInstallExtensionSkipsOwnInterpreter(t *testing.T) {
 	}
 }
 
+// Regression: installing the interpreter after the extension must remove the
+// extension (the interpreter has the debugger built in), so the two are never both
+// recorded. Otherwise uninstall would wrongly report an ambiguity ("both an
+// interpreter and an extension are installed").
+func TestInstallInterpreterRemovesInstalledExtension(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")
+	const originalIni = "zend_extension=xdebug.so\nmemory_limit=100M\n"
+	if err := os.WriteFile(loadedFile, []byte(originalIni), 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, fakePHP("8.3.7", true, "", ""))
+	client := release.NewClient()
+	client.BaseURL = srv.URL
+	env := linuxUserEnv(home)
+	opts := Options{Scope: platform.User, AssumeYes: true, Out: &bytes.Buffer{}, Client: client, Env: &env}
+	manifestPath := filepath.Join(home, ".local", "share", "php-debugger", "manifest.json")
+
+	// 1. Install the extension.
+	if err := InstallExtension(context.Background(), opts); err != nil {
+		t.Fatalf("install extension: %v", err)
+	}
+	m, _ := manifest.Load(manifestPath)
+	if m.Extension == nil {
+		t.Fatal("extension should be recorded after install")
+	}
+	soDst := m.Extension.SoPath
+
+	// 2. Install the interpreter -> must remove the extension.
+	io := opts
+	io.PHPVersion = "8.3"
+	if err := InstallInterpreter(context.Background(), io); err != nil {
+		t.Fatalf("install interpreter: %v", err)
+	}
+	m, _ = manifest.Load(manifestPath)
+	if m.Extension != nil {
+		t.Error("interpreter install must remove the extension record")
+	}
+	if _, ok := m.Interpreter("8.3"); !ok {
+		t.Error("interpreter should be recorded")
+	}
+	if _, err := os.Stat(soDst); !os.IsNotExist(err) {
+		t.Errorf("extension .so should have been removed, stat err=%v", err)
+	}
+	// Reverting the extension restored the user's original xdebug config.
+	if got, _ := os.ReadFile(loadedFile); !strings.Contains(string(got), "zend_extension=xdebug.so") {
+		t.Errorf("extension removal should restore the original xdebug ini, got %q", got)
+	}
+
+	// 3. Uninstall must work with no kind ambiguity (the real bug).
+	if err := Uninstall(context.Background(), opts, "", false); err != nil {
+		t.Fatalf("uninstall after install interpreter should not error, got: %v", err)
+	}
+	if _, err := os.Stat(manifestPath); !os.IsNotExist(err) {
+		t.Errorf("full uninstall should remove the manifest, stat err=%v", err)
+	}
+}
+
 // 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 {
@@ -315,7 +385,7 @@ func TestUpdateExtensionPreservesXdebugBackup(t *testing.T) {
 	var out bytes.Buffer
 	uo := opts
 	uo.Out = &out
-	if err := Update(context.Background(), uo, false, true); err != nil {
+	if err := Update(context.Background(), uo); err != nil {
 		t.Fatalf("Update: %v\n%s", err, out.String())
 	}
 
@@ -330,7 +400,7 @@ func TestUpdateExtensionPreservesXdebugBackup(t *testing.T) {
 
 	// 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 {
+		"", false); err != nil {
 		t.Fatalf("uninstall: %v", err)
 	}
 	got, err := os.ReadFile(loadedFile)
@@ -342,6 +412,64 @@ func TestUpdateExtensionPreservesXdebugBackup(t *testing.T) {
 	}
 }
 
+// Regression: uninstalling the extension must restore the modified ini file with
+// its original permissions. The backup is created via os.CreateTemp (mode 0600),
+// so a naive restore (rename/copy) clamped a world-readable php.ini down to
+// owner-only, and php running as another user (e.g. php-fpm as www-data) could no
+// longer read it.
+func TestUninstallExtensionRestoresIniPermissions(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")
+	const origMode os.FileMode = 0o644
+	if err := os.WriteFile(loadedFile,
+		[]byte("zend_extension=xdebug.so\nmemory_limit=100M\n"), origMode); err != nil {
+		t.Fatal(err)
+	}
+	// os.WriteFile is subject to umask; force the exact mode we assert on.
+	if err := os.Chmod(loadedFile, origMode); 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)
+	opts := Options{Scope: platform.User, AssumeYes: true, Out: &bytes.Buffer{}, Client: client, Env: &env}
+
+	if err := InstallExtension(context.Background(), opts); err != nil {
+		t.Fatalf("InstallExtension: %v", err)
+	}
+	// Sanity: install backed up the ini (there was xdebug to strip).
+	m, _ := manifest.Load(filepath.Join(home, ".local", "share", "php-debugger", "manifest.json"))
+	if m.Extension == nil || len(m.Extension.ConfigBackups) == 0 {
+		t.Fatalf("install should record an ini backup, got %+v", m.Extension)
+	}
+
+	if err := Uninstall(context.Background(), Options{Scope: platform.User, Out: &bytes.Buffer{}, Env: &env},
+		"", false); err != nil {
+		t.Fatalf("uninstall: %v", err)
+	}
+
+	fi, err := os.Stat(loadedFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := fi.Mode().Perm(); got != origMode {
+		t.Errorf("restored ini mode = %o, want %o (php as another user could not read it)", got, origMode)
+	}
+}
+
 // 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.