Skip to content

feat: unify php_server logic - #2499

Merged
AlliBalliBaba merged 91 commits into
mainfrom
refactor/phpserver
Aug 21, 2026
Merged

feat: unify php_server logic#2499
AlliBalliBaba merged 91 commits into
mainfrom
refactor/phpserver

Conversation

@AlliBalliBaba

Copy link
Copy Markdown
Contributor

Currently the concept of a php_server only 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_server blocks.

This PR is an attempt at refactoring the current php_server logic so it is properly mirrored on the FrankenPHP side without BC breaks for library users (and to prevent future bugs like mentioned in #2487)

Comment thread options.go Outdated
Comment thread options.go Outdated
Comment thread options.go Outdated
Comment thread phpserver.go Outdated
@AlliBalliBaba

AlliBalliBaba commented Aug 5, 2026

Copy link
Copy Markdown
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.

  • ServeHTTP() will now block up to 10s waiting until Init() has finished, making it impossible to send requests before startup. (the http server start can happen before the FrankenPHP start)
  • Shutdown() will now always wait for Startup() to finish, removing any potential race detections (mainly annoying in tests)

Comment thread caddy/app.go
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{} },
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes much more sense to not return the same app instance. Probably was the root cause of many subtle bugs in tests

@AlliBalliBaba

Copy link
Copy Markdown
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

@dunglas

dunglas commented Aug 5, 2026

Copy link
Copy Markdown
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
dunglas previously requested changes Aug 6, 2026
Comment thread caddy/module.go Outdated
Comment thread context.go Outdated
Comment thread caddy/app.go Outdated
Comment thread caddy/app.go Outdated
Comment thread caddy/app.go Outdated
Comment thread caddy/module.go Outdated
Comment thread caddy/workerconfig.go Outdated
Comment thread cgi.go Outdated
Comment thread frankenphp.go Outdated
Comment thread docs/library.md
@henderkes
henderkes requested a review from dunglas August 11, 2026 17:27
@AlliBalliBaba

Copy link
Copy Markdown
Contributor Author

Looking through global options again, it probably also makes sense to move maxWaitTime and maxRequests to Server. So maybe an extensible api like this would still be preferrable?

server := NewServer(root string, opts... ServerOption )

func WithServerLogger(logger) ServerOption
func WithServerMaxRequests(num) ServerOption
...

@dunglas

dunglas commented Aug 15, 2026

Copy link
Copy Markdown
Member

I agree, it feels like we'll add more options in the future

@henderkes

Copy link
Copy Markdown
Contributor

Let's get this merged first 😆 . There are plenty other features waiting on this too.

@AlliBalliBaba

Copy link
Copy Markdown
Contributor Author

Added the server options just to not have BC breaks. Ready to merge whenever

@AlliBalliBaba

AlliBalliBaba commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Will merge if there's no other blocker @dunglas. There's still some more cleanup to do, doesn't need to all be in this PR though.

@dunglas

dunglas commented Aug 21, 2026

Copy link
Copy Markdown
Member

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants