[5.6] Catches InvalidFileException when loading environment file with dotenv

[shrink]

Dotenv may throw an exception when loading a .env file, either InvalidPathException or InvalidFileException. At present LoadEnvironmentVariables is only catching InvalidPathException. This means that when you provide an invalid .env file Laravel outputs an unhelpful error message, e.g:

APP_NAME=Laravel Framework

Fatal error: Uncaught ReflectionException: Class config does not exist in /app/vendor/laravel/framework/src/Illuminate/Container/Container.php:767 Stack trace: #0
/app/vendor/laravel/framework/src/Illuminate/Container/Container.php(767): ReflectionClass->__construct('config') #1 
/app/vendor/laravel/framework/src/Illuminate/Container/Container.php(646): Illuminate\Container\Container->build('config') #2 
[...]

Many are confused by this error: 1, 2, 3, 4, 5, 6.

This change captures the additional error, meaning that when invalid .env file is provided (usually because a value containing a space lacks quotation marks) Laravel will fall back to the default application config.

---

I think that when this error occurs Laravel should not continue executing[1] because it is a very difficult situation to debug without an error, and it's unlikely a developer will want their application to run without a .env — and if they do, they can create an empty .env file — but I haven't added it in case there is a use case I am not aware of that necessitates allowing the application run even with an invalid .env file provided. Any thoughts on terminating execution here?

[1] As far as I know the only way to output an error here would be dd($e) because the error handling functionality isn't loaded at this point in the bootstrap process.

[taylorotwell]

How do you trigger an InvalidFileException in this case?

[shrink]

The exception is thrown when a value that contains a space is not enclosed in quotation marks, e.g:

APP_NAME=Laravel Framework

https://github.com/vlucas/phpdotenv/blob/master/src/Loader.php#L220

    // Unquoted values cannot contain whitespace
    if (preg_match('/\s+/', $value) > 0) {
        throw new InvalidFileException('Dotenv values containing spaces must be surrounded by quotes.');
    }

[decadence]

+1 for this. It's hard to debug because current Exception message says nothing about .env file.

[ntzm]

Surely this will make it harder to debug?

[sisve]

I'm not sure I see the positive gain here. I've added the app_name with unquoted spaces to my .env file, and running php artisan outputs the following on Laravel 5.5 (this error is also written to the log file).

[Dotenv\Exception\InvalidFileException]
Dotenv values containing spaces must be surrounded by quotes.

This change would silently ignore the problem instead of telling me. (Or am I missing something since I'm testing this on 5.5?)

[shrink]

Current behaviour:

1. You don't provide a .env file: The default configuration is loaded.
2. You have an invalid .env file, a fatal error occurs:

Fatal error: Uncaught ReflectionException: Class config does not exist [...]

Pull Request Behaviour

1. You don't provide a .env file: The default configuration is loaded.
2. You have an invalid .env file: The default configuration is loaded.

My Proposed Behaviour

1. You don't provide a .env file: request is terminated, dumping the Dotenv exception (dd($e))
2. You have an invalid .env file: request is terminated, dumping the Dotenv exception (dd($e))

Dumping the exception would result in this:

InvalidFileException {#76 ▼
  #message: "Dotenv values containing spaces must be surrounded by quotes."
  #code: 0
  #file: "/app/vendor/vlucas/phpdotenv/src/Loader.php"
  #line: 228
  trace: {▶}
}

Very easy to understand and fix.

---

The current behaviour is inconsistent and confusing (see the links above). At the very least the behaviour should be consistent. Personally I think that the request should be terminated and the exception dumped, but if that's not desirable for some reason then this pull request at least makes the behaviour consistent (if .env can't be loaded, then the request continues and the default configuration is used).

@sisve many developers don't interact with Laravel on the command line frequently, they develop in their browser, where the non-descript error is displayed.

@ntzm I don't believe so, either of the approaches (pull request, or preferred option) at the very least introduces consistent behaviour and "why is my configuration not being loaded?" is a much better starting point for this issue than the Fatal error: Uncaught ReflectionException: Class config does not exist error.

[shrink]

Although Laravel does create a .env file on install and Laravel does by default depend on the APP_KEY value in .env to be secure, it is possible that some Laravel applications out there do not have a .env file and so to implement my preferred solution would potentially break some applications (until they added an empty .env file).

The alternative is to keep the current behaviour and add additional behaviour for InvalidFileException, e.g:

try {
    (new Dotenv($app->environmentPath(), $app->environmentFile()))->load();
} catch (InvalidPathException $e) {
    //
} catch (InvalidFileException $e) {
    dd('Fatal Error: Values in .env containing spaces must be surrounded by quotes.');
}

1. An application does not have a .env file: the default configuration is loaded
2. An application has an invalid .env file: the error message ("Values in .env containing spaces must be surrounded by quotes.") is dumped.

I have updated the pull request to use the backwards compatible behaviour — missing .env does not trigger an error, an error is only triggered when the .env is invalid.

[GrahamCampbell]

I don't think this is right. There are way more reasons parsing can fail than just double quotes missing!

[shrink]

@GrahamCampbell Are you sure? As far as I can tell the only place the InvalidFileException is used is when checking the file for values containing spaces that are missing quotes: https://github.com/vlucas/phpdotenv/blob/master/src/Loader.php#L251 — so the only way this error can occur is if a value is missing quotes.

[GrahamCampbell]

Would it not be better to dump the exception message then?

[GrahamCampbell]

To be honest, this PR feels like a massive hack to me. Surely the correct fix would be to handle exceptions properly that occur at this early stage in the bootstrap process, and present them properly?

[shrink]

@GrahamCampbell That would be my preference, if there is a way to handle errors correctly then that would be much better. However, as far as I can tell, at this point in the bootstrap process Laravel doesn't have access to anything necessary to error properly (i.e: the exception system depends on valid configuration) so it would require changes to the architecture. I could very well be wrong there, though, are there ways to error properly here?

Re: dumping exception message vs. a custom error message, the exception message does not explicitly state the issue is with the .env file and some developers (novice, esl) may not make the connection between the package name "Dotenv" and the file ".env", as the exception message is: "Dotenv values containing spaces must be surrounded by quotes.".

That said, anything (whether it's dumping the exception, or a custom error message, or even loading the default configuration) is an improvement over the current non-descript uncaught fatal error that causes so much confusion, so if there's a different approach to this problem that is better than mine then I support it.

[chrissound]

This also seems to occur if you have an error (In my case a call to a undefined function) within config.php.