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 the application via a container #2793

Merged
merged 8 commits into from Aug 11, 2019

Conversation

@mnapoli
Copy link
Contributor

commented Aug 10, 2019

This is an implementation for what we discussed in #2778.

The idea is to simplify how to configure the application when using a PSR-11 container.

Here is an example with PHP-DI, using the default services from Slim:

$container = new DI\Container;

$app = AppFactory::createFromContainer($container);

Here is an example when we want to overload one service:

$container = new DI\Container;
$container->set(CallableResolverInterface::class, $customInstance);

$app = AppFactory::createFromContainer($container);

(in reality I would recommend using a config file, but the example here is simpler)

instead of the current way of doing it:

$container = new DI\Container;

AppFactory::setContainer($container);
AppFactory::setCallableResolver($customResolver);
$app = AppFactory::create($container);

The advantages:

  • AppFactory is no longer storing global state, meaning subsequent calls to AppFactory::create() will not be affected by any previous call to AppFactory
  • containers can come pre-configured for Slim

The 2nd point is interesting for PHP-DI: the bridge could provide a pre-configured container for Slim. Users would just have to call AppFactory::createFromContainer($container), which is pretty cool.

@coveralls

This comment has been minimized.

Copy link

commented Aug 10, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 691878f on mnapoli:2778-configure-app-via-container into a62257e on slimphp:4.x.

@mnapoli mnapoli referenced this pull request Aug 10, 2019
0 of 2 tasks complete
@l0gicgate
Copy link
Contributor

left a comment

This is great, thank you for this contribution. I would like to mention that I’d like to work closely with third-party library devs like you to make Slim flexible and sensical in terms of approaching different problems. I highly value your input @mnapoli

tests/Factory/AppFactoryTest.php Outdated Show resolved Hide resolved
tests/Mocks/MockContainer.php Outdated Show resolved Hide resolved

@l0gicgate l0gicgate requested a review from adriansuter Aug 11, 2019

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

Looks good to me. Please have a quick look before I merge @adriansuter!

@adriansuter

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

@l0gicgate Can be merged.

We need to make sure that this possibility gets its way into the documentation.

@l0gicgate l0gicgate added the Slim 4 label Aug 11, 2019

@l0gicgate l0gicgate added this to the 4.2.0 milestone Aug 11, 2019

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

@adriansuter

We need to make sure that this possibility gets its way into the documentation.

See slimphp/Slim-Website#411

@l0gicgate l0gicgate merged commit b8d2006 into slimphp:4.x Aug 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@mnapoli mnapoli deleted the mnapoli:2778-configure-app-via-container branch Aug 11, 2019

@l0gicgate l0gicgate referenced this pull request Aug 20, 2019
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.