Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Conversation

wesperinteractive
Copy link
Contributor

@wesperinteractive wesperinteractive commented Mar 19, 2018

Re-use the handler() method instead of directly instantiating a RequestHandlerMiddleware instance within the prepare() method.

@Ocramius
Copy link
Member

Requires test additions and better PR/Commit description

@geerteltink
Copy link
Member

Not sure if this needs additional tests. All it does is using the internal method to return a new instance of RequestHandlerMiddleware instead of returning RequestHandlerMiddleware directly.

@geerteltink
Copy link
Member

Also the tests are already in place:

public function testPrepareDecoratesRequestHandlersAsMiddleware()
{
$handler = $this->prophesize(RequestHandlerInterface::class)->reveal();
$middleware = $this->factory->prepare($handler);
$this->assertInstanceOf(RequestHandlerMiddleware::class, $middleware);
$this->assertAttributeSame($handler, 'handler', $middleware);
}
public function testHandlerDecoratesRequestHandlersAsMiddleware()
{
$handler = $this->prophesize(RequestHandlerInterface::class)->reveal();
$middleware = $this->factory->handler($handler);
$this->assertInstanceOf(RequestHandlerMiddleware::class, $middleware);
$this->assertAttributeSame($handler, 'handler', $middleware);
}

@Ocramius
Copy link
Member

Gotcha: not having any context on the nature of the patch besides the diff, I asked for a test, assuming a functional change.

@weierophinney weierophinney merged commit c3d1d44 into zendframework:master Mar 19, 2018
weierophinney added a commit that referenced this pull request Mar 19, 2018
weierophinney added a commit that referenced this pull request Mar 19, 2018
weierophinney added a commit that referenced this pull request Mar 19, 2018
@weierophinney
Copy link
Member

Thanks, @wesperinteractive

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants