extract the root from the site block (get rid of double root directive) by henderkes · Pull Request #2311 · php/frankenphp

henderkes · 2026-03-26T13:43:34Z

I got tired of writing

localhost {
    root /path/to/root
    php_server {
        root /path/to/root
        worker index.php
    }
}

so here's my idea of fixing it. This accounts for multiple roots being defined with matchers, but it requires a root [*] /path/to/root directive to be defined. That's what caddy now stores in the new down-propagating Helper.BlockState.

We're still keeping the root as a deprecated setting. We need to store the field anyway, so the only thing that changes is that it shouldn't be specified manually anymore.

francislavoie · 2026-03-26T14:27:31Z

I'm confused, why do all this, can't it just be fetched at runtime via GetVar() like file_server does in Caddy?

henderkes · 2026-03-26T14:52:33Z

I'm confused, why do all this, can't it just be fetched at runtime via GetVar() like file_server does in Caddy?

We need the general root of the site block to start workers at Caddy startup. That's why we need to define it inside the php_server block. But we also need to define it for outside the php_server block so that Caddy's own file serving (such as file_server works.

It's also better for performance, as in that we don't always need to parse the current root anew. When the root is explicitly defined (currently), it resolves it once and then reuses it.

francislavoie · 2026-03-26T14:59:04Z

Hmm, fair enough.

I guess we could add a thing to httpcaddyfile.Helper to carry an arbitrary key-val map, then the root directive parser could fill in the root key in that map with its known value, then later whatever other directives could pull the value. This would rely on Caddyfile parsing it in order tho, but as long as users don't override default directive order or throw root and php_server in a route with root being second, it would work pretty consistently I think.

PR welcome in Caddy if you want to wire that up.

henderkes · 2026-03-26T15:05:31Z

Sure, I'll look into exposing a key-value map to retrieve any other values from the site config, too. Thank you!

henderkes · 2026-03-26T15:10:07Z

State already exists shared for all instances of a site block, so just need to add a root key to it.

henderkes · 2026-03-26T15:39:30Z

caddyserver/caddy#7594

I changed this branch to use a tagged 2.11.3.rc on my repo to let CI pass, will wait for real 2.11.3 release and then combine this with a go mod bump.

mholt · 2026-03-26T19:46:12Z

What if the root has a placeholder in it though? It can't always be known at startup.

francislavoie · 2026-03-26T20:10:24Z

Placeholder for root almost never happens, if it does it would be an env-var if anything. Not really a relevant concern.

henderkes · 2026-03-26T20:19:17Z

What if the root has a placeholder in it though? It can't always be known at startup.

Then it would fail us either way. Php must know the root at provision time.

mholt · 2026-03-26T20:40:09Z

I've seen some configs that use map to set the root dynamically (while keeping it in the site owner's control). Or matchers that simply set it to a different value if some condition is matched.

So, just be aware that if you really do piggy-back on the root directive for your PHP root, it may have a placeholder that can't be evaluated until request-time.

If you truly need the root to be 100% correct in all cases, your own root value is probably ideal.

henderkes · 2026-03-26T21:08:05Z

That's a valid heads up. I'll try with using placeholders/env variables and test with that. That's on the consumer of the root field to safety check, though.

If anything this probably means we should keep the root setting for php_server for this case, but at the same time I don't see a situation where

localhost
root {placeholder} # resolves to != /path
php_server {
    root /path
}

wouldn't explode in the users face during runtime. At least this way it could warn (/fail with workers) on startup.

…root

AlliBalliBaba · 2026-03-29T13:10:55Z

caddy/module.go

+	// 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`.


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 }

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

AlliBalliBaba · 2026-03-29T13:14:37Z

caddy/caddy_test.go

+		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")


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

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!

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

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.

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

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

henderkes · 2026-03-29T15:31:03Z

I just realized this is a BC break, which is very unfortunate...

henderkes · 2026-03-29T15:38:47Z

Not sure why the tests are failing in CI, they run locally and they should succeed. Perhaps the php_ini isn't taking effect in CI for some reason?

So, I think we can't do this after all. Example:

localhost

root /app/public
root /test /app/test
php_server

A request to /test will now serve /app/public/index.php.

AlliBalliBaba · 2026-03-29T19:34:43Z

Hmm yeah you're right, trying to guarantee the correct root while provisioning might be too messy.

henderkes added 2 commits March 26, 2026 20:20

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

3a54a40

fix: support multiple roots defined

bcf3e68

use new exported h.State["root"]

8d14123

henderkes added 4 commits March 27, 2026 12:03

safety check for placeholders

824f93f

Merge remote-tracking branch 'origin/main' into enhancement/optional-…

0d0f336

…root

fix: make tests fail instead of hang in case a request timed out

c8c459d

change was merged upstream

d400fe5

henderkes marked this pull request as ready for review March 28, 2026 18:01

henderkes requested review from AlliBalliBaba and dunglas March 28, 2026 18:07

create test that indirectly checks it works

1d9fc41

AlliBalliBaba reviewed Mar 29, 2026

View reviewed changes

henderkes added 2 commits March 29, 2026 22:00

revert deprecation

98268f6

extend test

df197b2

Conversation

henderkes commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francislavoie commented Mar 26, 2026

Uh oh!

henderkes commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francislavoie commented Mar 26, 2026

Uh oh!

henderkes commented Mar 26, 2026

Uh oh!

henderkes commented Mar 26, 2026

Uh oh!

henderkes commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mholt commented Mar 26, 2026

Uh oh!

francislavoie commented Mar 26, 2026

Uh oh!

henderkes commented Mar 26, 2026

Uh oh!

mholt commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 26, 2026

Uh oh!

AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

henderkes Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

henderkes Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

AlliBalliBaba Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

henderkes Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

henderkes commented Mar 29, 2026

Uh oh!

henderkes commented Mar 29, 2026

Uh oh!

AlliBalliBaba commented Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

henderkes commented Mar 26, 2026 •

edited

Loading

henderkes commented Mar 26, 2026 •

edited

Loading

henderkes commented Mar 26, 2026 •

edited

Loading

mholt commented Mar 26, 2026 •

edited

Loading