-
Notifications
You must be signed in to change notification settings - Fork 10
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
Clarification: null "will be passed" for a "service that does not exist" #66
Comments
Gosh, I must admit I absolutely don't remember what triggered this. Reading this in retrospect, it seems weird, indeed. I remember that at some point, I was wondering how a service provider could inject a HTTP middleware at a given point in the stack of middlewares (for instance, a middleware doing error handling needs to come first in the stack, while CORS middleware can come later, etc...) What is haunting me however, is how we can resolve this "middlewares list" problem in a compiled fashion. |
We spent considerable time thinking about this in one of my previous jobs, and our conclusion, finally, was that, this is such a small problem, and the potential solutions all have serious drawbacks, that it was just not really worth bothering with. Numeric priority: fragmented, unreadable code - you'd likely need to var_dump your middleware stack to make sure it ends up calculated exactly the way you want it. Even then, somebody else could change something and break your project. Topological sorting: more reliable, but a topological sort is kind of a high price to pay, at every request, just to establish your middleware stack. Using extensions and hand-holding (assuming here class MyMiddlewareProvider implements ServiceProviderInterface
{
public function getExtensions()
{
return [
"middleware" => function (ContainerInterface $c, array $middleware) {
$pos = array_search(PreviousMiddleware::class, $middleware);
return array_splice($middleware, $pos, 0, MyMiddleware::class);
}
];
}
} I don't even know if I wrote that correctly, haha. No matter what you do, it's going to get fragmented, and in the end, what we opted for was a simple service override in the project provider, which would replace the whole list of middleware - there is typically, what, 10 middlewares in a project, tops? Just get over it and type them out. You'll get human-readable code that does exactly what you think it's going to do. It doesn't seem like a problem worth solving, unless maybe you're dreaming about auto discovery and all that jazz. Either way, it seems like more of a framework problem than a DI container problem? In a framework context, if I had to solve this for the sake of modularity or auto-discover, I would probably have something like a class MyMiddlewareProvider implements ServiceProviderInterface
{
public function getExtensions()
{
return [
MiddlewareRegistry::class => function (ContainerInterface $c, MiddlewareRegistry $registry) {
$registry->addMiddlewareBefore([PreviousMiddleware::class], MyMiddleware::class)`);
}
];
}
} I think there are more than enough ways to solve this problem without this PSR needing to do anything specific to solve it. Would you agree that this is probably outside the scope of this PSR? |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, wrong thread, will repost in #65 |
I'm changing the wording in that phrase to:
It's relevant to the example, although the requirement was not. I'm closing this issue, as the original question has been answered. (I'm sure we'll talk more about the other topics.) |
I remember I had this discussion with @mnapoli a few years back and he was also advocating to put this out of scope of this PSR. I guess I'm in minority here, but it is indeed a pet peeve of mine. I'm dreaming of something that (in the end) would have autodiscovery of service providers. In my ideal world, you would "composer install" a DI container and a bunch of service providers, and shazaam! everything would be set up automatically. |
I don't think you're the minority, really - if anything, I'm the minority. 😅 Ideally, I don't want frameworks to magically configure or do anything at all - I want them to implement things that are difficult to implement, but I think they should stay away from anything that's already easy and avoid adding complexity trying to make easy things easier. DI containers solve the hard problems for me already. Interoperable providers on top of that would take 90% off the setup work and make it more than easy enough, without adding any complexity you wouldn't have needed to add yourself anyway. Yet, I think most people would prefer what you're describing - if you could just But what do I know, maybe you'll figure out a great way to do it that's undeniably more convenient and safe, without adding much complexity at all - just because I don't have the imagination to come up with something like that doesn't mean it's not possible, and this PSR shouldn't get in the way of that. 😄✌️ |
@moufmouf in the "Entry Extension" section of the README, I found the following requirement:
service-provider/README.md
Line 284 in a55e717
While this isn't written in specification language (e.g. "SHOULD") this might create some confusion.
It is of course predicated on the preceding example of optional service dependency:
But the language here seems problematic: "for a service that does not exist".
I believe the intention with this example was to demonstrate compatibility with the use of "NULL services" - meaning, a service that has been explicitly registered as NULL, and as such does exist?
If a service "does not exist", meaning it isn't registered at all, I don't believe most existing DI containers would even attempt to invoke any extensions? So this requirement would create an interoperability problem.
As said though, this isn't written in specification language, so it was probably just meant as an example, and not as a requirement that DI containers attempt to invoke extensions for services that "do not exist"?
(I just want to make sure I understand the intention here, as I am working on extracting everything in "loose form" from the README into the PSR and meta document drafts.)
The text was updated successfully, but these errors were encountered: