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

Refactor settings from container #2104

Merged
merged 10 commits into from Mar 7, 2017

Conversation

@codeguy
Copy link
Member

commented Dec 11, 2016

Work in progress. Do not merge yet.

@codeguy codeguy added the Slim 4 label Dec 11, 2016

@codeguy codeguy added this to the 4.0 milestone Dec 11, 2016

@coveralls

This comment has been minimized.

Copy link

commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.6%) to 96.993% when pulling e6ac9e0 on codeguy:feature-refactor-container-settings into 394732e on slimphp:4.x.

@coveralls

This comment has been minimized.

Copy link

commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.5%) to 97.11% when pulling b6c2927 on codeguy:feature-refactor-container-settings into 394732e on slimphp:4.x.

@coveralls

This comment has been minimized.

Copy link

commented Dec 11, 2016

Coverage Status

Coverage increased (+0.01%) to 97.649% when pulling dac7c1f on codeguy:feature-refactor-container-settings into 394732e on slimphp:4.x.

@coveralls

This comment has been minimized.

Copy link

commented Dec 11, 2016

Coverage Status

Coverage decreased (-0.1%) to 97.521% when pulling 7a5c616 on codeguy:feature-refactor-container-settings into 394732e on slimphp:4.x.

@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2016

@akrabat What's your opinion on injecting settings in the constructor? Right now we accept a container instance in the constructor... but that will be unnecessary in 4.x. The container will be optional, so I think we should not ask for it in the constructor, but instead with a setter method setContainer($c).

We could update the constructor to accept settings though like:

$app = new \Slim\App([
    'foo' => 'bar
]);
@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2016

Currently, there are two ways you may use the 3.x constructor:

$app = new App([
    'settings' => [
        // settings
    ],
    // other container items
]);

The array argument will be used to populate a default Slim container. Else, you can pass your own container-interop container into the constructor:

$app = new App($myContainer);

Both scenarios would need to change to the following signature, where the argument is optional and if present, will be merged with the default settings:

$app = new App([
    // settings
]);
@codeguy

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2016

@akrabat I had to merge my latest work from #2102 into this branch. So you can either merge #2102 and then merge this PR, or you can simply merge this PR.

Latest changes in this PR make the App::__construct() method accept only an array of app settings which are then merged into the default settings array. It adds a public interface to interact with app settings array. And it separates settings from the app container entirely. To do this, I added getter/setter methods for app handlers (e.g. not found, not allowed, error, php error).

@coveralls

This comment has been minimized.

Copy link

commented Dec 17, 2016

Coverage Status

Coverage decreased (-1.009%) to 96.631% when pulling f957107 on codeguy:feature-refactor-container-settings into 394732e on slimphp:4.x.

codeguy added some commits Feb 19, 2017

@coveralls

This comment has been minimized.

Copy link

commented Feb 19, 2017

Coverage Status

Coverage decreased (-0.7%) to 96.611% when pulling 1db36bc on codeguy:feature-refactor-container-settings into 5abbab0 on slimphp:4.x.

@codeguy

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2017

Ping @akrabat. Can you review and merge this soon? I just merged in other latest changes from the primary 4.x branch.

@codeguy codeguy changed the title [WIP] Refactor settings from container [Needs Review] Refactor settings from container Feb 19, 2017

@geggleto
Copy link
Contributor

left a comment

👍

}
$this->container = $container;
$this->addSettings($settings);
$this->container = new Container();

This comment has been minimized.

Copy link
@geggleto

geggleto Mar 6, 2017

Contributor

?? I thought we were going to remove the container ;)

This comment has been minimized.

Copy link
@codeguy

codeguy Mar 6, 2017

Author Member

One step at a time :)

@akrabat akrabat merged commit 1db36bc into slimphp:4.x Mar 7, 2017

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.7%) to 96.611%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

akrabat added a commit that referenced this pull request Mar 7, 2017

@akrabat akrabat changed the title [Needs Review] Refactor settings from container Refactor settings from container Sep 16, 2018

@l0gicgate l0gicgate referenced this pull request Aug 1, 2019

Merged

Slim 4 Release #2769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.