Remove disabling shared ResponseInterface #547
Conversation
This feature is not in the PSR-11 spec so we can't relay on it. This provides another solution by requiring different ResponseInterfaces where needed.
There was a problem hiding this comment.
PSR-11 specification doesn't say either that returnet instance should be exactly the same. Maybe implementing "shared by default" is then not a good approach?
https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-11-container.md#112-reading-from-a-container
I think as described in PRS-11 spec it depends on the implementation and by default service manager shares all service. Other containers (like pimple) can be configured either to share or not the services.
As we deliver zend-pimple-config library it would be nice to have there the same functionality as we have in servicemanager, so configure it should configure pimple using servicemanager configuration that the final container behaves the same as servicemanager with the same configuration.
|
So if someone uses MyAwesomeContainer, and that container shares by default and you can't disable it, it means that that container would be incompatible with Expressive 3.0 without this PR. With this PR all PSR-11 containers are compatible and the application should work as expected.
I agree. But since we can add functionality to zend-pimple-config as we own that package. But we can't do that for MyAwesomeContainer as we don't have any influence on it or even should try to force it. |
Have you tested expressive 3.0-dev with container which has DISABLED sharing services? Please note that servicemanager and current pimple config shares services by default, but it's not behaviour specified by PSR-11.
This is why I'd like to have tests in separate repository and require this behaviour to be compatible with expressive. The same we do with routers/template renderers, we require some behaviour to be compatible with expressive. |
We cannot return just `ServerRequestFactory::fromGlobals` or `[ServerRequestFactory::class, 'fromGlobals']` as not all containers allow vanilla PHP callable services. Instead, we wrap it in an anonymous function here, which is allowed by all containers tested at this time.
The `ErrorResponseGenerator` service should be in the `Zend\Expressive\Middleware` namespace, not in the `Zend\Stratigility\Middleware` namespace.
There was a problem hiding this comment.
This looks like a nice solution!
The only change I'd make is to NOT define the response interface extensions. They're not technically needed.
Every container I've surveyed caches services based on service name. Thus, even if multiple services resolve to the same factory, requesting any of them will invoke the factory to produce a new instance. It's an effective way to produce non-shared instances.
This means, of course, "virtual services" for these response instances (i.e., FQCNs that do not resolve to an actual class), but I think that's a reasonable tradeoff here.
Remove disabling shared ResponseInterface Conflicts: src/ConfigProvider.php test/ConfigProviderTest.php
Their FQCNs do not resolve to actual classes/interfaces. As discussed with and requested by @weierophinney
| ResponseInterface::class => Container\ResponseFactory::class, | ||
| Router\DispatchMiddleware::class => Container\DispatchMiddlewareFactory::class, | ||
| Router\PathBasedRoutingMiddleware::class => Container\RouteMiddlewareFactory::class, | ||
| RouterResponseInterface::class => Container\ResponseFactory::class, |
There was a problem hiding this comment.
I wonder if we should make all of these virtual responses match the service they correspond to:
NotFoundMiddlewareResponseInterfaceRouteMiddlewareResponseInterface
| use Psr\Container\ContainerInterface; | ||
| use Psr\Http\Message\ResponseInterface; | ||
| use Zend\Expressive\Middleware\NotFoundMiddleware; | ||
| use Zend\Expressive\NotFoundResponseInterface; |
There was a problem hiding this comment.
Should be Zend\Expressive\Response\NotFoundResponseInterface (or the name I proposed; main problem is namespace).
| use Psr\Http\Message\ResponseInterface; | ||
| use Zend\Expressive\Router\PathBasedRoutingMiddleware; | ||
| use Zend\Expressive\Router\RouterInterface; | ||
| use Zend\Expressive\RouterResponseInterface; |
There was a problem hiding this comment.
Should be Zend\Expressive\Response namespace, with appropriate name.
test/ConfigProviderTest.php
Outdated
| use Zend\Expressive\Middleware; | ||
| use Zend\Expressive\MiddlewareContainer; | ||
| use Zend\Expressive\MiddlewareFactory; | ||
| use Zend\Expressive\NotFoundResponseInterface; |
There was a problem hiding this comment.
Should be under Zend\Expressive\Response namespace.
test/ConfigProviderTest.php
Outdated
| use Zend\Expressive\NotFoundResponseInterface; | ||
| use Zend\Expressive\Router\DispatchMiddleware; | ||
| use Zend\Expressive\Router\PathBasedRoutingMiddleware; | ||
| use Zend\Expressive\RouterResponseInterface; |
There was a problem hiding this comment.
Should be under Zend\Expressive\Response namespace.
| use Psr\Http\Message\ResponseInterface; | ||
| use Zend\Expressive\Container\NotFoundMiddlewareFactory; | ||
| use Zend\Expressive\Middleware\NotFoundMiddleware; | ||
| use Zend\Expressive\NotFoundResponseInterface; |
There was a problem hiding this comment.
Should be under Zend\Expressive\Response namespace.
| use Zend\Expressive\Container\RouteMiddlewareFactory; | ||
| use Zend\Expressive\Router\PathBasedRoutingMiddleware; | ||
| use Zend\Expressive\Router\RouterInterface; | ||
| use Zend\Expressive\RouterResponseInterface; |
There was a problem hiding this comment.
Should be under Zend\Expressive\Response namespace.
The disable shared service feature is not in the PSR-11 spec so we can't rely on it.
This PR provides another solution by requiring different ResponseInterfaces where needed.
See: #543 (comment)
This PR would make the following PR's not needed:
zendframework/zend-servicemanager#248
zendframework/zend-pimple-config#6