Skip to content

make frankenphp directive optional, thanks @francislavoie #1601

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.

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

Merged
merged 15 commits into from
Jun 2, 2025

Conversation

henderkes
Copy link
Contributor

No description provided.

@francislavoie
Copy link
Contributor

francislavoie commented May 23, 2025

I didn't realize a global variable was being used here

https://github.com/dunglas/frankenphp/blob/b6fcab5a95677a111405473f15d7b6da026b58e9/caddy/caddy.go#L38

This can be gotten rid of by having each handler instance call methods on the app instance in Provision to add their worker configs (and that would validate if there's conflicts).

So like:

app, err := ctx.App("frankenphp")
if err != nil {
	return err
}

app.AddWorkerConfig(...)

It's not right at all that UnmarshalCaddyfile is setting global variables that are later picked up at runtime. That means that if someone uses Caddy's config API to push a new JSON config (Caddyfile adapted to JSON out of band) then it wouldn't work because a different process adapted the Caddyfile config than the one that will actually run it.

@henderkes
Copy link
Contributor Author

henderkes commented May 23, 2025

edit : resolved

@henderkes henderkes requested a review from dunglas May 23, 2025 09:24
@dunglas
Copy link
Member

dunglas commented Jun 2, 2025

Excellent work. Thanks!

@dunglas dunglas merged commit 5a43e9f into php:main Jun 2, 2025
41 of 43 checks passed
henderkes added a commit to mhpcc/frankenphp that referenced this pull request Jun 5, 2025
* make frankenphp directive optional, thanks @francislavoie

* get rid of global variable

* update workers when adding to app

* suggestions

* goto instead of continue outer?

* remove empty frankenphp directives

* update config to reflect the optional frankenphp directive

* AI translations

* restore eol newlines

* don't double check for duplicate worker name

* add short form for php_server worker too

* translations

* AI hates EOL newlines now?

* suggestion to check for nil

* suggestion to use else if block
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.

6 participants