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 all commits
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
13 changes: 7 additions & 6 deletions test/Middleware/PathBasedRoutingMiddlewareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

namespace ZendTest\Expressive\Router\Middleware;

use Fig\Http\Message\RequestMethodInterface as RequestMethod;
use PHPUnit\Framework\TestCase;
use Prophecy\Argument;
use Prophecy\Prophecy\ObjectProphecy;
Expand Down Expand Up @@ -72,11 +73,11 @@ public function process(
public function commonHttpMethods()
{
return [
'GET' => ['GET'],
'POST' => ['POST'],
'PUT' => ['PUT'],
'PATCH' => ['PATCH'],
'DELETE' => ['DELETE'],
RequestMethod::METHOD_GET => [RequestMethod::METHOD_GET],
RequestMethod::METHOD_POST => [RequestMethod::METHOD_POST],
RequestMethod::METHOD_PUT => [RequestMethod::METHOD_PUT],
RequestMethod::METHOD_PATCH => [RequestMethod::METHOD_PATCH],
RequestMethod::METHOD_DELETE => [RequestMethod::METHOD_DELETE],
];
}

Expand Down Expand Up @@ -178,7 +179,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, [RequestMethod::METHOD_POST]);

$middleware = $this->createNoopMiddleware();
$test = $this->middleware->get('/foo', $middleware);
Expand Down
17 changes: 13 additions & 4 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, [RequestMethod::METHOD_GET]);
$this->assertFalse($route->allowsMethod(RequestMethod::METHOD_HEAD));
}

public function testRouteOptionsMethodIsNotAllowedByDefault()
{
$route = new Route('/foo', $this->noopMiddleware, []);
$route = new Route('/foo', $this->noopMiddleware, [RequestMethod::METHOD_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, [RequestMethod::METHOD_GET], 'test');
$this->assertSame('test', $route->getName());
}

Expand All @@ -116,7 +116,7 @@ public function testRouteNameWithGET()
public function testRouteNameWithGetAndPost()
{
$route = new Route('/test', $this->noopMiddleware, [RequestMethod::METHOD_GET, RequestMethod::METHOD_POST]);
$this->assertSame('/test^GET' . Route::HTTP_METHOD_SEPARATOR . 'POST', $route->getName());
$this->assertSame('/test^GET' . Route::HTTP_METHOD_SEPARATOR . RequestMethod::METHOD_POST, $route->getName());
}

public function testThrowsExceptionDuringConstructionIfPathIsNotString()
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, []);
}
}