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

Raise exception in Route constructor when no HTTP methods provided #59

Merged
merged 2 commits into from
Mar 7, 2018
Merged

Raise exception in Route constructor when no HTTP methods provided #59

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
733306d
Raise exception in Route constructor when no HTTP methods provided
weierophinney Mar 7, 2018
9c5dfc1
Use RequestMethodInterface constants
weierophinney Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,12 @@ public function getOptions() : array
*/
private function validateHttpMethods(array $methods) : array
{
if (empty($methods)) {
Copy link
Member

Choose a reason for hiding this comment

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

We can have here just ! $methods

Copy link
Member Author

Choose a reason for hiding this comment

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

Considering the argument is typed as an array, this makes it more semantically clear that we're checking to see if we have an empty array.

Copy link
Member

Choose a reason for hiding this comment

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

the result will be exactly the same and everywhere we are using just !, but fine - whatever you prefer, both works the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm aware the results will be the same. Readability and intent are important, however!

throw new Exception\InvalidArgumentException(
'HTTP methods argument was empty; must contain at least one method'
);
}

if (false === array_reduce($methods, function ($valid, $method) {
if (false === $valid) {
return false;
Expand Down
2 changes: 1 addition & 1 deletion test/Middleware/PathBasedRoutingMiddlewareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ public function testCommonHttpMethodsAreExposedAsClassMethodsAndReturnRoutes($me
public function testCreatingHttpRouteMethodWithExistingPathButDifferentMethodCreatesNewRouteInstance()
{
$this->router->addRoute(Argument::type(Route::class))->shouldBeCalledTimes(2);
$route = $this->middleware->route('/foo', $this->noopMiddleware, []);
$route = $this->middleware->route('/foo', $this->noopMiddleware, ['POST']);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use RequestMethod::METHOD_POST (for consistency)


$middleware = $this->createNoopMiddleware();
$test = $this->middleware->get('/foo', $middleware);
Expand Down
15 changes: 12 additions & 3 deletions test/RouteTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,13 @@ public function testRouteCanMatchMethod()

public function testRouteHeadMethodIsNotAllowedByDefault()
{
$route = new Route('/foo', $this->noopMiddleware, []);
$route = new Route('/foo', $this->noopMiddleware, ['GET']);
Copy link
Member

Choose a reason for hiding this comment

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

also here and in following tests

$this->assertFalse($route->allowsMethod(RequestMethod::METHOD_HEAD));
}

public function testRouteOptionsMethodIsNotAllowedByDefault()
{
$route = new Route('/foo', $this->noopMiddleware, []);
$route = new Route('/foo', $this->noopMiddleware, ['GET']);
$this->assertFalse($route->allowsMethod(RequestMethod::METHOD_OPTIONS));
}

Expand All @@ -103,7 +103,7 @@ public function testRouteNameForRouteAcceptingAnyMethodMatchesPathByDefault()

public function testRouteNameWithConstructor()
{
$route = new Route('/test', $this->noopMiddleware, [], 'test');
$route = new Route('/test', $this->noopMiddleware, ['GET'], 'test');
$this->assertSame('test', $route->getName());
}

Expand Down Expand Up @@ -230,4 +230,13 @@ public function testRouteIsMiddlewareAndProxiesToComposedMiddleware()
$route = new Route('/foo', $middleware->reveal());
$this->assertSame($response, $route->process($request, $handler));
}

public function testConstructorShouldRaiseExceptionIfMethodsArgumentIsAnEmptyArray()
{
$middleware = $this->prophesize(MiddlewareInterface::class)->reveal();

$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('empty');
new Route('/foo', $middleware, []);
}
}