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

Configure read default #7456

Closed
wants to merge 2 commits into from
Closed

Configure read default #7456

wants to merge 2 commits into from

Conversation

jadb
Copy link
Contributor

@jadb jadb commented Sep 26, 2015

No description provided.

@jadb jadb added this to the 3.1.1 milestone Sep 26, 2015
@markstory
Copy link
Member

Doesn't this encourage adding default values all over the app? Shouldn't we encourage default config files instead?

@ravage84
Copy link
Member

Having the option does not seem to be such a bad thing...

@jippi
Copy link
Contributor

jippi commented Sep 27, 2015

👎 from personal experience, having defaults scattered all over the place is sad

if you insist on it use Configure::read('meh') ?: 'meh';

@markstory
Copy link
Member

Having the option does not seem to be such a bad thing...

In isolation I agree, but in practice it leads to code that is hard to understand. Think of the mess you'd be in if the following happened:

// In one place
Configure::read('Security.salt', 'abc123');

// In another
Configure::read('Security.salt', '123abc');

Having a centralized place for default values (like a default config file) makes it impossible for configuration values to diverge.

@jadb
Copy link
Contributor Author

jadb commented Sep 27, 2015

Am happy either way.

The raised concerns make perfect sense when put in place to enforce good practices.

@jadb jadb closed this Sep 27, 2015
@ADmad ADmad deleted the configure-read-default branch September 27, 2015 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants