search

psecio / gatekeeper

Gatekeeper: An Authentication & Authorization Library
366 stars 23 forks source link

$envPath in loadConfig #37

Open Swader opened 8 years ago

Swader commented 8 years ago

I suggest moving the check

        $envPath = ($envPath !== null) ? $envPath : getcwd();

from the loadConfig method to the loadDotEnv method, and throwing a proper exception if the path was specified but loading failed. That calls for an exception, rather than just false, because something unexpected occurred. It's not a case for a soft fallback to false. Took me a while to debug what was happening as I encountered this.

I believe the loadDotEnv method is a better home for this check, considering that's the only method in which the actual parsing of the file and loading of the config values actually happens.

I realize, however, that this would be a BC break.

Thoughts? Should I submit a PR anyway?

enygma commented 8 years ago

I think throwing the exception is okay for a break like this. It would only throw if the file wasn't found - a negative case - so I'm good with this change.

Swader commented 8 years ago

Right, but the exception isn't specific and debugging it is hard. Could we at least have it throw out completely or return an error object of some kind? Rather than catching it and returning false? False really isn't helpful in debugging.