fix loading environment variable with symfony 5.1

[jfsenechal]

here is my proposition

[mtamazlicaru]

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?

[jfsenechal]

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

[mtamazlicaru]

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.
}

[jfsenechal]

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]

What is the point here to return null?

[jfsenechal]

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 ?

[mtamazlicaru]

Here just to do return, instead of return null.

[mtamazlicaru]

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]

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]

Here just to do return, instead of return null.

[mtamazlicaru]

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

[walva]

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

[Yozhef]

@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]

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]

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]

@jfsenechal can you please rebase in master)

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

[Yozhef]

@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]

@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]

@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:

1. Go to work
2. press button

[pieterocp]

It's been nearly 4 years, is this worth pulling out to another branch, rebasing, adding a few more tests to get this merged in? (and ofc, recognising the efforts of contributors involved in this changeset)