Remove embedded extension config, make --extensions-dir required (Phase 5)

AGENTS.md

@@ -274,8 +274,8 @@ R sub-dep data.json files live under `/tmp/r-sub-deps/source-{pkg}-latest/data.j
 | `internal/stack/stack.go` | `Stack` struct, `Load(stacksDir, name)` |
 | `internal/recipe/recipe.go` | `Recipe` interface, `Registry` |
 | `internal/recipe/php.go` | `PHPRecipe`; dispatches to `Load` or `LoadFromDir` based on `ExtensionsDir` |
-| `internal/php/extensions.go` | `Load`, `LoadFromDir`, `applyPatch` — extension YAML loading + merge |
-| `internal/php/assets/` | Extension YAML files (base + per-version patches) |
+| `internal/php/extensions.go` | `LoadFromDir`, `applyPatch` — extension YAML loading + merge; `--extensions-dir` is required |
+| `internal/php/testdata/` | Extension YAML files (base + per-version patches) — test fixtures only, not embedded |
 | `internal/archive/archive.go` | `StripTopLevelDir`, `StripFiles`, `InjectFile` |
 | `internal/portile/` | configure/make/install abstraction |
 | `internal/apt/` | apt-get install wrapper |

internal/php/extensions.go

@@ -2,114 +2,37 @@
 //
 // # Extension data files
 //
-// The YAML files that define which PHP extensions and native modules are built
-// live in assets/ alongside the Go source:
+// Extension YAML files define which PHP extensions and native modules are compiled
+// into PHP binaries. In production, these files are owned by the php-buildpack
+// repository (config/extensions/) and passed to binary-builder at build time via
+// the --extensions-dir CLI flag.
 //
-//	internal/php/assets/
-//	  php8-base-extensions.yml      — full list for all PHP 8.x (native modules + PECL extensions)
-//	  php81-extensions-patch.yml    — overrides/exclusions specific to PHP 8.1.x
-//	  php82-extensions-patch.yml    — overrides/exclusions specific to PHP 8.2.x
-//	  php83-extensions-patch.yml    — overrides/exclusions specific to PHP 8.3.x
+// The testdata/ directory in this package contains a copy of the extension YAML
+// files for use in unit tests. These are NOT compiled into the binary.
 //
 // # Loading extensions
 //
-// There are two ways to load extensions:
+// [LoadFromDir] is the primary API. It reads base + patch YAML files from a
+// directory on the filesystem and returns a merged [ExtensionSet].
 //
-//   - [Load] reads from the YAML files embedded in this package at compile time.
-//     This is the default behavior when binary-builder runs without --extensions-dir.
+// [Load] is a convenience wrapper for tests that delegates to LoadFromDir
+// using the testdata/ directory relative to the caller's working directory.
+// It should NOT be used in production code paths.
 //
-//   - [LoadFromDir] reads from a directory on the filesystem, using the same
-//     naming convention as the embedded assets. This allows the extension
-//     configuration to be owned by a different repository (e.g. the php-buildpack)
-//     and passed to binary-builder at build time via the --extensions-dir CLI flag.
-//
-// Both functions return an [ExtensionSet] after applying base + patch merging.
-//
-// # Adding a new PHP minor version (e.g. 8.4)
-//
-//  1. Create assets/php84-extensions-patch.yml with any additions or exclusions
-//     relative to the PHP 8 base (an empty patch is valid: "---\nextensions:\n").
-//
-// No code changes are required — the file is discovered automatically via the
-// embedded FS glob.
-//
-// # Adding a new PHP major version (e.g. 9)
-//
-//  1. Create assets/php9-base-extensions.yml with the full extension list for PHP 9.x.
-//  2. Create a patch file for each shipped minor version (e.g. assets/php90-extensions-patch.yml).
-//
-// Again, no code changes are required.
-//
-// # File naming convention (drives auto-discovery)
+// # File naming convention
 //
 //   - Base files:  php{major}-base-extensions.yml   (e.g. php8-base-extensions.yml)
 //   - Patch files: php{major}{minor}-extensions-patch.yml (e.g. php84-extensions-patch.yml)
-//
-// Important: only .yml files are embedded (the glob is assets/*.yml). A file with a
-// .yaml extension would be silently ignored.
 package php
 
 import (
-	"embed"
 	"fmt"
 	"os"
 	"path/filepath"
-	"regexp"
-	"strings"
 
 	"gopkg.in/yaml.v3"
 )
 
-//go:embed assets/*.yml
-var assetsFS embed.FS
-
-var (
-	// baseFileRE matches e.g. "assets/php8-base-extensions.yml" and captures the major version ("8").
-	baseFileRE = regexp.MustCompile(`^assets/php(\d+)-base-extensions\.yml$`)
-	// patchFileRE matches e.g. "assets/php83-extensions-patch.yml" and captures major+minor ("83").
-	// The \d{2,} quantifier is intentional: patch filenames must encode both the major AND minor
-	// digit together (e.g. "php81", "php90"), never a bare major digit. This prevents a file named
-	// "php8-extensions-patch.yml" from matching (that would be a base file), and ensures a future
-	// PHP 9 patch is named "php90-extensions-patch.yml", not "php9-extensions-patch.yml" (which
-	// would be silently ignored).
-	patchFileRE = regexp.MustCompile(`^assets/php(\d{2,})-extensions-patch\.yml$`)
-)
-
-// embeddedBases maps PHP major version (e.g. "8") → base YAML bytes.
-// embeddedPatches maps PHP major+minor (e.g. "83") → patch YAML bytes.
-// Both are populated once at init() from the embedded FS.
-var (
-	embeddedBases   map[string][]byte
-	embeddedPatches map[string][]byte
-)
-
-func init() {
-	embeddedBases = make(map[string][]byte)
-	embeddedPatches = make(map[string][]byte)
-
-	entries, err := assetsFS.ReadDir("assets")
-	if err != nil {
-		panic("php/extensions: cannot read embedded assets: " + err.Error())
-	}
-
-	for _, e := range entries {
-		if e.IsDir() || !strings.HasSuffix(e.Name(), ".yml") {
-			continue
-		}
-		path := "assets/" + e.Name()
-		data, err := assetsFS.ReadFile(path)
-		if err != nil {
-			panic("php/extensions: cannot read embedded file " + path + ": " + err.Error())
-		}
-
-		if m := baseFileRE.FindStringSubmatch(path); m != nil {
-			embeddedBases[m[1]] = data
-		} else if m := patchFileRE.FindStringSubmatch(path); m != nil {
-			embeddedPatches[m[1]] = data
-		}
-	}
-}
-
 // Extension represents a single PHP extension or native module.
 type Extension struct {
 	Name    string `yaml:"name"`
@@ -136,11 +59,9 @@ type patchCategory struct {
 }
 
 // Load returns the ExtensionSet for the given PHP major+minor version by reading
-// the embedded YAML data compiled into this package.
-//
-// It loads the base file for the major version (e.g. "8" → php8-base-extensions.yml),
-// then applies the patch file for the specific minor version if one exists
-// (e.g. "8"+"2" → php82-extensions-patch.yml).
+// from the testdata/ directory in this package. This is a convenience function
+// for tests only — production code should use [LoadFromDir] with the path provided
+// via the --extensions-dir CLI flag.
 //
 // Merge rules (applied by applyPatch):
 //   - Exclusions run first. If an exclusion has a matching addition, the addition
@@ -149,29 +70,19 @@ type patchCategory struct {
 //   - Remaining additions (no matching exclusion) override by name if present,
 //     or are appended.
 func Load(phpMajor, phpMinor string) (*ExtensionSet, error) {
-	baseData, ok := embeddedBases[phpMajor]
-	if !ok {
-		return nil, fmt.Errorf("php/extensions: no base extensions file for PHP major version %q", phpMajor)
-	}
-
-	var patchData []byte
-	if pd, ok := embeddedPatches[phpMajor+phpMinor]; ok {
-		patchData = pd
-	}
-
-	return loadFromData(baseData, patchData, phpMajor, phpMinor)
+	return LoadFromDir("testdata", phpMajor, phpMinor)
 }
 
 // LoadFromDir returns the ExtensionSet for the given PHP major+minor version by
 // reading YAML files from a directory on the filesystem. The directory must use
-// the same naming convention as the embedded assets:
+// the following naming convention:
 //
 //   - Base files:  php{major}-base-extensions.yml   (e.g. php8-base-extensions.yml)
 //   - Patch files: php{major}{minor}-extensions-patch.yml (e.g. php84-extensions-patch.yml)
 //
-// This allows the extension configuration to be owned by a different repository
-// (e.g. the php-buildpack) and passed to binary-builder at build time via the
-// --extensions-dir CLI flag.
+// This is the primary API for loading extension configuration. In production,
+// the directory is provided by the php-buildpack repo via the --extensions-dir
+// CLI flag.
 func LoadFromDir(dir, phpMajor, phpMinor string) (*ExtensionSet, error) {
 	basePath := filepath.Join(dir, fmt.Sprintf("php%s-base-extensions.yml", phpMajor))
 	baseData, err := os.ReadFile(basePath)
@@ -193,8 +104,8 @@ func LoadFromDir(dir, phpMajor, phpMinor string) (*ExtensionSet, error) {
 	return loadFromData(baseData, patchData, phpMajor, phpMinor)
 }
 
-// loadFromData is the shared implementation for Load and LoadFromDir. It parses
-// the base YAML, optionally applies a patch, and returns the merged ExtensionSet.
+// loadFromData is the shared implementation. It parses the base YAML, optionally
+// applies a patch, and returns the merged ExtensionSet.
 // When patchData is nil or empty, the base set is returned as-is.
 func loadFromData(baseData, patchData []byte, phpMajor, phpMinor string) (*ExtensionSet, error) {
 	var set ExtensionSet

internal/php/extensions_test.go

@@ -264,24 +264,21 @@ extensions:
 	assert.Equal(t, "redis", set.Extensions[0].Name)
 }
 
-func TestLoadFromDir_MatchesEmbeddedLoad(t *testing.T) {
-	// LoadFromDir reading the real embedded assets directory should produce
-	// the same result as the embedded Load() function.
-	//
-	// This verifies that the two code paths are functionally equivalent.
-	embeddedSet, err := php.Load("8", "4")
+func TestLoadFromDir_MatchesLoad(t *testing.T) {
+	// Load() delegates to LoadFromDir("testdata", ...), so both should
+	// produce identical results. This verifies the delegation works.
+	loadSet, err := php.Load("8", "4")
 	require.NoError(t, err)
 
-	// The embedded assets live at internal/php/assets/ relative to the repo root.
 	// In tests, the working directory is the package directory (internal/php/),
-	// so the assets dir is at "./assets".
-	dirSet, err := php.LoadFromDir("assets", "8", "4")
+	// so the testdata dir is at "./testdata".
+	dirSet, err := php.LoadFromDir("testdata", "8", "4")
 	require.NoError(t, err)
 
-	assert.Equal(t, embeddedSet.NativeModules, dirSet.NativeModules,
-		"NativeModules should match between embedded and directory-based loading")
-	assert.Equal(t, embeddedSet.Extensions, dirSet.Extensions,
-		"Extensions should match between embedded and directory-based loading")
+	assert.Equal(t, loadSet.NativeModules, dirSet.NativeModules,
+		"NativeModules should match between Load and LoadFromDir")
+	assert.Equal(t, loadSet.Extensions, dirSet.Extensions,
+		"Extensions should match between Load and LoadFromDir")
 }
 
 func TestLoadFromDir_PatchFileReadError(t *testing.T) {

internal/recipe/php.go

@@ -65,13 +65,12 @@ var phpConfigureFlags = []string{
 
 // PHPRecipe builds PHP and all of its extensions.
 //
-// By default, extension data (which PECL extensions and native modules to build)
-// is loaded from YAML files embedded in the internal/php package. When ExtensionsDir
-// is set, extension data is loaded from that directory instead, allowing the
-// configuration to be owned by a different repository (e.g. the php-buildpack).
+// ExtensionsDir is required and must point to a directory containing the extension
+// YAML files (base + per-version patches). In production, this is provided by the
+// php-buildpack repo via the --extensions-dir CLI flag.
 type PHPRecipe struct {
 	Fetcher       fetch.Fetcher
-	ExtensionsDir string // optional: filesystem directory containing extension YAML files
+	ExtensionsDir string // required: filesystem directory containing extension YAML files
 }
 
 func (p *PHPRecipe) Name() string { return "php" }
@@ -89,18 +88,13 @@ func (p *PHPRecipe) Build(ctx context.Context, s *stack.Stack, src *source.Input
 	phpMajor := parts[0]
 	phpMinor := parts[1]
 
-	// Load extension set (base + patch).
-	// If ExtensionsDir is set, load from the filesystem; otherwise use the
-	// embedded YAML data compiled into internal/php/.
-	var (
-		extSet *php.ExtensionSet
-		err    error
-	)
-	if p.ExtensionsDir != "" {
-		extSet, err = php.LoadFromDir(p.ExtensionsDir, phpMajor, phpMinor)
-	} else {
-		extSet, err = php.Load(phpMajor, phpMinor)
+	// Load extension set (base + patch) from the extensions directory.
+	// ExtensionsDir is required — it is provided by the php-buildpack repo via
+	// the --extensions-dir CLI flag in production, or by test fixtures in tests.
+	if p.ExtensionsDir == "" {
+		return fmt.Errorf("php: --extensions-dir is required for PHP builds (extension config has moved to the php-buildpack repo)")
 	}
+	extSet, err := php.LoadFromDir(p.ExtensionsDir, phpMajor, phpMinor)
 	if err != nil {
 		return fmt.Errorf("php: loading extensions: %w", err)
 	}

internal/recipe/php_test.go

@@ -3,6 +3,7 @@ package recipe_test
 import (
 	"context"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -15,6 +16,19 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// phpTestdataDir is the absolute path to the extension YAML testdata files.
+// Must be resolved at init time before any test calls useTempWorkDir() which
+// changes the working directory.
+var phpTestdataDir string
+
+func init() {
+	abs, err := filepath.Abs("../php/testdata")
+	if err != nil {
+		panic("php_test: cannot resolve testdata path: " + err.Error())
+	}
+	phpTestdataDir = abs
+}
+
 func TestPHPRecipeName(t *testing.T) {
 	r := &recipe.PHPRecipe{}
 	assert.Equal(t, "php", r.Name())
@@ -41,7 +55,7 @@ func TestPHPRecipeBuildInstallsAptPackages(t *testing.T) {
 	src := &source.Input{Version: "8.3.2"}
 	outData := &output.OutData{}
 
-	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()}
+	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir}
 	_ = r.Build(context.Background(), s, src, fakeRun, outData)
 
 	assert.True(t, hasCallMatching(fakeRun.Calls, "apt-get", "libssl-dev"), "should apt-get install libssl-dev")
@@ -65,7 +79,7 @@ func TestPHPRecipeBuildCreatesSymlinks(t *testing.T) {
 	src := &source.Input{Version: "8.3.2"}
 	outData := &output.OutData{}
 
-	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()}
+	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir}
 	_ = r.Build(context.Background(), s, src, fakeRun, outData)
 
 	assert.True(t, hasCallMatching(fakeRun.Calls, "ln", "/usr/local/include/curl"), "should create curl symlink")
@@ -84,7 +98,7 @@ func TestPHPRecipeBuildConfigureFlags(t *testing.T) {
 	src := &source.Input{Version: "8.3.2"}
 	outData := &output.OutData{}
 
-	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()}
+	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir}
 	_ = r.Build(context.Background(), s, src, fakeRun, outData)
 
 	// The configure command is run via bash -c with LIBS=-lz prefix.
@@ -113,15 +127,15 @@ func TestPHPRecipeBuildPopulatesSubDependencies(t *testing.T) {
 	src := &source.Input{Version: "8.3.2"}
 	outData := &output.OutData{}
 
-	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()}
+	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir}
 	_ = r.Build(context.Background(), s, src, fakeRun, outData)
 
-	// SubDependencies should be populated from the embedded extension YAML.
+	// SubDependencies should be populated from the extension YAML testdata.
 	// Check a representative sample of well-known extensions.
 	require.NotNil(t, outData.SubDependencies)
 	assert.Contains(t, outData.SubDependencies, "apcu", "apcu should be in sub-dependencies")
 	assert.Contains(t, outData.SubDependencies, "rabbitmq", "rabbitmq should be in sub-dependencies")
-	assert.Greater(t, len(outData.SubDependencies), 20, "should have many sub-dependencies from the embedded YAML")
+	assert.Greater(t, len(outData.SubDependencies), 20, "should have many sub-dependencies from the testdata YAML")
 }
 
 func TestPHPRecipeBuildDownloadsSource(t *testing.T) {
@@ -141,7 +155,7 @@ func TestPHPRecipeBuildDownloadsSource(t *testing.T) {
 	outData := &output.OutData{}
 
 	f := newFakeFetcher()
-	r := &recipe.PHPRecipe{Fetcher: f}
+	r := &recipe.PHPRecipe{Fetcher: f, ExtensionsDir: phpTestdataDir}
 	_ = r.Build(context.Background(), s, src, fakeRun, outData)
 
 	// Should download PHP source via Fetcher (not wget).
@@ -160,7 +174,7 @@ func TestPHPRecipeBuildInvalidVersion(t *testing.T) {
 	src := &source.Input{Version: "invalid"}
 	outData := &output.OutData{}
 
-	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher()}
+	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: phpTestdataDir}
 	err := r.Build(context.Background(), s, src, fakeRun, outData)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "invalid version")
@@ -221,3 +235,23 @@ func TestPHPRecipeBuildExtensionsDirMissingBaseFile(t *testing.T) {
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "extensions")
 }
+
+func TestPHPRecipeBuildRequiresExtensionsDir(t *testing.T) {
+	// ExtensionsDir is required. Build should fail with a clear error when
+	// it is not set (empty string).
+	useTempWorkDir(t)
+	fakeRun := runner.NewFakeRunner()
+
+	s := &stack.Stack{
+		Name:        "cflinuxfs4",
+		AptPackages: map[string][]string{"php_build": {}},
+		PHPSymlinks: []stack.Symlink{},
+	}
+	src := &source.Input{Version: "8.3.2"}
+	outData := &output.OutData{}
+
+	r := &recipe.PHPRecipe{Fetcher: newFakeFetcher(), ExtensionsDir: ""}
+	err := r.Build(context.Background(), s, src, fakeRun, outData)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "--extensions-dir is required")
+}