feat: unify php_server logic - #2499
Open
AlliBalliBaba wants to merge 82 commits into
Open
Conversation
AlliBalliBaba
force-pushed
the
refactor/phpserver
branch
from
July 1, 2026 20:57
09cd397 to
39db699
Compare
dunglas
reviewed
Jul 2, 2026
Follow-up to #2499, targeting `refactor/phpserver`. Docs-only. The refactor introduces new public API for library users (`NewServer`, `WithServer`, `Server.ServeHTTP`, `WithWorkerServerScope`, `WithWorkerMatcher`) with no documentation outside godoc. This adds a `docs/library.md` page covering: - minimal setup: `NewServer` + `Init(WithServer(...))` + `Server.ServeHTTP` as an `http.Handler` - registering multiple servers (the library equivalent of multiple `php_server` blocks) - scoping workers to a server, by path and by request matcher - per-request options - the compatibility path: package-level `ServeHTTP()` keeps working through the fallback server Also linked from the README docs list and `llms.txt`. The examples follow the current `NewServer` signature on the branch; if #2529 lands first, the snippets need a one-line update (happy to rebase either way). Translations are left to the usual translation update flow.
…enphp into refactor/phpserver
Contributor
Author
|
This PR just got a bit bigger, mainly due to @nicolas-grekas contributions (#2529 #2530 #2531 #2532). IMO this PR does not need to contain everything relating to serves yet, probably to come in future PRs:
|
This was referenced Jul 20, 2026
henderkes
self-requested a review
July 21, 2026 01:26
Contributor
|
In the process of reviewing, generally seems solid, doing a tiny bit of cleanup now. |
henderkes
approved these changes
Jul 22, 2026
henderkes
left a comment
henderkes
left a comment
Contributor
There was a problem hiding this comment.
Note: seems like WithWorkerName can reach for cross-server workers by name, perhaps that's worth a note. I don't think it was possible before, because it looked in the modules local list and then fell through to the global one.
Contributor
|
Would anyone be available to merge this one? Or let me know how I can help get it merged? 🙏 |
Contributor
|
One of you two please merge. It's a bigger refactor so we should have more than only one approval :) |
Follow-up to #2499, targeting `refactor/phpserver`. Four commits, each independently reviewable. Findings from a close read of the branch. Nothing here changes the direction of the refactor; the first two are the ones I would not want to ship. ### 1. The per-worker Mercure hub is dropped `assignMercureHub()` fills `workerConfig.options`, but the new `toWorkerOptions()` builds a fresh slice and never appends it, where `Start()` used to pass `w.options` directly. The field is now written and never read. `WithWorkerMercureHub()` appends `WithMercureHub()` to the worker request options, which is what lets a worker publish from its startup code, outside `frankenphp_handle_request()`. Request-scoped publishing kept working because the module carries `WithMercureHub()` in its own request options, which is why no test noticed. #2543 extends the same function and inherits the bug. ### 2. `FrankenPHPModule.server` can be nil in `ServeHTTP()` It is assigned by `FrankenPHPApp.Start()`, but caddy starts apps by ranging over a map, so the http app can begin serving first. The handler dereferenced it unconditionally, turning that window into a nil pointer panic where `main` returned a clean `ErrNotRunning`. Two neighbouring lifecycle issues are fixed in the same commit: modules left in `app.modules` by a config that failed to provision are registered again by the next reload (the app is a process singleton and `reset()` only runs at the end of `Start()`), and `match` in a global worker block was parsed then silently ignored, so it is now rejected at parse time as `docs/config.md` already documents. ### 3. A `Server` could not be registered twice `unregisterServers()` only flipped `isRegistered`, so the worker slices and maps kept the previous run's entries and a second `Init()` with the same `*Server` failed with `two workers in a server cannot have the same filename`. Caddy dodges this by building a fresh `NewServer` per `Start()`, but `docs/library.md` presents `NewServer` + `Init` as the library pattern with no hint the instance is single-use. Same commit: the `server_<idx>` default was written into `s.name` permanently, servers were marked registered about a hundred lines before `initWorkers()` and the thread setup (a request in that window found neither a worker nor a regular thread, so `activateServers()` is split out), and `WithWorkerMatcher()` without `WithWorkerServerScope()` was a silent no-op that still got path-matched via `globalWorkersByPath`, the opposite of what the matcher asked for. ### 4. Logger and prepared env Worker startup contexts hardcoded `globalLogger` even though the worker's server was at hand, so worker boot messages and worker stdout bypassed the per-`php_server` logger this branch introduces. The module used to append `WithRequestLogger()` to each worker's request options and that line is gone, so nothing else covered it. `go_register_server_variables()` merged the prepared env whenever the server had one, but `registerPreparedEnv()` only runs from `go_update_request_info()`, which returns early without a request. For a server-scoped extension worker handling a message the merge copied whatever the thread-local prepared env held from an earlier request. ### Tests Added: `toWorkerOptions()` keeps provisioned options, a `Server` re-registered after `Shutdown()` still serves its worker and keeps its name, and a request matcher without a server scope is rejected. Both modules pass `go vet` including test files. The runtime tests need CI: on my box (WSL2) per-thread engine bootstrap of an embed ZTS build is pathologically slow and linking needs dev libs I do not have, so every `Init()`-based test is unrunnable locally, including on unmodified base commits. ### Not included `worker.mercureHub` is assigned by `configureMercure()` and never read anywhere; removing the dead field touches the `nomercure` build-tag pair, so I left it alone. Say the word and I will fold it in.
Contributor
Author
|
Some more small fixes by @nicolas-grekas are now merged in (see #2565) I also added some tweaks to make startups race-free.
|
AlliBalliBaba
commented
Aug 5, 2026
Comment on lines
-72
to
82
| func (f FrankenPHPApp) CaddyModule() caddy.ModuleInfo { | ||
| func (*FrankenPHPApp) CaddyModule() caddy.ModuleInfo { | ||
| return caddy.ModuleInfo{ | ||
| ID: "frankenphp", | ||
| New: func() caddy.Module { return &f }, | ||
| New: func() caddy.Module { return &FrankenPHPApp{} }, | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Makes much more sense to not return the same app instance. Probably was the root cause of many subtle bugs in tests
Contributor
Author
|
I'll probably merge this branch soon if that's fine with everyone involved . Otherwise we'll just end up stalling other PRs |
Member
|
I'm trying to tag a release. I hope it will be good tomorrow. Please wait for the new release to be tagged before merging, so we'll have more time to test this when merged in main. |
dunglas
requested changes
Aug 6, 2026
| @@ -0,0 +1,98 @@ | |||
| --- | |||
Member
There was a problem hiding this comment.
Shouldn't you update ExampleServeHTTP(), which is directly rendered on https://pkg.go.dev, instead of adding a new entry? Or at least couldn't we avoid duplications and redirect from this new page to the Go pkg page?
Contributor
Author
There was a problem hiding this comment.
IMO we don't necessarily need this at all, but I can update the example and link to https://pkg.go.dev/.
If that's fine with @nicolas-grekas
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently the concept of a
php_serveronly exists on the caddy side and not the FrankenPHP side.Lately we have been moving more and more in a direction of scoping requests or workers to specific
php_serverblocks.This PR is an attempt at refactoring the current
php_serverlogic so it is properly mirrored on the FrankenPHP side without BC breaks for library users (and to prevent future bugs like mentioned in #2487)