Improvements after review (#17)

Author: carlos-granados

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
 

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), ",") {

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

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,16 +44,39 @@ 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
 	}
 	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)
+}

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
+}