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

4.x - Configure the application via the container? #2778

Closed
mnapoli opened this issue Aug 5, 2019 · 10 comments
Closed

4.x - Configure the application via the container? #2778

mnapoli opened this issue Aug 5, 2019 · 10 comments
Labels

Comments

@mnapoli
Copy link
Contributor

mnapoli commented Aug 5, 2019

Following #2770 I am working on the PHP-DI / Slim integration and I have a question: was it considered to use the container to provide all the services needed to initialize the application? (I mean the route collector, callable resolver, router, etc)

Because at the moment everything is configured via a global static locator (the AppFactory class). Given everything is static in there, it is not compatible with the dependency injection (I cannot inject things coming from the container, or using the container). It also means that if I create 2 applications (for example in tests) the configuration from one app will bleed into the next one.

If we made everything come from the container, then building an application would be simpler:

$app = new App($container);

Then if you want to overload anything, you can do it by defining the service in the container.

Such behavior would also be more in line with what other frameworks are doing, which would follow the principle of least astonishment.

Anyway, maybe you've already considered it? Would it be too late to change? Maybe not?

@adriansuter
Copy link
Contributor

To me this seems plausible. But we have to pay attention, as Slim 4 might be used without container.

@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 5, 2019

Absolutely, and maybe a "default" container could be a simple PHP array? This is, after all, a container, even though a poor one. (and I'm not talking about reproducing Pimple at all, just a plain simple array with objects, not closures)

But again most users that don't use a container and want to use the defaults will keep doing new App() and that's good for them.

@l0gicgate
Copy link
Member

You’ll just have to create your own factory or extend ours if you want to retain the PSR-17 detection.

As for everything else it can be worked around by extending the App as you were doing before.

Like @adriansuter said, we want to leave the possibility of not having to use a container at all.

@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 5, 2019

You’ll just have to create your own factory or extend ours if you want to retain the PSR-17 detection.

I'm not sure what you mean or how it relates to this issue to be honest. Maybe I missed something?

As for everything else it can be worked around by extending the App as you were doing before.

That doesn't address the problems I've raised around the global static locator that is now included in Slim 4 AFAICT.

Like @adriansuter said, we want to leave the possibility of not having to use a container at all.

Agreed, what I suggest should be compatible with that.

@l0gicgate
Copy link
Member

You need a way to provide Request and Response for your DI bridge to work. Hence why I’m saying you might need to implement your own set of PSR17 factories in order to be able to inject them in the container (or use middleware) for your Invoker to work.

Honestly, I don’t really feel as this is a Slim architecture problem. The fact that your repo was tightly coupled to Slim 3’s internal mechanisms is the real problem. You’ll just have to engineer things differently to make it work.

@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 6, 2019

You need a way to provide Request and Response for your DI bridge to work.

Oh wait I don't understand why this is necessary?

I don't want to store the request and response in the container as those are request-scoped objects, not stateless services. Could you detail why you think those need to be in the container? Maybe there is a quiproquo in our discussion ;)

Honestly, I don’t really feel as this is a Slim architecture problem. The fact that your repo was tightly coupled to Slim 3’s internal mechanisms is the real problem. You’ll just have to engineer things differently to make it work.

Again, I don't know what you mean? What was coupled?

The main thing that was coupled was that PHP-DI had to provide the exact list of services that Slim expected to find in the container. That was a coupling constraint that was set by Slim, not PHP-DI. I had no choice to provide those services, because Slim forced any container to provide them. But maybe you are not talking about this?


Side note: I think we are not talking about the same thing here. Maybe it might be worth that we both clarify what we are talking about.

Here is my part:

  • the AppFactory class is a static container, this is global state
  • global state is usually something we want to avoid for several reasons (I can detail them if necessary)
  • we could move away from global state by using the DI container to provide services (after all, that's exactly a container's job)
  • we could do that while keeping the container optional

If that's clearer for you let me know. Feel free to clarify as well the topic you were mentioning.

@l0gicgate
Copy link
Member

l0gicgate commented Aug 6, 2019

the AppFactory class is a static container, this is global state

AppFactory is simply a helper class used to create an App, it’s not used to manage any state anywhere in the app itself so I’m not sure what you’re referring to here. If so, please reference where it’s being used as global state. You can create an App without using AppFactory, they’re not coupled.

To me "global state" is something that would be used and shared by multiple components (like a container). There are no components referencing or depending on AppFactory in the codebase.

we could move away from global state by using the DI container to provide services (after all, that's exactly a container's job)

we could do that while keeping the container optional

If we use the DI container to provide services internally (ie.: ResponseFactory, ServerRequestCreator, MiddlewareDispatcher, etc.) then that means we are depending on it for the App to function, which also means it cannot be optional.

@l0gicgate l0gicgate changed the title V4: configure the application via the container? 4.x - Configure the application via the container? Aug 7, 2019
@adriansuter
Copy link
Contributor

How about a method like AppFactory::createFromContainer() which requires to have a container and which gets the services like callable resolver, etc. from the container.

The method gets optionally the container keys of the services.

That way, a user can use a pre-compiled container containing all the custom slim services.

@l0gicgate
Copy link
Member

@adriansuter that's a good idea. @mnapoli any thoughts on that?

@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 10, 2019

That is a very good idea!

I've opened #2793 to propose an implementation.

bednic pushed a commit to bednic/Slim that referenced this issue Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants