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

Added configurable default value to Configure::read() #10890

Merged
merged 2 commits into from Jul 13, 2017

Conversation

Projects
None yet
6 participants
@h-moriya
Contributor

h-moriya commented Jul 10, 2017

default value to Configure::read()

Configure::read('key','none'); = Configure::read('key') ?? 'none';

It works the same as env ()

markstory and others added some commits Jul 10, 2017

@dereuromark

This comment has been minimized.

Member

dereuromark commented Jul 10, 2017

Sounds reasonable given the other reader methods also have this default argument.

@dereuromark dereuromark added this to the 3.4.11 milestone Jul 10, 2017

@cleptric

This comment has been minimized.

Member

cleptric commented Jul 10, 2017

What use case do you see for this feature? Do you end up missing a config option on a given case?

@dereuromark

This comment has been minimized.

Member

dereuromark commented Jul 10, 2017

For BC reasons this could however go into 3.5 better
As people could have extended the Configure class and call e.g. MyConfigure::read() which would then throw notices due to the signature change.
Very theoretical, though.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 10, 2017

Codecov Report

Merging #10890 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #10890      +/-   ##
============================================
- Coverage     94.94%   94.93%   -0.01%     
  Complexity    12137    12137              
============================================
  Files           422      422              
  Lines         30168    30168              
============================================
- Hits          28643    28641       -2     
- Misses         1525     1527       +2
Impacted Files Coverage Δ Complexity Δ
src/Core/Configure.php 96.47% <100%> (ø) 39 <2> (ø) ⬇️
src/Cache/Engine/FileEngine.php 85.47% <0%> (-1.12%) 72% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bf71a5...585d509. Read the comment docs.

@markstory markstory changed the base branch from master to 3.next Jul 10, 2017

@markstory

This comment has been minimized.

Member

markstory commented Jul 10, 2017

Moved to 3.next

@dereuromark dereuromark modified the milestones: 3.5.0, 3.4.11 Jul 10, 2017

@markstory markstory self-assigned this Jul 12, 2017

@ravage84

Lovely, that was on my TODO list for so long. Thanks for taking care of.

@@ -16,4 +16,4 @@
// @license https://opensource.org/licenses/mit-license.php MIT License
// +--------------------------------------------------------------------------------------------+ //
////////////////////////////////////////////////////////////////////////////////////////////////////
3.4.9
3.4.10

This comment has been minimized.

@ravage84

ravage84 Jul 12, 2017

Member

You don't need to do that. This is taken careof when it get's released.

This comment has been minimized.

@dereuromark

dereuromark Jul 12, 2017

Member

this is because of branch change. can be disregarded.

markstory added a commit to cakephp/docs that referenced this pull request Jul 13, 2017

@markstory markstory merged commit 65c9018 into cakephp:3.next Jul 13, 2017

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codecov/patch 100% of diff hit (target 94.94%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +5.05% compared to 3bf71a5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
stickler-ci No lint errors found.
@markstory

This comment has been minimized.

Member

markstory commented Jul 13, 2017

Thanks @h-moriya 👏

@h-moriya h-moriya deleted the h-moriya:config branch Jul 13, 2017

markstory added a commit to cakephp/docs that referenced this pull request Jul 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment