Removes Zend\Expressive\Authentication\ResponsePrototypeTrait #17
Conversation
…Trait and use factoryFactories instead
|
Ping @webimpress |
src/OAuth2Adapter.php
Outdated
|
|
||
| /** | ||
| * Constructor | ||
| * | ||
| * @param ResourceServer $resourceServer | ||
| * @param ResponseInterface $responsePrototype | ||
| */ | ||
| public function __construct(ResourceServer $resourceServer, ResponseInterface $responsePrototype) | ||
| public function __construct(ResourceServer $resourceServer, callable $responseFactory) |
There was a problem hiding this comment.
Please remove the whole PHPDoc on __construct. It's not updated and we are not using them when there is no any additional information in it.
src/OAuth2Middleware.php
Outdated
| */ | ||
| protected $responsePrototype; | ||
| protected $responseFactory; | ||
|
|
||
| /** | ||
| * Constructor | ||
| * | ||
| * @param AuthorizationServer $server | ||
| * @param ResponseInterface $responsePrototype | ||
| */ |
There was a problem hiding this comment.
The same here, please remove PHPDocs
test/OAuth2AdapterFactoryTest.php
Outdated
| @@ -10,6 +10,7 @@ | |||
|
|
|||
| namespace ZendTest\Expressive\Authentication\OAuth2; | |||
|
|
|||
| use function foo\func; | |||
There was a problem hiding this comment.
Why we need it? Please remove.
test/OAuth2AdapterTest.php
Outdated
| $this->response->reveal() | ||
| function () { | ||
| return $this->response->reveal(); | ||
| } |
There was a problem hiding this comment.
I would add property in that class (and also in other tests), because we have the same usage there many times - and use then just $this->responseFactory.
- Adds property declarations, with annotations detailing types. - Use a `$responseFactory` property instead of creating a factory each time the SUT is instantiated.
test/OAuth2AdapterFactoryTest.php
Outdated
| ->willReturn(false); | ||
| ->get(ResponseInterface::class) | ||
| ->willReturn(function () { | ||
| }); |
There was a problem hiding this comment.
Also here we can use this->responseFactory
There was a problem hiding this comment.
No, we cannot, as it is not defined in this class.
In point of fact, this test likely needs to be re-written, as it's supposedly testing that the service can be created if the response service is missing. We should either test that an exception occurs if the service is missing (as get() will raise an exception), or that a TypeError occurs due to an invalid service. I'll make those changes shortly.
There was a problem hiding this comment.
No, we cannot, as it is not defined in this class.
Actually it is - line 41: https://github.com/zendframework/zend-expressive-authentication-oauth2/pull/17/files/f9ce613482a2dbf64bb841c7619b3d33f39890b4#diff-d706f562446541d43749de5bbfed80b7R41
test/OAuth2MiddlewareFactoryTest.php
Outdated
| ->willReturn(false); | ||
| ->get(ResponseInterface::class) | ||
| ->willReturn(function () { | ||
| }); |
There was a problem hiding this comment.
And here - this->responseFactory
There was a problem hiding this comment.
Ok, here is not defined :)
Ensures that the factories are testing expected behavior: - TypeError should be raised for a non-callable response factory. - TypeError should be raised for a response instance returned instead of a response factory.
Using PR: zendframework/zend-expressive#561 as a guide this PR removes the ResponsePrototypeTrait in favor of factoryFactories.
Fixes issue: #16