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

Follow-up to #103 #105

Closed
wants to merge 8 commits into from
Closed

Follow-up to #103 #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a95c2bd
Fixed issue with Throwable being passed to createApiProblemFromException
Aug 13, 2016
7fe0be3
Renamed validate status code method
Sep 27, 2016
0ace572
Removed IDE specific entries from .gitignore
Oct 11, 2016
5f1e3bb
Combined exception/throwable methods into one. Minor dockblock fixes.
Oct 11, 2016
d14863f
Reverted exception class change
Oct 11, 2016
1a3a2b4
Reverted methods name to avoid BC Break
michalbundyra Oct 11, 2016
89611b7
Added tests to check exception and error in methods
michalbundyra Oct 11, 2016
0091309
test with Error only runs only on PHP7+
michalbundyra Oct 11, 2016
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
54 changes: 28 additions & 26 deletions src/RestController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
namespace ZF\Rest;

use ArrayAccess;
use Exception;
use Throwable;
use Traversable;
use Zend\Http\Header\Allow;
use Zend\Http\Response;
Expand Down Expand Up @@ -368,9 +370,9 @@ public function create($data)

try {
$value = $this->getResource()->create($data);
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand Down Expand Up @@ -427,9 +429,9 @@ public function delete($id)

try {
$result = $this->getResource()->delete($id);
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand Down Expand Up @@ -460,9 +462,9 @@ public function deleteList($data)

try {
$result = $this->getResource()->deleteList($data);
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand Down Expand Up @@ -494,9 +496,9 @@ public function get($id)

try {
$entity = $this->getResource()->fetch($id);
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand All @@ -520,7 +522,7 @@ public function get($id)
/**
* Return collection of entities
*
* @return Response|HalCollection
* @return Response|HalCollection|ApiProblem
*/
public function getList()
{
Expand All @@ -529,9 +531,9 @@ public function getList()

try {
$collection = $this->getResource()->fetchAll();
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand Down Expand Up @@ -642,9 +644,9 @@ public function patch($id, $data)

try {
$entity = $this->getResource()->patch($id, $data);
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand Down Expand Up @@ -679,9 +681,9 @@ public function update($id, $data)

try {
$entity = $this->getResource()->update($id, $data);
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand All @@ -706,7 +708,7 @@ public function update($id, $data)
* a collection, i.e. create and/or update multiple entities in a collection.
*
* @param array $data
* @return array
* @return array|ApiProblem
*/
public function patchList($data)
{
Expand All @@ -715,9 +717,9 @@ public function patchList($data)

try {
$collection = $this->getResource()->patchList($data);
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand All @@ -739,7 +741,7 @@ public function patchList($data)
* Update an existing collection of entities
*
* @param array $data
* @return array
* @return array|ApiProblem
*/
public function replaceList($data)
{
Expand All @@ -750,9 +752,9 @@ public function replaceList($data)
$collection = $this->getResource()->replaceList($data);
} catch (Exception\InvalidArgumentException $e) {
return new ApiProblem(400, $e->getMessage());
} catch (\Throwable $e) {
} catch (Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
} catch (Exception $e) {
return $this->createApiProblemFromException($e);
}

Expand Down Expand Up @@ -810,24 +812,24 @@ protected function createAllowHeaderWithAllowedMethods(array $methods)
}

/**
* @param \Exception $e
* @param Exception|Throwable $e
* @return ApiProblem
*/
protected function createApiProblemFromException(\Exception $e)
protected function createApiProblemFromException($e)
{
return new ApiProblem($this->getHttpStatusCodeFromException($e), $e);
}

/**
* Ensure we have a valid HTTP status code for an ApiProblem
*
* @param \Exception $e
* @param Exception|Throwable $e
* @return int
*/
protected function getHttpStatusCodeFromException(\Exception $e)
protected function getHttpStatusCodeFromException($e)
{
$code = $e->getCode();
if (!is_int($code)
if (! is_int($code)
|| $code < 100
|| $code >= 600
) {
Expand Down
36 changes: 36 additions & 0 deletions test/RestControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1828,4 +1828,40 @@ public function testContentLocationHeaderIsGeneratedOnlyFromLinkHref($headers)
$this->assertContains('http://localhost.localdomain/resource/foo', $location);
$this->assertNotContains('true', $location);
}

/**
* @requires PHP 7.0
*
* @dataProvider methods
*
* @param string $method
* @param string $event
* @param array $argv
*/
public function testErrorInMethodReturnsApiProblem($method, $event, $argv)
Copy link
Member

@weierophinney weierophinney Oct 11, 2016

Choose a reason for hiding this comment

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

This test should be conditional on whether or not PHP 7 is being used. You can do that with an annotation: @requires PHP 7.

{
$this->resource->getEventManager()->attach($event, function ($e) {
throw new \Error('error: failed');
});

$result = call_user_func_array([$this->controller, $method], $argv);
$this->assertProblemApiResult(500, 'error: failed', $result);
}

/**
* @dataProvider methods
*
* @param string $method
* @param string $event
* @param array $argv
*/
public function testExceptionInMethodReturnsApiProblem($method, $event, $argv)
{
$this->resource->getEventManager()->attach($event, function ($e) {
throw new \Exception('exception: failed');
});

$result = call_user_func_array([$this->controller, $method], $argv);
$this->assertProblemApiResult(500, 'exception: failed', $result);
}
}