Conversation
d1b2072 to
129303e
Compare
keep track of separate prepared_env set for a php_server correctly exposes env vars into $_ENV when E is in `variables_order` also fixes the problem of lazy server eval I didn't think about last commit
|
Tbh the name 'prepared_env' has always confused me. "request_specific_env", "request_local_env", "request_context_env", "request_env" or something similar would make more sense IMO |
Prepared as in prepared by the |
|
It is kind of scoped to the request since you can add request-specific caddy replacers to it. We currently lack an equivalent of the php_server scope from the caddy side in the frankenphp library side, but adding one would probably be too much for this PR. |
|
Hmm that's true, but it would be a bit confusing to call it request_env as that could be understood to be the sandboxed one. Perhaps caddy_handler_env? Or just live with prepared_env? |
|
Hmm yeah maybe let's keep it for now. It could be |
|
Looks like there's currently some race conditions in tests between restarts. Not yet sure when they were introduced (not this PR) |
|
Yeah that was already fixed by @alexandre-daubois by a rw lock. |
|
@dunglas should be ready for review+merge now. |
There was a problem hiding this comment.
Pull request overview
This PR addresses #1674 by ensuring environment variables set via the env subdirective (and WithRequestEnv) are available through getenv() in addition to $_SERVER, by introducing a distinct “prepared environment” layer that is merged appropriately.
Changes:
- Add a per-thread
prepared_envlayer in the C runtime and update getenv/$_ENV population logic to include it. - Update Go ↔ C integration to register “prepared env” early (before PHP userland runs) and merge it into
$_SERVER. - Add regression tests and test scripts covering
getenv()visibility,variables_orderbehavior, and interaction withputenv().
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
testdata/env/prepared-env-survives-putenv.php |
Adds a regression script ensuring prepared env remains visible after putenv(). |
testdata/env/prepared-env-getenv.php |
Adds a regression script asserting prepared env is visible via getenv() and $_SERVER (and conditionally $_ENV). |
frankenphp.h |
Exposes new C APIs for prepared env registration/merge. |
frankenphp.c |
Implements prepared_env layer and updates getenv/$_ENV merging behavior. |
frankenphp_test.go |
Adds Go tests reproducing #1674 and verifying layering/variables_order behavior. |
cgi.go |
Registers prepared env earlier (for getenv()), and merges it into $_SERVER later. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Marc <m@pyc.ac>
(unless placeholders are used, bad idea anyway)
closes #1674
was looking through old issues and found this one that already had a plan to solve it