Skip to content

extract the root from the site block (get rid of double root directive) #2311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
henderkes wants to merge 10 commits into
base: main
Choose a base branch
from
+62 −5
Open

extract the root from the site block (get rid of double root directive) #2311

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a54a40
extract the root from the site block (get rid of double root directive)
henderkes Mar 26, 2026
bcf3e68
fix: support multiple roots defined
henderkes Mar 26, 2026
8d14123
use new exported h.State["root"]
henderkes Mar 26, 2026
824f93f
safety check for placeholders
henderkes Mar 27, 2026
0d0f336
Merge remote-tracking branch 'origin/main' into enhancement/optional-…
henderkes Mar 27, 2026
c8c459d
fix: make tests fail instead of hang in case a request timed out
henderkes Mar 27, 2026
d400fe5
change was merged upstream
henderkes Mar 28, 2026
1d9fc41
create test that indirectly checks it works
henderkes Mar 28, 2026
98268f6
revert deprecation
henderkes Mar 29, 2026
df197b2
extend test
henderkes Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion caddy/admin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ func TestAutoScaleRegularThreadsOnAutomaticThreadLimit(t *testing.T) {
wg.Add(requestsPerTry)
for range requestsPerTry {
go func() {
defer wg.Done()
tester.AssertGetResponse(endpoint, http.StatusOK, "slept for 2 ms and worked for 1000 iterations")
wg.Done()
}()
}
wg.Wait()
Expand Down
21 changes: 21 additions & 0 deletions caddy/caddy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,27 @@ func TestPHPServerDirective(t *testing.T) {
tester.AssertGetResponse("http://localhost:"+testPort+"/not-found.txt", http.StatusOK, "I am by birth a Genevese (i not set)")
}

func TestPHPServerDirectiveWorker(t *testing.T) {
tester := caddytest.NewTester(t)
initServer(t, tester, `
{
skip_install_trust
admin localhost:2999
http_port `+testPort+`
https_port 9443
}

localhost:`+testPort+` {
root ../testdata
php_server {
worker worker-with-counter.php
}
}
`, "caddyfile")

tester.AssertGetResponse("http://localhost:"+testPort+"/worker-with-counter.php", http.StatusOK, "requests:1")
Comment on lines +480 to +497
Copy link
Copy Markdown
Contributor

@AlliBalliBaba AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this test also green on main? Maybe would make sense to test with multiple php_servers and multiple roots.

Copy link
Copy Markdown
Contributor Author

@henderkes henderkes Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this test also green on main? Maybe would make sense to test with multiple php_servers and multiple roots.

It would fail to start because it wouldn't find the worker-with-counter.php. But yes, I'll do that!

Copy link
Copy Markdown
Contributor

@AlliBalliBaba AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh because the path will be relative to the working directory and not relative to the server root. I guess it makes sense then 👍

Copy link
Copy Markdown
Contributor

@AlliBalliBaba AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes this a small BC break though for people expecting the working directory. I wonder if we should check both relative path to root and relative path to working directory. Or at least warn if one or the other was found.

Copy link
Copy Markdown
Contributor Author

@henderkes henderkes Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly I think we can't do this at all, after all.
#2311 (comment)

Ugh, I'm so tired of double typing the root :(

}

func TestPHPServerDirectiveDisableFileServer(t *testing.T) {
tester := caddytest.NewTester(t)
initServer(t, tester, `
Expand Down
2 changes: 1 addition & 1 deletion caddy/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ replace github.com/dunglas/frankenphp => ../
retract v1.0.0-rc.1 // Human error

require (
github.com/caddyserver/caddy/v2 v2.11.2
github.com/caddyserver/caddy/v2 v2.11.3-0.20260328174442-62e9c052648f
github.com/caddyserver/certmagic v0.25.2
github.com/dunglas/caddy-cbrotli v1.0.1
github.com/dunglas/frankenphp v1.12.1
Expand Down
4 changes: 2 additions & 2 deletions caddy/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
github.com/bits-and-blooms/bitset v1.24.4 h1:95H15Og1clikBrKr/DuzMXkQzECs1M6hhoGXLwLQOZE=
github.com/bits-and-blooms/bitset v1.24.4/go.mod h1:7hO7Gc7Pp1vODcmWvKMRA9BNmbv6a/7QIWpPxHddWR8=
github.com/caddyserver/caddy/v2 v2.11.2 h1:iOlpsSiSKqEW+SIXrcZsZ/NO74SzB/ycqqvAIEfIm64=
github.com/caddyserver/caddy/v2 v2.11.2/go.mod h1:ASNYYmKhIVWWMGPfNxclI5DqKEgU3FhmL+6NZWzQEag=
github.com/caddyserver/caddy/v2 v2.11.3-0.20260328174442-62e9c052648f h1:UmlXx5sytQ3IwWTA8kN/e9AJyaKgeZ2N1jEInipX3HQ=
github.com/caddyserver/caddy/v2 v2.11.3-0.20260328174442-62e9c052648f/go.mod h1:eu0tCpKfbDkztZ5znboBOWG95MaypGCiEe2+455hZIA=
github.com/caddyserver/certmagic v0.25.2 h1:D7xcS7ggX/WEY54x0czj7ioTkmDWKIgxtIi2OcQclUc=
github.com/caddyserver/certmagic v0.25.2/go.mod h1:llW/CvsNmza8S6hmsuggsZeiX+uS27dkqY27wDIuBWg=
github.com/caddyserver/zerossl v0.1.5 h1:dkvOjBAEEtY6LIGAHei7sw2UgqSD6TrWweXpV7lvEvE=
Expand Down
22 changes: 21 additions & 1 deletion caddy/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type FrankenPHPModule struct {
mercureContext
hotReloadContext

// Root sets the root folder to the site. Default: `root` directive, or the path of the public directory of the embed app it exists.
// Deprecated: Use the `root` directive in the site block instead.
Root string `json:"root,omitempty"`
// SplitPath sets the substrings for splitting the URI into two parts. The first matching substring will be used to split the "path info" from the path. The first piece is suffixed with the matching substring and will be assumed as the actual resource (CGI script) name. The second piece will be set to PATH_INFO for the CGI script to use. Default: `.php`.
Copy link
Copy Markdown
Contributor

@AlliBalliBaba AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should necessarily deprecate this. Not using it myself, but theoretically you can have multiple php_servers per domain with different roots, same as you can have multiple file_servers with different roots (without handle {...}).

@path1 path /path1/*
php_server @app {
    root /some/root
}

@path2 path /path2/*
php_server @path2{
    root /other/root
}

Copy link
Copy Markdown
Contributor Author

@henderkes henderkes Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah that's a fair point. It needs to stay in the code anyway, so I'll revert the deprecation.

SplitPath []string `json:"split_path,omitempty"`
Expand Down Expand Up @@ -261,6 +261,7 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
for d.NextBlock(0) {
switch d.Val() {
case "root":
caddy.Log().Named("caddyfile").Warn("DEPRECATED: the 'root' subdirective of 'php'/'php_server' is deprecated, use the 'root' directive in the site block instead")
if !d.NextArg() {
return d.ArgErr()
}
Expand Down Expand Up @@ -418,6 +419,7 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error)
// parse the php_server subdirectives
switch dispenser.Val() {
case "root":
caddy.Log().Named("caddyfile").Warn("DEPRECATED: the 'root' subdirective of 'php_server' is deprecated, use the 'root' directive in the site block instead")
if !dispenser.NextArg() {
return nil, dispenser.ArgErr()
}
Expand Down Expand Up @@ -470,6 +472,16 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error)
return nil, err
}

// If no root was specified inside php_server, inherit it from the site-level root directive.
// Skip roots containing placeholders (e.g. {env.APP_ROOT}).
// We can check for `{` here because {$ENV} vars are already resolved earlier
if phpsrv.Root == "" {
if siteRoot := extractSiteRoot(h); siteRoot != "" && !strings.Contains(siteRoot, "{") {
phpsrv.Root = siteRoot
fsrv.Root = siteRoot
}
}

if frankenphp.EmbeddedAppPath != "" {
if phpsrv.Root == "" {
phpsrv.Root = filepath.Join(frankenphp.EmbeddedAppPath, defaultDocumentRoot)
Expand Down Expand Up @@ -675,6 +687,14 @@ func prependWorkerRoutes(routes caddyhttp.RouteList, h httpcaddyfile.Helper, f F
return routes
}

func extractSiteRoot(h httpcaddyfile.Helper) string {
// Caddy stores only unmatched or wildcard matcher roots
if root, ok := h.BlockState["root"].(string); ok {
return root
}
return ""
}

// Interface guards
var (
_ caddy.Provisioner = (*FrankenPHPModule)(nil)
Expand Down
Loading