Skip to content

add php_server's env vars to sandboxed environment#2451

Open
henderkes wants to merge 9 commits into
mainfrom
fix/1674
Open

add php_server's env vars to sandboxed environment#2451
henderkes wants to merge 9 commits into
mainfrom
fix/1674

Conversation

@henderkes
Copy link
Copy Markdown
Contributor

closes #1674

was looking through old issues and found this one that already had a plan to solve it

Comment thread cgi.go Outdated
@henderkes henderkes force-pushed the fix/1674 branch 2 times, most recently from d1b2072 to 129303e Compare May 28, 2026 12:03
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
Comment thread cgi.go Outdated
Comment thread frankenphp.c Outdated
Comment thread frankenphp.c
Comment thread frankenphp.c
@AlliBalliBaba
Copy link
Copy Markdown
Contributor

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

Comment thread frankenphp.c Outdated
Comment thread testdata/env/prepared-env-getenv.php
@henderkes
Copy link
Copy Markdown
Contributor Author

henderkes commented May 30, 2026

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 php(_server) directive. I'm open to suggestions, but request_ is incorrect because it's not scoped to requests.

@AlliBalliBaba
Copy link
Copy Markdown
Contributor

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.

@henderkes
Copy link
Copy Markdown
Contributor Author

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?

@AlliBalliBaba
Copy link
Copy Markdown
Contributor

Hmm yeah maybe let's keep it for now. It could be scoped_env if we ever add something like the request scoping from #2398

@AlliBalliBaba
Copy link
Copy Markdown
Contributor

AlliBalliBaba commented May 31, 2026

Looks like there's currently some race conditions in tests between restarts. Not yet sure when they were introduced (not this PR)

@henderkes
Copy link
Copy Markdown
Contributor Author

Yeah that was already fixed by @alexandre-daubois by a rw lock.

@henderkes
Copy link
Copy Markdown
Contributor Author

@dunglas should be ready for review+merge now.

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.

Env vars set with the env subdirective in the php(_server) directive are not available to getenv()

2 participants