Intercept exceptions from the ServerRequest factory #373
Conversation
|
Ping @michaelmoussa — thoughts? |
| * | ||
| * @var string | ||
| */ | ||
| private $serverRequestFactory = ServerRequestFactory::class; |
There was a problem hiding this comment.
@weierophinney you can mock this using Mockery without this private property + reflection if desired, by the way.
/**
* @runInSeparateProcess
* @preserveGlobalState disabled
*/
public function testSomething()
{
$serverRequestFactory = m::mock('alias:' . ServerRequestFactory::class);
$serverRequestFactory->shouldReceive('fromGlobals')
->once()
->andReturn(['foo' => 'bar']);
$this->assertSame(['foo' => 'bar'], ServerRequestFactory::fromGlobals());
}Both annotations are required for this to work, else you will hit:
Mockery\Exception\RuntimeException: Could not load mock Zend\Diactoros\ServerRequestFactory, class already exists
No real harm in leaving it how it is now, but IMO the less "this is only used for testing" stuff in real classes, the better.
|
@weierophinney PR looks good overall. Only suggestion was using Mockery alias mocks, which would allow us to remove an otherwise unnecessary private property as well as the two test asset classes. Regarding bug vs feature, this looks like more of a "doing it this way is better than we are currently doing it" than "it is currently wrong and we must fix". Probably makes more sense for a minor release than a patch release. |
I actually tried those first, but could not get them to work; I suspect the fact that the Regarding minor release vs patch, I'll rebase this against develop, and then update the PR to base it against there. Thanks for the feedback, @michaelmoussa ! |
|
Initial run worked, but then when I started removing the extraneous code, I started getting "serialization of closure is not allowed' errors from PHPUnit. Trying to debug... |
|
So, the solution was to wrap the test case in a try/catch block, which helped me discover that the response passed to the emitter was not the expected one... which brings me back to the original issue I had with |
|
@weierophinney "Serialization of Closure" in that context is usually due to what's being output by the test run in isolation. IIRC the tests in isolation communicate back to the main PHPUnit process by spitting out a serialized response which the main process reads. If you get annoyed enough, feel free to share what you've got and I'll see if anything screams at me. |
As noted in zendframework/zend-diactoros#171, the `ServerRequestFactory::fromGlobals()` method raises exceptions on malformed information, such as: - malformed file upload data - invalid HTTP protocol versions At the application level, we should catch these, and invoke the final handler with a 400 response when caught.
weierophinney
force-pushed
the
hotfix/handle-request-exceptions
branch
from
bd33eb6 to
f8cbf4f
Compare
|
@michaelmoussa Got it sorted; wrapped the internals in a try/catch, and the problem was with assertions in the closure failing. By wrapping in a try/catch, I was able to raise a failure assertion from the catch blocks. The result: I discovered I wasn't importing All is working now. I rebased against develop, and squashed my commits so that we don't have a history of those unnecessary test assets, and then re-targeted this PR against the develop branch. Ready for review again! |
|
LGTM 👍 |
|
@michaelmoussa I'll have you merge; I've got a few more proposals for 1.1.0 to sort out before we do a release, though, so don't tag. :) |
|
@weierophinney done. I used |
|
Perfect! On Sep 2, 2016 12:00 AM, "Michael Moussa" notifications@github.com wrote:
|
As noted in zendframework/zend-diactoros#171, the
ServerRequestFactory::fromGlobals()method raises exceptions on malformed information, such as:At the application level, we should catch these, and invoke the final handler with a 400 response when caught.
I've opened this against master, as I consider the current behavior a "bug"; however, in a way, it's technically a new feature. I'd appreciate comments as to the preferred release. 😄