Register the default services from a Provider. #1745
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I agree having the services registered in the container class looked very strange at first glance. |
| * | ||
| * @param Container $container | ||
| */ | ||
| public function register(PimpleContainer $container) |
There was a problem hiding this comment.
Please remove the type hint. Any DIC that uses array access with closures would in theory work with this...
There was a problem hiding this comment.
Wouldn't it be better to code against ContainerInterface instead
Additionally check for services with that interface instead of ArrayAccess?
if (!$container->has('environment')) {
}There was a problem hiding this comment.
@juliangut I ported the service code exactly as it was previously in the container.
I don't disagree with the idea; I just think that it's a bit outside the scope of that PR.
…ependency on Pimple.
|
Thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By splitting the default Slim Container in 2 (The
Containerand its associatedService Providerto register services) this allows one to use a custom (Pimple-based) container and still auto-register all required service for Slim.It does not solve the more generic problem of having a reusable provider (because container-interop does not have a "setter" method to register service) but it is, I believe, one step in the right direction. In other words, this won't work more for general-purpose container, but it at least allows any Pimple container to register Slim services without having to have a bridge to the SlimContainer.