feat: unify php_server logic - #2499
Open
AlliBalliBaba wants to merge 75 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
This was referenced Jul 19, 2026
Contributor
|
I opened a few follow up to help get this merged ASAP, if that helps! |
Contributor
|
@AlliBalliBaba feel free to cherry-pick/squasj my other PRs as you prefer. I'm looking forward to this being merged quickly so let me know if I can help in any way (rebase, etc) |
Follow-up to #2499, targeting `refactor/phpserver`. `Server.isRegistered` is written by `Init()`/`Shutdown()` while in-flight `ServeHTTP()` calls read it concurrently, e.g. during an admin API reload under traffic; the race detector flags the plain bool. This makes it an `atomic.Bool`. The pre-existing global `isRunning` flag has the same pattern, but `Server` is the surface library users now interact with, so it seemed worth fixing here; `isRunning` can be handled separately if wanted.
Follow-up to #2499, targeting `refactor/phpserver`. Test-only. During review, an explicit `server_idx` colliding with an auto-assigned one could make one handler serve another's files; the fix (deferring registration to a single pass keyed by index) had no regression test, and nothing in `caddy/` exercised `server_idx` at all. This adds unit coverage for: - Caddyfile parsing assigns incrementing indexes - modules with the same `server_idx` share one server instance, registered only once - modules without `server_idx` each get their own server - a module with an explicit `server_idx` equal to an already registered one joins the existing server instead of re-registering it (the double-registration scenario possible with JSON configs POSTed to the admin API) These are pure Go unit tests on `registerModules()`; an end-to-end admin API config-swap test would be a natural follow-up but needs the PHP runtime, so it is not included here.
…enphp into refactor/phpserver
Follow-up to the discussion in #2499 (#2499 (comment)): this adds an optional human-readable **name** to `Server`, targeting the `refactor/phpserver` branch. Three commits, each independently buildable: 1. **Core**: `NewServer()` takes a `name` as first argument. When empty, it defaults to the server index at registration time. A `Server.Name()` getter is exposed; the field stays private. 2. **Caddy**: a name is resolved for each `php_server` block when registering the module. Cascade: first host of the enclosing route's host matcher (e.g. `api.example.com`), else the first listener address of the http server containing the module. Reuses the `*caddyhttp.App` already fetched in `Provision()`. 3. **Consumer**: `createUniqueWorkerName()` qualifies colliding worker names with the server name before falling back to the numeric postfix. Two blocks each declaring a `queue` worker now yield `queue` and `two.example.com:queue` instead of `queue` and `queue_1`, so metrics stay attributable. Non-colliding names are unchanged (no BC impact on metric labels). The last commit is separable if you'd rather keep the naming behavior untouched for now; the first two are what #2398 needs to reuse `Server` as the scope for background workers without a parallel label registry. Not included on purpose: enforcing name uniqueness across servers (the index remains the identity), and any change to `NewServer`'s signature style. If you'd prefer variadic `ServerOption`s over the growing positional list, I'm happy to adapt. --------- Signed-off-by: Alexander Stecher <45872305+AlliBalliBaba@users.noreply.github.com> Co-authored-by: Alexander Stecher <45872305+AlliBalliBaba@users.noreply.github.com>
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 :) |
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)