feat: make frankenphp directive optional in Caddyfile (#1601)

* make frankenphp directive optional, thanks @francislavoie

* get rid of global variable

* update workers when adding to app

* suggestions

* goto instead of continue outer?

* remove empty frankenphp directives

* update config to reflect the optional frankenphp directive

* AI translations

* restore eol newlines

* don't double check for duplicate worker name

* add short form for php_server worker too

* translations

* AI hates EOL newlines now?

* suggestion to check for nil

* suggestion to use else if block

Author: henderkes

caddy/admin_test.go

@@ -225,8 +225,6 @@ func TestAddModuleWorkerViaAdminApi(t *testing.T) {
 			skip_install_trust
 			admin localhost:2999
 			http_port `+testPort+`
-
-			frankenphp
 		}
 
 		localhost:`+testPort+` {
@@ -252,8 +250,6 @@ func TestAddModuleWorkerViaAdminApi(t *testing.T) {
 		skip_install_trust
 		admin localhost:2999
 		http_port ` + testPort + `
-
-		frankenphp
 	}
 
 	localhost:` + testPort + ` {

caddy/caddy.go

@@ -33,10 +33,6 @@ const (
 
 var iniError = errors.New("'php_ini' must be in the format: php_ini \"<key>\" \"<value>\"")
 
-// FrankenPHPModule instances register their workers, and FrankenPHPApp reads them at Start() time.
-// FrankenPHPApp.Workers may be set by JSON config, so keep them separate.
-var moduleWorkerConfigs []workerConfig
-
 func init() {
 	caddy.RegisterModule(FrankenPHPApp{})
 	caddy.RegisterModule(FrankenPHPModule{})
@@ -104,6 +100,40 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error {
 	return nil
 }
 
+func (f *FrankenPHPApp) generateUniqueModuleWorkerName(filepath string) string {
+	var i uint
+	filepath, _ = fastabs.FastAbs(filepath)
+	name := "m#" + filepath
+
+retry:
+	for _, wc := range f.Workers {
+		if wc.Name == name {
+			name = fmt.Sprintf("m#%s_%d", filepath, i)
+			i++
+
+			goto retry
+		}
+	}
+
+	return name
+}
+
+func (f *FrankenPHPApp) addModuleWorkers(workers ...workerConfig) ([]workerConfig, error) {
+	for i := range workers {
+		w := &workers[i]
+		if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(w.FileName) {
+			w.FileName = filepath.Join(frankenphp.EmbeddedAppPath, w.FileName)
+		}
+		if w.Name == "" {
+			w.Name = f.generateUniqueModuleWorkerName(w.FileName)
+		} else if !strings.HasPrefix(w.Name, "m#") {
+			w.Name = "m#" + w.Name
+		}
+		f.Workers = append(f.Workers, *w)
+	}
+	return workers, nil
+}
+
 func (f *FrankenPHPApp) Start() error {
 	repl := caddy.NewReplacer()
 
@@ -115,9 +145,7 @@ func (f *FrankenPHPApp) Start() error {
 		frankenphp.WithPhpIni(f.PhpIni),
 		frankenphp.WithMaxWaitTime(f.MaxWaitTime),
 	}
-	// Add workers from FrankenPHPApp and FrankenPHPModule configurations
-	// f.Workers may have been set by JSON config, so keep them separate
-	for _, w := range append(f.Workers, moduleWorkerConfigs...) {
+	for _, w := range append(f.Workers) {
 		opts = append(opts, frankenphp.WithWorkers(w.Name, repl.ReplaceKnown(w.FileName, ""), w.Num, w.Env, w.Watch))
 	}
 
@@ -143,7 +171,6 @@ func (f *FrankenPHPApp) Stop() error {
 	f.Workers = nil
 	f.NumThreads = 0
 	f.MaxWaitTime = 0
-	moduleWorkerConfigs = nil
 
 	return nil
 }
@@ -383,6 +410,23 @@ func (FrankenPHPModule) CaddyModule() caddy.ModuleInfo {
 // Provision sets up the module.
 func (f *FrankenPHPModule) Provision(ctx caddy.Context) error {
 	f.logger = ctx.Slogger()
+	app, err := ctx.App("frankenphp")
+	if err != nil {
+		return err
+	}
+	fapp, ok := app.(*FrankenPHPApp)
+	if !ok {
+		return fmt.Errorf(`expected ctx.App("frankenphp") to return *FrankenPHPApp, got %T`, app)
+	}
+	if fapp == nil {
+		return fmt.Errorf(`expected ctx.App("frankenphp") to return *FrankenPHPApp, got nil`)
+	}
+
+	workers, err := fapp.addModuleWorkers(f.Workers...)
+	if err != nil {
+		return err
+	}
+	f.Workers = workers
 
 	if f.Root == "" {
 		if frankenphp.EmbeddedAppPath == "" {
@@ -496,25 +540,6 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c
 	return nil
 }
 
-func generateUniqueModuleWorkerName(filepath string) string {
-	var i uint
-	name := "m#" + filepath
-
-outer:
-	for {
-		for _, wc := range moduleWorkerConfigs {
-			if wc.Name == name {
-				name = fmt.Sprintf("m#%s_%d", filepath, i)
-				i++
-
-				continue outer
-			}
-		}
-
-		return name
-	}
-}
-
 // UnmarshalCaddyfile implements caddyfile.Unmarshaler.
 func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 	// First pass: Parse all directives except "worker"
@@ -600,28 +625,14 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 					}
 				}
 
-				if wc.Name == "" {
-					wc.Name = generateUniqueModuleWorkerName(wc.FileName)
-				}
-				if !strings.HasPrefix(wc.Name, "m#") {
-					wc.Name = "m#" + wc.Name
-				}
-
 				// Check if a worker with this filename already exists in this module
 				for _, existingWorker := range f.Workers {
 					if existingWorker.FileName == wc.FileName {
 						return fmt.Errorf(`workers in a single "php_server" block must not have duplicate filenames: %q`, wc.FileName)
 					}
 				}
-				// Check if a worker with this name and a different environment or filename already exists
-				for _, existingWorker := range moduleWorkerConfigs {
-					if existingWorker.Name == wc.Name {
-						return fmt.Errorf("workers must not have duplicate names: %q", wc.Name)
-					}
-				}
 
 				f.Workers = append(f.Workers, wc)
-				moduleWorkerConfigs = append(moduleWorkerConfigs, wc)
 			}
 		}
 	}

caddy/caddy_test.go

@@ -31,8 +31,6 @@ func TestPHP(t *testing.T) {
 			admin localhost:2999
 			http_port `+testPort+`
 			https_port 9443
-
-			frankenphp
 		}
 
 		localhost:`+testPort+` {
@@ -63,8 +61,6 @@ func TestLargeRequest(t *testing.T) {
 			admin localhost:2999
 			http_port `+testPort+`
 			https_port 9443
-
-			frankenphp
 		}
 
 		localhost:`+testPort+` {
@@ -182,8 +178,6 @@ func TestNamedModuleWorkers(t *testing.T) {
 		{
 			skip_install_trust
 			admin localhost:2999
-
-			frankenphp
 		}
 
 		http://localhost:`+testPort+` {
@@ -383,8 +377,6 @@ func TestPHPServerDirective(t *testing.T) {
 			admin localhost:2999
 			http_port `+testPort+`
 			https_port 9443
-
-			frankenphp
 		}
 
 		localhost:`+testPort+` {
@@ -406,8 +398,6 @@ func TestPHPServerDirectiveDisableFileServer(t *testing.T) {
 			admin localhost:2999
 			http_port `+testPort+`
 			https_port 9443
-
-			frankenphp
 			order php_server before respond
 		}
 
@@ -434,8 +424,6 @@ func TestMetrics(t *testing.T) {
 		http_port `+testPort+`
 		https_port 9443
 		metrics
-
-		frankenphp
 	}
 
 	localhost:`+testPort+` {
@@ -800,7 +788,6 @@ func TestAllDefinedServerVars(t *testing.T) {
 			skip_install_trust
 			admin localhost:2999
 			http_port `+testPort+`
-			frankenphp
 		}
 		localhost:`+testPort+` {
 			route {

caddy/config_test.go

@@ -7,11 +7,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// resetModuleWorkers resets the moduleWorkerConfigs slice for testing
-func resetModuleWorkers() {
-	moduleWorkerConfigs = make([]workerConfig, 0)
-}
-
 func TestModuleWorkerDuplicateFilenamesFail(t *testing.T) {
 	// Create a test configuration with duplicate worker filenames
 	configWithDuplicateFilenames := `
@@ -38,53 +33,6 @@ func TestModuleWorkerDuplicateFilenamesFail(t *testing.T) {
 	// Verify that an error was returned
 	require.Error(t, err, "Expected an error when two workers in the same module have the same filename")
 	require.Contains(t, err.Error(), "must not have duplicate filenames", "Error message should mention duplicate filenames")
-	resetModuleWorkers()
-}
-
-func TestModuleWorkersDuplicateNameFail(t *testing.T) {
-	// Create a test configuration with a worker name
-	configWithWorkerName1 := `
-	{
-		php_server {
-			worker {
-				name test-worker
-				file ../testdata/worker-with-env.php
-				num 1
-			}
-		}
-	}`
-
-	// Parse the first configuration
-	d1 := caddyfile.NewTestDispenser(configWithWorkerName1)
-	module1 := &FrankenPHPModule{}
-
-	// Unmarshal the first configuration
-	err := module1.UnmarshalCaddyfile(d1)
-	require.NoError(t, err, "First module should be configured without errors")
-
-	// Create a second test configuration with the same worker name
-	configWithWorkerName2 := `
-	{
-		php_server {
-			worker {
-				name test-worker
-				file ../testdata/worker-with-env.php
-				num 1
-			}
-		}
-	}`
-
-	// Parse the second configuration
-	d2 := caddyfile.NewTestDispenser(configWithWorkerName2)
-	module2 := &FrankenPHPModule{}
-
-	// Unmarshal the second configuration
-	err = module2.UnmarshalCaddyfile(d2)
-
-	// Verify that an error was returned
-	require.Error(t, err, "Expected an error when two workers have the same name, but different environments")
-	require.Contains(t, err.Error(), "must not have duplicate names", "Error message should mention duplicate names")
-	resetModuleWorkers()
 }
 
 func TestModuleWorkersWithDifferentFilenames(t *testing.T) {
@@ -111,8 +59,6 @@ func TestModuleWorkersWithDifferentFilenames(t *testing.T) {
 	require.Len(t, module.Workers, 2, "Expected two workers to be added to the module")
 	require.Equal(t, "../testdata/worker-with-env.php", module.Workers[0].FileName, "First worker should have the correct filename")
 	require.Equal(t, "../testdata/worker-with-counter.php", module.Workers[1].FileName, "Second worker should have the correct filename")
-
-	resetModuleWorkers()
 }
 
 func TestModuleWorkersDifferentNamesSucceed(t *testing.T) {
@@ -130,6 +76,7 @@ func TestModuleWorkersDifferentNamesSucceed(t *testing.T) {
 
 	// Parse the first configuration
 	d1 := caddyfile.NewTestDispenser(configWithWorkerName1)
+	app := &FrankenPHPApp{}
 	module1 := &FrankenPHPModule{}
 
 	// Unmarshal the first configuration
@@ -158,12 +105,15 @@ func TestModuleWorkersDifferentNamesSucceed(t *testing.T) {
 	// Verify that no error was returned
 	require.NoError(t, err, "Expected no error when two workers have different names")
 
-	// Verify that both workers were added to moduleWorkerConfigs
-	require.Len(t, moduleWorkerConfigs, 2, "Expected two workers to be added to moduleWorkerConfigs")
-	require.Equal(t, "m#test-worker-1", moduleWorkerConfigs[0].Name, "First worker should have the correct name")
-	require.Equal(t, "m#test-worker-2", moduleWorkerConfigs[1].Name, "Second worker should have the correct name")
+	_, err = app.addModuleWorkers(module1.Workers...)
+	require.NoError(t, err, "Expected no error when adding the first module workers")
+	_, err = app.addModuleWorkers(module2.Workers...)
+	require.NoError(t, err, "Expected no error when adding the second module workers")
 
-	resetModuleWorkers()
+	// Verify that both workers were added
+	require.Len(t, app.Workers, 2, "Expected two workers in the app")
+	require.Equal(t, "m#test-worker-1", app.Workers[0].Name, "First worker should have the correct name")
+	require.Equal(t, "m#test-worker-2", app.Workers[1].Name, "Second worker should have the correct name")
 }
 
 func TestModuleWorkerWithEnvironmentVariables(t *testing.T) {
@@ -198,8 +148,6 @@ func TestModuleWorkerWithEnvironmentVariables(t *testing.T) {
 	require.Len(t, module.Workers[0].Env, 2, "Expected two environment variables")
 	require.Equal(t, "production", module.Workers[0].Env["APP_ENV"], "APP_ENV should be set to production")
 	require.Equal(t, "true", module.Workers[0].Env["DEBUG"], "DEBUG should be set to true")
-
-	resetModuleWorkers()
 }
 
 func TestModuleWorkerWithWatchConfiguration(t *testing.T) {
@@ -236,8 +184,6 @@ func TestModuleWorkerWithWatchConfiguration(t *testing.T) {
 	require.Equal(t, "./**/*.{php,yaml,yml,twig,env}", module.Workers[0].Watch[0], "First watch pattern should be the default")
 	require.Equal(t, "./src/**/*.php", module.Workers[0].Watch[1], "Second watch pattern should match the configuration")
 	require.Equal(t, "./config/**/*.yaml", module.Workers[0].Watch[2], "Third watch pattern should match the configuration")
-
-	resetModuleWorkers()
 }
 
 func TestModuleWorkerWithCustomName(t *testing.T) {
@@ -256,6 +202,7 @@ func TestModuleWorkerWithCustomName(t *testing.T) {
 	// Parse the configuration
 	d := caddyfile.NewTestDispenser(configWithCustomName)
 	module := &FrankenPHPModule{}
+	app := &FrankenPHPApp{}
 
 	// Unmarshal the configuration
 	err := module.UnmarshalCaddyfile(d)
@@ -267,8 +214,9 @@ func TestModuleWorkerWithCustomName(t *testing.T) {
 	require.Len(t, module.Workers, 1, "Expected one worker to be added to the module")
 	require.Equal(t, "../testdata/worker-with-env.php", module.Workers[0].FileName, "Worker should have the correct filename")
 
-	// Verify that the worker was added to moduleWorkerConfigs with the m# prefix
-	require.Equal(t, "m#custom-worker-name", module.Workers[0].Name, "Worker should have the custom name")
-
-	resetModuleWorkers()
+	// Verify that the worker was added to app.Workers with the m# prefix
+	module.Workers, err = app.addModuleWorkers(module.Workers...)
+	require.NoError(t, err, "Expected no error when adding the worker to the app")
+	require.Equal(t, "m#custom-worker-name", module.Workers[0].Name, "Worker should have the custom name, prefixed with m#")
+	require.Equal(t, "m#custom-worker-name", app.Workers[0].Name, "Worker should have the custom name, prefixed with m#")
 }