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

Remove container #2290

Merged
merged 4 commits into from
Nov 3, 2017
Merged

Remove container #2290

merged 4 commits into from
Nov 3, 2017

Conversation

akrabat
Copy link
Member

@akrabat akrabat commented Aug 13, 2017

Container is now completely optional. App now creates its own request and response in run() and the rest of the objects in DefaultServicesProvider were already defined within App.

Also removed the magic __call method on App that allows access to the container as there's no need for it.

Note that Pimple is a dev dependency for testing routing, CallableResolver and DeferredCallable.

AppTest has also been tidied a little.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 94.798% when pulling b12b87d on akrabat:remove-container into ee32eb6 on slimphp:4.x.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 94.798% when pulling b12b87d on akrabat:remove-container into ee32eb6 on slimphp:4.x.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 94.798% when pulling 1a7e068 on akrabat:remove-container into ee32eb6 on slimphp:4.x.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 94.798% when pulling 0706f07 on akrabat:remove-container into ee32eb6 on slimphp:4.x.

@akrabat
Copy link
Member Author

akrabat commented Aug 13, 2017

Ping @mnapoli, @geekish, @moufmouf for thoughts as you showed interest in the original issue about this.

Key thing is that Slim no longer has any concept of default services in the container. i.e. the container is just for the user's code.

@moufmouf
Copy link

It makes complete sense to me. 👍

@mnapoli
Copy link
Contributor

mnapoli commented Aug 13, 2017

That's good news!

and the rest of the objects in DefaultServicesProvider were already defined within App.

I'm not sure I understand this part: can "core" services of Slim still be overridden? Is Slim doing something like $router = $container->has('router') ? $container->get('router') : $this->buildDefaultRouter() ? (this is pseudo-code)

@Sam-Burns
Copy link
Contributor

I'd suggest pinning to Pimple 3.1 in dev at a minimum, to try and fix the Travis warning.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 94.798% when pulling 8c295e3 on akrabat:remove-container into ee32eb6 on slimphp:4.x.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.8%) to 94.798% when pulling 0b16b20 on akrabat:remove-container into ee32eb6 on slimphp:4.x.

@akrabat
Copy link
Member Author

akrabat commented Aug 14, 2017

and the rest of the objects in DefaultServicesProvider were already defined within App.

I'm not sure I understand this part: can "core" services of Slim still be overridden? Is Slim doing something like $router = $container->has('router') ? $container->get('router') : $this->buildDefaultRouter() ? (this is pseudo-code)

The App has methods like: set/getRouter(), set/getNotFoundHandler() etc.

@mnapoli
Copy link
Contributor

mnapoli commented Aug 14, 2017

@akrabat OK so a question that's maybe a bit out of scope here but just to get it right: when using a container it might make sense to let the container create the "Application" class (the container would call setRouter(), etc. with the correct dependencies)?

@akrabat
Copy link
Member Author

akrabat commented Aug 14, 2017

OK so a question that's maybe a bit out of scope here but just to get it right: when using a container it might make sense to let the container create the "Application" class (the container would call setRouter(), etc. with the correct dependencies)?

@mnapoli Yes. If you need to customise the App's dependencies, then letting the app, then letting the container create the App object would probably make sense.

@mnapoli
Copy link
Contributor

mnapoli commented Aug 14, 2017

Awesome great then!

@bnf
Copy link
Contributor

bnf commented Aug 16, 2017

Slim\Collection and Slim\Interfaces\CollectionInterface seem to be unused with this change applied. May those be removed as well?

@geggleto
Copy link
Member

@akrabat this PR needs to be rebased.

@akrabat akrabat mentioned this pull request Sep 24, 2017
It's not used as process() catches everything.
Container is now completely optional. App now creates its own request
and response in `run()` and the rest of the objects in
`DefaultServicesProvider` were already defined within `App`.

Also removed the magic `__call` method on `App` that allows access to
the container as there's no need for it.

Note that Pimple is a dev dependency for testing routing,
`CallableResolver` and `DeferredCallable`.

AppTest has also been tidied a little.

Closes #2288
@akrabat
Copy link
Member Author

akrabat commented Nov 3, 2017

Rebased to tip of 4.x

These were only being used by Container which is now gone.
@akrabat
Copy link
Member Author

akrabat commented Nov 3, 2017

Slim\Collection and Slim\Interfaces\CollectionInterface seem to be unused with this change applied. May those be removed as well?

Yes, you're right @bnf. I've removed them.
Thanks!

@coveralls
Copy link

Coverage Status

Coverage increased (+1.9%) to 94.872% when pulling 8385ca7 on akrabat:remove-container into 0dab1d1 on slimphp:4.x.

@silentworks silentworks merged commit 6e1d776 into slimphp:4.x Nov 3, 2017
@akrabat akrabat added this to the 4.0 milestone Nov 26, 2017
@filips123
Copy link

How can we now use dependency injection for tools like Monolog?

@akrabat
Copy link
Member Author

akrabat commented Oct 19, 2018

How can we now use dependency injection for tools like Monolog?

Slim 4 accepts provision of your own PSR-11 compatible container and will use it to resolve actions, etc. Hence you now have more flexibility and can bring your choice of container to your Slim project.

@filips123
Copy link

@akrabat So I could set my own container, include dependencies (like Monolog) into it, and then access dependencies with $app->container->dependency?

This could be in the documentation for v4.

@akrabat
Copy link
Member Author

akrabat commented Oct 19, 2018

Technically, you'd have to pass your container into your action's constructor to have a $this->container available, but that's the general idea.

Docs for v4 are barely started.

@akrabat akrabat deleted the remove-container branch April 18, 2019 06:07
@l0gicgate l0gicgate mentioned this pull request Apr 25, 2019
@l0gicgate l0gicgate mentioned this pull request Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants