This repository was archived by the owner on Jan 21, 2020. It is now read-only.
Simplify condition and used proper constant name #50
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In case when there is a route without any allowed method the result should contain empty array of allowed methods and `isMethodFailure` should be true.
michalbundyra
commented
Mar 7, 2018
| @@ -444,10 +435,6 @@ private function injectRoute(Route $route) : void | |||
| $methods = self::HTTP_METHODS_STANDARD; | |||
| } | |||
|
|
|||
| if (empty($methods)) { | |||
| $methods = self::HTTP_METHODS_EMPTY; | |||
There was a problem hiding this comment.
We shouldn't add default empty methods if any method is not allowed.
I think it is better to throw an exception when we try add route without any methods.
rc3 updates `Route` to raise an exception for an empty list of HTTP methods, removing the need for `testNoMethodsAllowedForRoute`, and making it possible to simply `return RouteResult::forRouteFailure($allowedMethods)` within `marshalMethodNotAllowedResult()`.
michalbundyra
commented
Mar 7, 2018
test/FastRouteRouterTest.php
Outdated
| @@ -650,4 +634,11 @@ public function testGenerateUriRaisesExceptionForMissingMandatoryParameters() | |||
|
|
|||
| $router->generateUri('foo'); | |||
| } | |||
|
|
|||
| public function method() : Generator | |||
There was a problem hiding this comment.
We don't need this method anymore, and also we can remove import Generator in top.
michalbundyra
commented
Mar 7, 2018
test/FastRouteRouterTest.php
Outdated
| use PHPUnit\Framework\TestCase; | ||
| use Prophecy\Prophecy\ProphecyInterface; | ||
| use Psr\Http\Message\ServerRequestInterface; | ||
| use Psr\Http\Message\UriInterface; | ||
| use Psr\Http\Server\MiddlewareInterface; | ||
| use Zend\Diactoros\ServerRequest; |
There was a problem hiding this comment.
This is also not used.
weierophinney
added a commit
that referenced
this pull request
Mar 7, 2018
weierophinney
added a commit
that referenced
this pull request
Mar 7, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The wrong constant name was used:
Route::METHOD_ANYinstead ofRoute::HTTP_METHOD_ANY.Also simplify condition as in both case we call
RouteResult::fromRouteFailure.