Throw warning on invalid config

[laurencei]

Currently if you call a config value that is does not exist/invalid - it is a silent failure:

dd(config('custom.i_do_not_exist')) // null

This has lead to a few times where silent errors have crept into my application and took a while to debug, since it is not obvious at first the config value is wrong (as it will be null).

It would be nice if, while the application is in APP_DEBUG mode, it would throw an exception when calling an invalid config item?

dd(config('custom.i_do_not_exist')) // EXCEPTION; config item does not exist

[happyDemon]

Couldn't you just check what the config function returns and then throw an exception yourself?

Some people rely on the default value you can add as the second parameter.

[laurencei]

But the config function returns null when it is wrong - which is not the same thing as not found or a catchable exception. And you could need to add this everywhere you are doing a config item lookup as well.

Basically what I am saying; is the best thing for the framework to silently fail (with null) when a config value is not found - versus throwing an exception which makes it easy to debug and obvious during development?

Maybe it can be a config flag in itself (which is ironic). You could have strict_config => false as an app option?

When strict_config is false - that is the current default behaviour and will return null when a value is not found.

But when strict_config is true - that throws an exception when a value is not found (and no default is given), instead of returning null.

This would help towards possibly making it BC and not breaking any current applications?

[jlaswell]

But the config function returns null when it is wrong - which is not the same thing as not found or a catchable exception.

I think equating a missing config value with being wrong is a little misdirected. IMHO, the config function returns null when no value is not found or is exactly null. That makes sense to me.

You could also handle this with your own global config function such as the following.

function exceptionalConfig(string $key)
{
    $value = config($key);
    if (is_null($value)) {
        throw new \Exception('Missing config value for '.$key);
    }
    return $value;
}

Here, we are only using a generic Exception type, which prevents me from having something like a MissingCacheDriverConfiguration exception. If I do this inline, I think you may even be able to do something like this.

config('cache.driver', function () {
    throw new MissingCacheDriverConfiguration();
});

While I'm not a huge fan of the above, it makes more sense to me reading this: "Give me the 'cache.driver' config value, but default to throwing a MissingCacheDriverConfiguration exception." Again, I'm not sure if that will actually throw the exception... but the point of clarity remains.

[laurencei]

But that assumes that null is an incorrect answer. There are many cases where null could be considered a valid config item.

So simply looking for a null config item is not the same as no config item found.

What I am saying is should the framework continue to return null on an error? Wouldnt it make more sense to throw an Exception which can be handled however you want? You could even make a default Exception handler that returns null to ensure backwards compatibility.

During development this would seem to help alot. You would be instantly alerted to any issues with your config, rather than having to debug it and getting silent failures...

[Garbee]

I think an exception should be thrown when a config key is not found that is asked for. That is useful information.

However, it should not throw if the key exists and it is null, as that is a valid config entry. Check if the config key exists alone is simple enough.

I don't think another config option to toggle this is necessary. I don't see a good return on having that included when the exception for a non-existent key is a sensible thing to do.

[laurencei]

The only reason I thought to include a strict config mode is to easily allow for backwards comparability.

This way we would keep existing functionality, and people can switch to strict mode when they want, and do testing etc.

[Garbee]

BC isn't a massive concern here IMO. It should be fine making sure the change is known in the release notes for people. If anything, this change is bringing a problem within people's code to their attention which is what should be done.

The thing is the error is very explicit, you are asking for a configuration item that does not exist. That is due to an error in your codebase. We shouldn't say null is wrong, but just if the key itself is missing.

Is there any use-case where asking for a config key that doesn't exist is useful in a program?

[franzliedke]

This is a typical trade-off situation: On one side, things fail gracefully, but some errors can go unnoticed for a while. On the other side, you have explicit errors, but also hard failures.

I do think that BC plays a big role here, tweaking details like this for such a widely-used feature is tough and should not be done lightly when the decision on the trade-off has already been made.

I personally think that the current solution fits well with PHP's dynamic and often very forgiving nature. null will often be interpreted as some form of default or empty value, so it is definitely a sane value.

[Garbee]

I don't think people are dynamically calling config keys very often. I'd actually be interested to know what cases people do think it is appropriate.

Was a conscious decision made here originally to not throw an error or was it just not considered when being created?

null is sane, however, it can also be misleading if you typo the string for the parameter and a check then always results in null by accident instead of getting the requested key. I think the end goal with the request is to have Laravel help prevent more programmer-error occurrences instead of letting them slip by un-noticed.

[kyranb]

Was a conscious decision made here originally to not throw an error or was it just not considered when being created?

@taylorotwell

[AlexDanault]

(Bringing #348 back over here). I think an undefined value IS legit and needs to stay legit. Now, as proposed here, a global switch to force ALL keys to exists is not flexible at all IMHO.

Here's what I am suggesting:

Add two new functions: envOrFail() and configOrFail();

I already have (and rely on) those functions (tough they aren't very clean at the moment) in my projects, because there are some env() and config() calls which do not have a default value and should never be undefined.

The problem with env() and config() as they are is that they return null when the key does not exist. That's alright for some cases, but there are some settings which should never be undefined, otherwise the application stability is at stake.

Used at the proper places, envOrFail() and configOrFail() would at least cut down debuging time (because a typo in a env/config file or env/config call would show up right away) and at best prevent live applications from going berserk because an env/config was forgotten/typoed during deployment (at the hell of trying to debug why something is misbehaving in production).

The functions would behave like firstOrFail(), it'd simply throw an exception if the key is not defined. I think there's a problem with the envOrFail() because it returns null when the value is not defined AND when it is defined to "null" or "(null)". I'd suggest passing "(undefined)" as a default value to the underlying env() call to catch a real undefined value -vs- a value defined as null.

[laurencei]

The problem with env() and config() as they are is that they return null when the key does not exist. That's alright for some cases, but there are some settings which should never be undefined, otherwise the application stability is at stake.

What is a valid situation where you can accept a config value not being set - that you cannot just create it in your config file during development?

Explicit config values ensures a safety net for "application stability".

p.s. I dont have a problem with env() returning null - because you expect times for a ENV variable not to be set (that is the whole point).

But config() is more explicit - there is no benefit of not explicitly having it defined - even if that is at least a null in your config file.

[AlexDanault]

I do have a problem with env() returning null in some cases. But we definitely do not want to touch env() as its behavior is correct. Hence envOrFail(). Having env() return null when you do not expect a null can be disatrous. Validating each of them is not very DRY. I think it would be a nice addition to the framework.

For config(), while I agree that it makes very little sense to allow undefined keys, there's a possibility that the change would be a breaking one. There's certainly a package out there which reads non-existing config keys (and not doing anything with it), and we don't want to find that one out the hard way. Hence configOrFail(), which offloads to the dev the responsibility to use it when appropriate.

[laurencei]

there's a possibility that the change would be a breaking one

Yes - so we can have it in 5.4 and it is acceptable to be breaking.

But you can also just make it configurable:

$strict_config = true;

If that variable is true - throw an exception when the config is not found
If that variable is false - it keeps the current behavior and returns null.
If that variable is not defined (ironic) - it keeps the current behavior and returns null.

That means it should not be a breaking change at all. But you set is to be true in new 5.4 applications, but allow people to turn it off if needed (and/or to save a BC change).

[crynobone]

Would prefer to use something like https://github.com/vlucas/phpdotenv#requiring-variables-to-be-set

[AlexDanault]

@crynobone vlucas' $dotenv->required() is exactly where my suggestion of envOrFail() comes from. But Laravel's env() checks more than just the .env file, it also checks the environment variables. A key not set in the .env file is just as bad as an environment variable not set, which is why envOrFail() catches both.

[crynobone]

A key not set in the .env file is just as bad as an environment variable not set, which is why envOrFail() catches both.

Using envOrFail() would be just as bad, since env() in general sometimes failed especially under certain condition:

• env() returns first value in two laravel apps while doing curl from one to another on the same server framework#16407
• [5.0] Environment variables sometimes return null framework#8191
• .env issue with PHP thread-safe framework#7354

You certainly wouldn't want to have random exception during these scenarios.

[sisve]

Those linked issues are from people that have not cached their configuration file. This is required in environments where one process handles many requests (hhvm, apache in some configurations, etc). php artisan config:cache must be executed during deploys for all versions of Laravel to avoid reading possibly wrong env variables.

vlucas/phpdotenv#76 (comment)

[andjd]

I want to underscore that having another function or a configuration option to get config() to fail on an invalid key would be really helpful. I find that mis-typed config keys are a recurring source of errors that can be difficult to diagnose.

[stephan-v]

Why has this been closed without any comment? I am still running into this issue quite a lot in projects. Having to implement exceptions yourself for this is just stupid.

[thannaske]

This is the idea repo. Consider your idea declined.

Returning null even the config variable does not exist sounds perfectly fine to me.

[sisve]

No, the closed status is a bit weird. It can also means this is already in an internal todo list, that the idea can be submitted in a PR (another example), or that it's a weird state of "open to looking at other ideas".

There's no connection between the open/closed status of an issue, and whether it is a good or bad idea.

[Jimbolino]

For anyone who stumbled here after a google search. This can be fixed by writing your config files like this:

return [
    'system_url' => env('SYSTEM_URL', function () { throw new Exception('SYSTEM_URL is empty'); }),
    'embed_url' => env('EMBED_URL', function () { throw new Exception('EMBED_URL is empty'); }),
];

In the upcoming php8.0 the syntax will be better, because you should be able to throw exceptions when a parameter is expected:
https://php.watch/versions/8.0/throw-expressions

[ClCfe]

hello
a global flag in app.php conf to throw if config key isn't found would be great
Personnally, I always want my config keys to exist

[ClCfe]

Couldn't you just check what the config function returns and then throw an exception yourself?

Some people rely on the default value you can add as the second parameter.

Hum, I see only one reason why people do purposely rely on default value, knowing that key may be undefined:
config file is git ignored

Problem: i can't see why it is a good practice to not put all config in git, given that you can use env() in config file

Why on earth would you do
config('user.domain', 1); without creating the file user.php and/or key "domain":1 inside it ?!

if 2 years later someone creates config/user.php, and defines a key "domain" to be 2, he will break existing code

Does anyone here greps the code before defining a new config? LOL