slim-jwt-auth
slim-jwt-auth copied to clipboard
tuupola
Configurable response factory
Background
This middleware uses tuupola/http-factory to create a new response in case an error occurs.
The new response is then passed to the error callback function.
The mentioned http factory detects one of several supported installed PSR-7 libraries and uses one to create the response.
The Issue
After installing a package that consequently installed it's own dependency (zendframework/zend-diactoros PSR-7 implementation), which is one of the supported PSR-7 implementations tuupola/http-factory will detect and use, my error callback caused error, because it was expecting a Slim response implementation (Slim\Http\Response), but got the Zend implementation (Zend\Diactoros\Response) instead.
While this was not a grave issue and was solved by using bare Psr\Http\Message\ResponseInterface, there is currently no way around it.
The solution
This solution provides a way to pass custom response factory to the middleware, so that one needs not rely on the default tuupola/http-factory, but falls back to the default, if no custom one is provided.
Hi, Mika. I marked 2 lines for review. If you like this in general, I will finish the PR by providing a documentation entry and a test. Let me know. Thanks!
Sorry it took a while.
You already did the correct fix which is to code again ResponseInterface and not a concrete implementation. Also I think no package should force install a specific PSR-7 implementation.
That said I have had plan to do something similar. However the factory parameter be a PSR-17 factory and not a callable. If none given it should default to the autodiscovering Tuupola\Http\Factory\ResponseFactory.
Mika, I have addressed your remark, however while I am writing the documentation entry, I realized a shortcoming with the approach to provide a response factory instance - what if one does not want to create the instance of the factory every time or wants to provide it on demand? In most cases surely the factory is a trivial object with no dependencies, but what if not? One needs to create the instance every time during the initialization, even if it is not used (the check passes and the factory is not needed).
You did not like the "callable response factory" approach I implemented originally, how about supporting a "factory provider" use case?
To demostrate, compare these:
$app = new Slim\App;
// compare this
$app->add(new Tuupola\Middleware\JwtAuthentication([
"secret" => "supersecretkeyyoushouldnotcommittogithub",
"responseFactory" => new MyResponseFactory,
]));
// to the following
$app->add(new Tuupola\Middleware\JwtAuthentication([
"secret" => "supersecretkeyyoushouldnotcommittogithub",
"responseFactory" => function(): ResponseFactoryInterface {
return new MyResponseFactory;
},
]));
Quick EDIT: I mean, both cases should be supported not just one of them.
Mika, I implemented support for the "factory provider", let me know what you think.
From my point of view, it does not make sense to crate the response factory every on every request, even if not needed.
Changed timezones, now I should be available again.
I thought about this for a bit. While technically you are technically correct I think the speed penalty is so small it does not warrant the additional complexity of using factory provider instead of factory itself.
Also if you are using a custom PSR-17 factory you are probably using the same factory with the framework itself. This means the factory probably has already been instantiated either directly or using a container.
While I don't really think using a provider brings much more complexity, I agree to go the simpler way.
If I wanted to initialize a "heavy" response factory (say OriginalFactory) in a lazy way (on demand), I could implement something like this easily:
class LazyFactory implements Psr17ResponseFactory {
private $original = null;
private $provider;
function __construct(callable $provider){
$this->provider = $provider;
}
function createRequest( ...$args ){
return $this->resolveFactory()->createRequest( ...$args );
}
function resolveFactory(): OriginalFactory {
return $this->original ?? $this->original = call_user_func($this->provider);
}
}
And use it as the PSR-17 response factory instead of the original OriginalFactory.
Mika, the test fails, because I need a response factory in order to test the new response factory, but your requirement of zendframework/zend-diactoros is 1.3, but their response factory is present since 2.0. So there is the option to install other PSR-15 implementation or to increase the requirement for the test or to rewrite the test to cope with version 1.3 as well. Let me know.
I have updated the test so that it passes with diactoros v1. Should be all good now.
Yeah, could also just remove the old Diactoros from composer.json. It is not really needed anymore.
It is not really needed anymore.
I believe it is needed for the tests. No other response factory is present (besides your meta factory).
Anyway the tests pass and tie PR should be ready. Take a look.
bump. Any news? what is missing to approving this issue.
The only way that i can see in order to workaround, is making a decorator object.
