Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance(get): Allow setting a default value on Config::get() #13

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Log1x
Copy link
Member

@Log1x Log1x commented Apr 14, 2020

No description provided.

@coveralls
Copy link

coveralls commented Apr 14, 2020

Pull Request Test Coverage Report for Build 51

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 47: 0.0%
Covered Lines: 19
Relevant Lines: 19

💛 - Coveralls

Copy link
Collaborator

@austinpray austinpray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some failing test cases 5244479

Co-Authored-By: Austin Pray <[email protected]>
@retlehs retlehs requested a review from QWp6t January 26, 2024 16:36
@tangrufus
Copy link
Member

tangrufus commented Jan 30, 2025

Discussion: If Config::get() returns default values, do we still need UndefinedConfigKeyException?

In my opinion, since we always have a default value ($default = null), we never need to throw UndefinedConfigKeyException.
If users really need to check whether a key is defined in the configMap, we can add a Config::has() method.

I vote for something similar to https://github.com/laravel/framework/blob/4a74d20f8d960b38d75dd525946e893c75a94dbc/src/Illuminate/Config/Repository.php#L33-L58


But, see

I throw a `UndefinedConfigKeyException` when a user attempts to fetch a value with `Config::get(k)` that doesn't exist in `Config::$configMap`. I do this because the control flow is nice and `return null` is not working because null is a viable configuration value. We don't use `Config::get(k)` anywhere but it is a part of the API because I can see users needing it.

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.

4 participants