Crell / EnvMapper

Easily map environment variables into defined classed objects, ready for Dependency Injection.
GNU Lesser General Public License v3.0
84 stars 3 forks source link

Reliance on $_ENV #6

Closed bcremer closed 1 year ago

bcremer commented 1 year ago

The $_ENV superglobal is not populated when the recommended value for variables_order is used.

variables_order = GPCS, see: https://github.com/php/php-src/blob/master/php.ini-production#L160

A dotenv loader like vlucas/phpdotenv or symfony/dotenv may populate $_ENV during script runtime.

But in production we often times do not use a dotenv loader but "native" exported Environment variables:

DATABASE_USER=foobar php myscript.php

In those cases $_ENV is empty and the objects cannot be populated.

Would you be open for PR that inroduces a getenv() fallback to the env-reading like so?

$var = $_ENV[$name] ?? getenv($name)
jdreesen commented 1 year ago

The readme says:

EnvMapper does not use getenv() as it is much slower.

So I'm not sure if it's a good idea to call getenv($name) for each variable.

But maybe we can retrieve the data once from getenv() at the beginning when $_ENV is empty, like:

$source ??= $_ENV ?: getenv();

In fact, the library already allows this behavior by letting you pass this as the third argument of map().

Crell commented 1 year ago

Sigh, PHP's env handling is such a mess...

A .env loader should be fine, as long as it runs before EnvMapper does. Which if it doesn't, the app is misconfigured and it should be adjusted to run the env loader first.

I'm not sure if a simple ?? getenv() would work, though, because $_ENV could be partially populated. It should always have the basic Unix stuff (which 99.9% of the time we ignore), so a blanket check for it wouldn't work.

Passing in getenv() to $source should work, though. (I added that argument for testing, but hey look, useful features!) Whether or not to try reading getenv() directly in the class, I'm not sure about. I suppose it depends on the performance implications.

That said... most of the time I'd expect someone to be using real-envs or a .env, both of which should populate $_ENV. So it's an edge case, at least.

jdreesen commented 1 year ago

Well, as @bcremer said, whether $_ENV gets populated depends on variables_order, which does not necessarily contain E by default.

I just tested it (on my Windows system) and if variables_order is just GPCS (default), $_ENV is completely empty. If it is EGPCS instead, $_ENV contains a whole bunch of entries.

This means I would have to change the ini or use an .env loader which populates $_ENV for me to use this library (or pass getenv() as the third argument).

So maybe we should at least add a paragraph in the readme, that this library (currently) relies on the E in variables_order to be set or some kind of .env loader.

Crell commented 1 year ago

I ended up adding a comment to the README: https://github.com/Crell/EnvMapper/commit/86269148062b151315dc06bab07280f1ccc84363

Closing this issue as resolved, since there's now clear documentation of the limitation and a workaround.