PHP-DI / Slim-Bridge

PHP-DI integration with the Slim framework
http://php-di.org/doc/frameworks/slim.html
MIT License
176 stars 38 forks source link

Reordered $resolvers #83

Closed githubjeka closed 1 year ago

githubjeka commented 1 year ago

Now https://github.com/PHP-DI/Slim-Bridge/blob/master/src/Bridge.php#L44

         $resolvers = [
            // Inject parameters by name first
            new AssociativeArrayResolver(),
            // Then inject services by type-hints for those that weren't resolved
            new TypeHintContainerResolver($container),
            // Then fall back on parameters default values for optional route parameters
            new DefaultValueResolver(),
        ];

The code

$app->post('/', [MyController::class, 'run'])

final class MyController
{
    public function run(\App\Model\RpcRequest $request): Response
    {
        // ...
    }
}

throws

TypeError
Argument 1 passed to App\MyController::run() must be an instance of \App\Model\RpcRequest, instance of Slim\Psr7\Request given

This looks more like a bug than consistent behavior, even though it has been documented. It is possible that this will lead to broken backward compatibility :/

mnapoli commented 1 year ago

Yeah that sounds a lot like a breaking change 😕

But I'm confused here, App\Model\RpcRequest $request comes from your container? Because it sounds a lot like state (request object that depends on the current request being processed), which shouldn't be in a container (should only contain services/stateless objects).

githubjeka commented 1 year ago

App\Model\RpcRequest could be a valid dto that has been checked and set in container on middleware level.

mnapoli commented 1 year ago

That DTO contains state. One great piece on that topic: https://igor.io/2013/03/31/stateless-services.html

Anyway, that's not the core of the topic here, but slightly related: I think the order of the resolvers was planned intentionally like that because:

I don't think we'll change this (at least for the use case discussed here).

githubjeka commented 1 year ago

Thanks for the clarification and your time.