Allow to set trusted proxies in the environment variables #68
Conversation
| @@ -21,6 +22,19 @@ | |||
| /** @var Composer\Autoload\ClassLoader */ | |||
| $loader = require __DIR__.'/../vendor/autoload.php'; | |||
|
|
|||
| if (file_exists(__DIR__.'/../.env')) { | |||
| (new Dotenv())->load(__DIR__.'/../.env'); | |||
| } | |||
There was a problem hiding this comment.
Is that needed in the app.php?
There was a problem hiding this comment.
Not really related to #69, but we need it if people want to configure production using .env files. I agree it's not Symfony best practice, but 90% of our people don't have real server variables.
There was a problem hiding this comment.
I don't like this at all. There are valid reasons why Symfony recommends not to use the Dotenv component in production.
src/Resources/skeleton/web/app.php
Outdated
| } | ||
|
|
||
| // see https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/3.3/public/index.php#L27 | ||
| if ($trustedProxies = $_SERVER['TRUSTED_PROXIES'] ?? false) { |
There was a problem hiding this comment.
This is a rather unconventional piece of code. 😄 I guess we should at least use ?? null instead of ?? false, shouldn't we?
There was a problem hiding this comment.
I see you have copied it from Symfony …
| @@ -21,6 +22,19 @@ | |||
| /** @var Composer\Autoload\ClassLoader */ | |||
| $loader = require __DIR__.'/../vendor/autoload.php'; | |||
|
|
|||
| if (file_exists(__DIR__.'/../.env')) { | |||
| (new Dotenv())->load(__DIR__.'/../.env'); | |||
| } | |||
There was a problem hiding this comment.
I don't like this at all. There are valid reasons why Symfony recommends not to use the Dotenv component in production.
Yes, the don't recommend it, because you should use environment variables. Unfortunately, as we know, 95% of Contao setups on shared hosting don't support environment variables. That's what the DotEnv component is for. If your server does support ENV variables, don't create the file and use it to adjust your setup, and then this will work as well. Obviously not through the Contao Manager, but if you have and use environment variables, you probably don't need the Contao Manager. |
|
Correct me if I'm wrong, but environment variables can easily be set in e.g. a But nevertheless, this kind of configuration should go into the |
No they don't, because this is application configuration that is loaded before the kernel is booted or the container is available. |
aschempp
force-pushed
the
feature/trusted-proxies
branch
from
2fe5fab
to
9293911
Compare
|
Thank you @aschempp. |
Description ----------- This allows the Contao Manager (and other API tools) to write all environment variables. Necessary to be able to e.g. add trusted proxies (#68). FYI, changing/removing existing commands is intended and that's what the API version is for. Commits ------- b09f472 Replace the access-key API command with generic dot-env commands 34c743a Added unit tests for DotEnv commands 699183d Fix the coding style.
fixes #63