Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

add MethodNotAllowedException::getAllowedMethods() #13

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented Apr 21, 2019

this PR adds the getAllowedMethods() method to the MethodNotAllowedException exception class.

this helps developers to send the correct Allow header value.

see : https://tools.ietf.org/html/rfc7231#section-7.4.1
see : https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Allow
see : https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405

Note: this PR contains a small BC-break, in case someone is extending the MethodNotAllowedException class ( which is unlikely ).

Example :

try {
  list($handler, $data) = $router->routeRequest($request);

  $response = await $handler->handle(
    $request->withAttribute('data', $data)
  );

} catch(MethodNotAllowedException $e) {
  $response = new Response(405)
	|> $$->withAddedHeader('Allow', $e->getAllowedMethods());

} catch(NotFoundException $e) {
  $response = new Response(404);

}

await $emitter->emit($response); 
exit(0);

@azjezz
Copy link
Contributor Author

azjezz commented Apr 21, 2019

this PR also fixes the build by updating dependencies ( hhast 4.0.1 uses static function variables )

Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay - inlien comments and this also needs a rebase, but generally seems good.

src/http-exceptions/MethodNotAllowedException.php Outdated Show resolved Hide resolved
src/router/BaseRouter.php Outdated Show resolved Hide resolved
src/router/BaseRouter.php Outdated Show resolved Hide resolved
tests/RouterTest.php Outdated Show resolved Hide resolved
src/router/BaseRouter.php Outdated Show resolved Hide resolved
@azjezz azjezz requested a review from fredemmott November 19, 2019 21:49
@azjezz azjezz force-pushed the master branch 2 times, most recently from 00ee05c to 5fc0109 Compare November 19, 2019 21:56
tests/RouterTest.php Outdated Show resolved Hide resolved
@azjezz azjezz force-pushed the master branch 2 times, most recently from 790fa0b to 28b7bb3 Compare November 19, 2019 22:22
Copy link
Contributor

@jjergus jjergus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me!

Just one comment in the code + can you please address the lint warning?

src/router/BaseRouter.php Show resolved Hide resolved
@fredemmott
Copy link
Contributor

fredemmott commented Nov 22, 2019

Will merge once lint's fixed

the whole HEAD <-> GET reroute* should probably be removed in the future.

#15

@fredemmott fredemmott merged commit cb83d40 into hhvm:master Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants