Skip to content

Conversation

nexxai
Copy link
Contributor

@nexxai nexxai commented Aug 12, 2024

Oussama posted a tweet today about the Env::getOrFail() method, which allows a developer to ensure that if an environment variable is missing, the app will hard fail. This can be extremely useful in cases where the env var is an API key and you need to ensure that it's been set correctly, and based on the replies to the tweet, it seems like I'm not the only one who didn't know it exists but would find it helpful.

I wanted to add this helper method so that I don't have to import the Illuminate\Support\Env class in every config file where I have API keys being loaded.

Following the current convention, where env() actually proxies the Env::get() method, I created envOrFail() which proxies the Env::getOrFail() method. I'm happy to rename this to something else if requested.

If this PR is accepted, I will also add an entry in the docs publicizing this helper.

@rodrigopedra
Copy link
Contributor

As env() is recommended to be used only in configuration files, why not just call Env::getOrFail() directly?

@nexxai
Copy link
Contributor Author

nexxai commented Aug 12, 2024

As env() is recommended to be used only in configuration files, why not just call Env::getOrFail() directly?

As I mentioned in my original post, I prefer not importing Illuminate\Support\Env in each config/...php file just to get access to the getOrFail() method. I'd prefer an equivalent helper to env() that simply enforces the requirement that the environment variable actually exists.

Since that is the only difference between Env::get() / env() and Env::getOrFail() / envOrFail(), I don't think there should be a problem. They would otherwise be used in the same way (only in config files, etc.)

@nexxai nexxai changed the title [11.x] Adding envOrFail helper [11.x] Adding envOrFail() helper Aug 12, 2024
@rodrigopedra
Copy link
Contributor

As I mentioned in my original post, I prefer not importing Illuminate\Support\Env in each config/...php file

You prefer not to import it on each config file, I prefer not to have on more user-defined function on the global namespace, regardless if I use it or not.

But that is a matter of preference.

Nonetheless, as this a new helper, added to the global namespace, I believe it should be sent to master, right?

So it won't conflict in case someone already has a helper named the same in their codebase.

@nexxai
Copy link
Contributor Author

nexxai commented Aug 12, 2024

The if (! function_exists('envOrFail'))... makes sure that if someone has already defined it in their codebase, this PR will not override their functionality.

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

@nexxai
Copy link
Contributor Author

nexxai commented Aug 12, 2024

Hey @taylorotwell I apologize for the late message; I was just out getting lunch and was talking to @jasonmccreary about this. He had actually suggested instead of a separate helper method, I add another (defaulted) argument to the existing env() method signature instead of creating a whole other method. I was going to submit an update to this PR when I got home, but it looks like you closed it before I had a chance.

The API we discussed was something like: function env($key, $default = null, $fail = false)

If I was to submit a PR in that form, would it be more acceptable for you?

@rodrigopedra
Copy link
Contributor

The if (! function_exists('envOrFail'))... makes sure that if someone has already defined it in their codebase, this PR will not override their functionality.

Only if their code base is autoloaded by composer before the framework's. Otherwise, it will break.

And even then, if the framework has a different implementation and this helper starts to be largely adopted by itself or by 3rd-party packages, then you end up with unexpected behavior.

Just to be clear, I have a preference for not adding an extra helper with such a narrow use-case that can be easily solved by calling Env::getOrFail(), but I don't oppose to adding it. I just think it is better to be done on a major version to avoid any pitfalls.

If this, or the alternate extra parameter to the env() helper suggestion, get added to the next version, I will read the upgrade guide, as usual, and handle any changes needed by then.

As a workaround, you can either add this helper to your project's codebase, or pair the env() helper with the throw_unless helper.

// this will fail with a missing API_KEY
throw_unless(env('API_KEY')); 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants