Copy matched parameters for HEAD requests #74
Conversation
|
|
||
| public function testInvokesHandlerWithRequestComposingRouteResultAndAttributes() | ||
| { | ||
| $result = $this->prophesize(RouteResult::class); |
There was a problem hiding this comment.
From experience it's bad to mock RouteResult. It is easy to mock it wrongly and I think it's better to instantiate it using one of fromRoute or fromRouteFailure. Then the test will be clearer and we can be sure that we have the proper instance.
There was a problem hiding this comment.
That would mean the other tests are bad as well. This is code I copied from the test before.
There was a problem hiding this comment.
Yes, not future-proof, and it should be changed imho. I did some of these changes here:
adb00ca
As this is a new test it should be fixed before merging, and in separate PR we can fix others.
Don't you think that test will be clearer if you use fromRoute or fromRouteFailure to create proper RouteResult ?
There was a problem hiding this comment.
It's a complicated test. I don't have time to update all tests. This issue already took me a some time to debug and fix. I'll see what I can do in the weekend.
There was a problem hiding this comment.
I'm not saying you have to fix all other test, just your test. All others can be fixed later on in separate PR.
|
|
||
| $parameters = ['foo' => 'bar', 'baz' => 'bat']; | ||
| $result = $this->prophesize(RouteResult::class); | ||
| $result->isFailure()->willReturn(false); |
There was a problem hiding this comment.
As commented above. Here you are mocking it because you know what are the internal calls (because you checked them). Why you are not mocking isSuccess method? I think this test is not future proof, so as I suggested above RouteResult should NOT be mocked.
|
Thanks, @xtreamwayz! |
Someone is spamming my site with HEAD requests and they are causing exceptions. I don't care much about the spamming, I'm more interested in why the request fails. It turns out for a normal GET request, the matched route parameters are copied into the request as attributes in the RouteMiddleware. The ImplicitHeadMiddleware runs after the RouteMiddleware and it tries to match the request again as a HEAD request, however it doesn't copy the matched route parameters into the request. This causes the handler to load it as a GET request, but it doesn't get the parameters and thus throws exceptions. I could check for an existing id but without a valid id, the GET route wouldn't be matched anyway and so shouldn't the HEAD request.
There are 2 solutions I can think of.
$request->getAttribute('id')can be used.Unless someone has a better idea, I'll go for solution 2.
masterbranch, and submit against that branch.CHANGELOG.mdentry for the fix.