fix loading environment variable with symfony 5.1 #127

jfsenechal · 2020-06-11T19:12:36Z

here is my proposition

mtamazlicaru · 2020-09-05T11:56:00Z

src/ServiceContainer/SymfonyExtension.php

@@ -176,12 +178,25 @@ private function loadMinkParameters(ContainerBuilder $container): void
    private function loadBootstrap(?string $bootstrap): void
    {
        if ($bootstrap === null) {
+            $this->loadEnv();


Maybe instead of loading it in loadBootstrap method, to load it separately just before calling loadBoostrap, to make a check if bootstrap is not specified to try to load environment variables.

Just to have a separated context. What do you think?

yes I think you are right, since the bootstrap.php file has disappeared

What you mean has disappeared, i see that in configure method, there is still bootstrap configuration, and also there is configuration of environment, and in loadEnv method, where APP_ENV is loaded.

What i think is to let loadBoostrap method like it was, and if bootstrap configuration is null, then to have a fallback to load from .env file, if no, to do nothing like before.

Example:

$this->loadBootstrap($this->autodiscoverBootstrap($config['bootstrap'])); if (empty($config['bootstrap'])) { // try to load by calling loadEnv() method. }

What you mean has disappeared

The file was config/bootstrap.php

yes of course nothing should be changed to keep a retro compatibility

I agree with you

mtamazlicaru · 2020-09-05T11:57:12Z

src/ServiceContainer/SymfonyExtension.php

+            (new Dotenv())->bootEnv(basename(dirname(__DIR__)).'/../.env', $env);
+        }
+        catch (PathException $exception) {
+            return null;


What is the point here to return null?

I think it was to do something like this if another method of loading variables came up :

if ($bootstrap === null) { if( $this->loadEnv() === nul)) ..... return; }

Should we throw an exception if the variables could not be loaded ?

Here just to do return, instead of return null.

mtamazlicaru · 2020-09-07T07:59:30Z

src/ServiceContainer/SymfonyExtension.php

@@ -176,12 +178,25 @@ private function loadMinkParameters(ContainerBuilder $container): void
    private function loadBootstrap(?string $bootstrap): void
    {
        if ($bootstrap === null) {
+            $this->loadEnv();


What you mean has disappeared, i see that in configure method, there is still bootstrap configuration, and also there is configuration of environment, and in loadEnv method, where APP_ENV is loaded.

What i think is to let loadBoostrap method like it was, and if bootstrap configuration is null, then to have a fallback to load from .env file, if no, to do nothing like before.

Example:

$this->loadBootstrap($this->autodiscoverBootstrap($config['bootstrap'])); if (empty($config['bootstrap'])) { // try to load by calling loadEnv() method. }

mtamazlicaru · 2020-09-07T08:02:59Z

src/ServiceContainer/SymfonyExtension.php

            return;
        }

        require_once $bootstrap;
    }

+    private function loadEnv()
+    {
+        $env = getenv('APP_ENV');


Here i think is needed to pick the env like this

$config['environment'] ?? $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? 'test'; // Is how is done in 117 line number.

mtamazlicaru · 2020-09-07T08:03:23Z

src/ServiceContainer/SymfonyExtension.php

+            (new Dotenv())->bootEnv(basename(dirname(__DIR__)).'/../.env', $env);
+        }
+        catch (PathException $exception) {
+            return null;


Here just to do return, instead of return null.

mtamazlicaru · 2020-09-08T05:39:13Z

Seems ok to me, now we need to see what the maintainer (@pamil) has to say. Thanks @jfsenechal

walva · 2020-11-27T14:01:21Z

Hello there ! Do you need help for this? I had the issue today and I fixed it thanks to #126 (comment)

Yozhef · 2021-12-13T12:43:58Z

@jfsenechal can you please rebase in master)

Yozhef

I think this is a good solution but we need to start the CI process - so after merging support Symfony 6 let's do a merge and make sure everything works

pamil · 2021-12-13T13:00:37Z

src/ServiceContainer/SymfonyExtension.php

+        $env = $config['environment'] ?? $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? 'test';
+
+        try {
+            (new Dotenv())->bootEnv(basename(dirname(__DIR__)) . '/../.env', $env);


All the other IO-related functions in this file use CWD as the base - I wouldn't switch it to __DIR__ just in this PR as it introduces a non-standard behaviour. Could we go with the following?

Suggested change

(new Dotenv())->bootEnv(basename(dirname(__DIR__)) . '/../.env', $env);

(new Dotenv())->bootEnv('.env', $env);

pamil · 2021-12-13T13:02:50Z

src/ServiceContainer/SymfonyExtension.php

@@ -75,6 +77,10 @@ public function load(ContainerBuilder $container, array $config): void

        $this->loadBootstrap($this->autodiscoverBootstrap($config['bootstrap']));

+        if (empty($config['bootstrap'])) {


If using an autodiscovered bootstrap file, it would load the env variables two times, we should check for that.

Additionally, we should mention that in docs - might be not that obvious that it only happens if there's no explicitly passed bootstrap file.

jfsenechal · 2021-12-13T14:18:45Z

@jfsenechal can you please rebase in master)

Hi, I am sorry I don't know how to do that

Yozhef · 2021-12-13T15:47:05Z

@jfsenechal can you please rebase in master)

Hi, I am sorry I don't know how to do that

Okey, can you please merge original master in your pr - because we add Git Hub Action when run test

jfsenechal · 2021-12-13T21:28:53Z

@jfsenechal can you please rebase in master)

Hi, I am sorry I don't know how to do that

Okey, can you please merge original master in your pr - because we add Git Hub Action when run test

can you the commands to be executed i never did that

Yozhef · 2021-12-14T10:46:53Z

@jfsenechal you need to pull in your fork actual master then in your pr git pull master -> then you see conflict, resolve his and push )

How pull in your fork actual master:

Go to work
press button

jfsenechal added 4 commits June 11, 2020 21:09

fix loading environment variable with symfony 5.1

4744163

add dependency symfony/dotenv

7cf5e6c

Load class Dotenv

a96f521

Catch PathException

068cb02

mtamazlicaru reviewed Sep 5, 2020

View reviewed changes

mtamazlicaru reviewed Sep 7, 2020

View reviewed changes

jfsenechal added 3 commits September 7, 2020 13:53

Change method to get app env

3fc6f21

Return void instead of null

4766c95

try to load by calling loadEnv() after autodiscoverBootstrap

cdb00f8

mtamazlicaru approved these changes Sep 8, 2020

View reviewed changes

jfsenechal added 2 commits September 16, 2020 10:13

fix ecs

22728b4

fix missing $config variable

91cb6b8

jfsenechal mentioned this pull request Oct 1, 2020

Environment variable not found since symfony 5.1 #126

Open

Yozhef approved these changes Dec 13, 2021

View reviewed changes

pamil requested changes Dec 13, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix loading environment variable with symfony 5.1 #127

fix loading environment variable with symfony 5.1 #127

jfsenechal commented Jun 11, 2020

mtamazlicaru Sep 5, 2020

jfsenechal Sep 7, 2020

mtamazlicaru Sep 7, 2020

jfsenechal Sep 7, 2020

mtamazlicaru Sep 5, 2020

jfsenechal Sep 7, 2020

mtamazlicaru Sep 7, 2020

mtamazlicaru Sep 7, 2020

mtamazlicaru Sep 7, 2020

mtamazlicaru Sep 7, 2020

mtamazlicaru commented Sep 8, 2020

walva commented Nov 27, 2020

Yozhef commented Dec 13, 2021

Yozhef left a comment

pamil Dec 13, 2021

pamil Dec 13, 2021

jfsenechal commented Dec 13, 2021

Yozhef commented Dec 13, 2021

jfsenechal commented Dec 13, 2021

Yozhef commented Dec 14, 2021

	(new Dotenv())->bootEnv(basename(dirname(__DIR__)) . '/../.env', $env);
	(new Dotenv())->bootEnv('.env', $env);

		@@ -75,6 +77,10 @@ public function load(ContainerBuilder $container, array $config): void

		$this->loadBootstrap($this->autodiscoverBootstrap($config['bootstrap']));

		if (empty($config['bootstrap'])) {

fix loading environment variable with symfony 5.1 #127

Are you sure you want to change the base?

fix loading environment variable with symfony 5.1 #127

Conversation

jfsenechal commented Jun 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtamazlicaru commented Sep 8, 2020

walva commented Nov 27, 2020

Yozhef commented Dec 13, 2021

Yozhef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfsenechal commented Dec 13, 2021

Yozhef commented Dec 13, 2021

jfsenechal commented Dec 13, 2021

Yozhef commented Dec 14, 2021