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

Catch Exceptions From Inputs #33

Closed
wants to merge 2 commits into from
Closed

Catch Exceptions From Inputs #33

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
70b5cc6
Catch Exceptions From Inputs
Jan 14, 2015
f3afcca
Adding i18n dependency in require-dev for test to show when filters
Jan 15, 2015
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
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@
"require-dev": {
"phpunit/phpunit": ">=3.7.0",
"squizlabs/php_codesniffer": "1.5.*",
"zendframework/zend-loader": "~2.3"
"zendframework/zend-loader": "~2.3",
"zendframework/zend-i18n": "~2.3"
},
"autoload": {
"psr-4": {
Expand Down
16 changes: 14 additions & 2 deletions src/ContentValidationListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,20 @@ public function onRoute(MvcEvent $e)
}

$inputFilter->setData($data);
if ($inputFilter->isValid()) {
return;
/**
* A specific Filter may throw an Exception if an invalid value is given.
* Convert any Exception to return a failed validation response
*/
try {
if ($inputFilter->isValid()) {
return;
}
} catch (FilterInvalidArgumentException $ex) {
Copy link
Member

Choose a reason for hiding this comment

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

As noted by another reviewer, this is not a valid exception type.

In fact, looking through the various exceptions thrown by Zend\Filter, the only time I see InvalidArgumentException thrown is typically for setters; DateTimeFormatter and the various File filters are anomalies here.

Additionally, also noted by another reviewer, validators may also throw exceptions, so type-hinting against filter exceptions only will mean those bubble outwards.

As such, this should likely be hinting against Exception, which also means that the message must change. I'll note that below.

return new ApiProblemResponse(
new ApiProblem(422, 'Invalid value supplied to filter', null, null, array(
'validation_messages' => array($ex->getMessage()),
))
);
Copy link
Member

Choose a reason for hiding this comment

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

There are a few problems with this approach. First, it may not be an invalid value at all; it may be a misconfigured filter/validator. Second, passing the exception message into validation_messages could potentially leak environment information into the response, and should only be done when in development mode. Finally, as noted, if this isn't a validation issue, but a configuration issue, 422 is an incorrect status code, and the error should not be returned in the validation_messages array at all.

}

return new ApiProblemResponse(
Expand Down
36 changes: 36 additions & 0 deletions test/ContentValidationListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,42 @@ public function testApiProblemResponseFromInvalidContentContainsValidationErrorM
$this->assertInternalType('array', $asArray['validation_messages']['bar']);
}

public function testReturnsApiProblemResponseIfAnInputFilterThrowsAnException()
{
$services = new ServiceManager();
$factory = new InputFilterFactory();
$services->setService('FooValidator', $factory->createInputFilter(array(
'foo' => array(
'name' => 'foo',
'validators' => array(
array('name' => 'DateTime'),
),
),
)));
$listener = new ContentValidationListener(array(
'Foo' => array('input_filter' => 'FooValidator'),
), $services);

$request = new HttpRequest();
$request->setMethod('POST');

$matches = new RouteMatch(array('controller' => 'Foo'));

$dataParams = new ParameterDataContainer();
$dataParams->setBodyParams(array(
'foo' => '12314',
));

$event = new MvcEvent();
$event->setRequest($request);
$event->setRouteMatch($matches);
$event->setParam('ZFContentNegotiationParameterData', $dataParams);

$response = $listener->onRoute($event);
$this->assertInstanceOf('ZF\ApiProblem\ApiProblemResponse', $response);
return $response;
}

public function testReturnsApiProblemResponseIfParametersAreMissing()
{
$services = new ServiceManager();
Expand Down