Skip to content

Conversation

@henderkes
Copy link
Contributor

because I feel like even with the best documentation, that's very strange behaviour

@henderkes henderkes marked this pull request as draft May 16, 2025 08:30
@henderkes henderkes marked this pull request as ready for review May 16, 2025 09:11
@henderkes henderkes changed the title print warning message when php-server is called without root print warning message when php-server is called without root, update docs May 16, 2025
@dunglas
Copy link
Member

dunglas commented May 16, 2025

It's only when you use the embedding mechanism, not the Caddyfile directive. It's intended for me because most embedded apps need to store files outside the document root (as Symfony, Laravel, etc do, the only exception is WordPress).

@henderkes
Copy link
Contributor Author

It also affects the Caddyfile in the embedded app, though, which was what led to my surprise when testing the cwd fix.

I've added another sentence to make it clear in the embed.go docs for the Caddyfile mention too.

@dunglas
Copy link
Member

dunglas commented May 16, 2025

Hum, we shouldn't do that if their is a Caddyfile, indeed.

@dunglas
Copy link
Member

dunglas commented May 16, 2025

The root isn't changed if there is a Caddyfile. This select statement will prevent the code changing the root to be called: https://github.com/dunglas/frankenphp/blob/main/caddy/php-server.go#L122

@henderkes
Copy link
Contributor Author

In my opinion we shouldn't do it at all, even if it's a BC break. Having php-server perform differently between whether there is a Caddyfile or not is even more confusing.

Just have users specify ./php-server --root public if they want to serve the public directory.

@dunglas
Copy link
Member

dunglas commented May 16, 2025

The embedding case is a bit special: we want the app to be fully functional without requiring custom CLI arguments.
Also, we don't introduce breaking changes except when totally necessary (security for instance).

@henderkes
Copy link
Contributor Author

henderkes commented May 16, 2025

Then we'll have to leave it as it is and just add as much documentation as possible. I think the way the PR does it is the best we can do without breaking anything.

At a later point I think we should juggle the documentation with a specific page just for all the extra cli commands that we add to caddy.

@henderkes
Copy link
Contributor Author

The root isn't changed if there is a Caddyfile. This select statement will prevent the code changing the root to be called: https://github.com/dunglas/frankenphp/blob/main/caddy/php-server.go#L122

Oh, I didn't see this reply earlier. I'll have to figure out what causes the root to bet set to public even though there's a Caddyfile.

@henderkes
Copy link
Contributor Author

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.

2 participants