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

Fixed issue with Throwable being passed to createApiProblemFromException #103

Merged
merged 5 commits into from
Oct 11, 2016
Merged

Fixed issue with Throwable being passed to createApiProblemFromException #103

Changes from all commits
Commits
Show all changes
5 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
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
137 changes: 71 additions & 66 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,10 +370,10 @@ public function create($data)

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

if ($this->isPreparedResponse($value)) {
Expand Down Expand Up @@ -427,10 +429,10 @@ public function delete($id)

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

$result = $result ?: new ApiProblem(422, 'Unable to delete entity.');
Expand Down Expand Up @@ -460,10 +462,10 @@ public function deleteList($data)

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

$result = $result ?: new ApiProblem(422, 'Unable to delete collection.');
Expand Down Expand Up @@ -494,10 +496,10 @@ public function get($id)

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

$entity = $entity ?: new ApiProblem(404, 'Entity not found.');
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,10 +531,10 @@ public function getList()

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

if ($this->isPreparedResponse($collection)) {
Expand Down Expand Up @@ -642,10 +644,10 @@ public function patch($id, $data)

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

if ($this->isPreparedResponse($entity)) {
Expand Down Expand Up @@ -679,10 +681,10 @@ public function update($id, $data)

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

if ($this->isPreparedResponse($entity)) {
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,10 +717,10 @@ public function patchList($data)

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

if ($this->isPreparedResponse($collection)) {
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,10 +752,10 @@ public function replaceList($data)
$collection = $this->getResource()->replaceList($data);
} catch (Exception\InvalidArgumentException $e) {
return new ApiProblem(400, $e->getMessage());
} catch (\Throwable $e) {
return $this->createApiProblemFromException($e);
} catch (\Exception $e) {
return $this->createApiProblemFromException($e);
} catch (Throwable $e) {
return $this->createApiProblem($e);
} catch (Exception $e) {
return $this->createApiProblem($e);
}

if ($this->isPreparedResponse($collection)) {
Expand Down Expand Up @@ -809,33 +811,6 @@ protected function createAllowHeaderWithAllowedMethods(array $methods)
return $allow;
}

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

/**
* Ensure we have a valid HTTP status code for an ApiProblem
*
* @param \Exception $e
* @return int
*/
protected function getHttpStatusCodeFromException(\Exception $e)
{
$code = $e->getCode();
if (!is_int($code)
|| $code < 100
|| $code >= 600
) {
return 500;
}
return $code;
}

/**
* Injects the resource with the identity composed in the event, if present
*/
Expand Down Expand Up @@ -998,4 +973,34 @@ protected function createHalEntity($entity)
$this->getRouteIdentifierName()
);
}

/**
* @param Exception|Throwable $error
* @return ApiProblem
*/
private function createApiProblem($error)
Copy link
Member

Choose a reason for hiding this comment

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

In reviewing, I realized something: the original was protected, so that it could be called by extending classes. Let's make it protected again, and reinstate the original name (createApiProblemFromException()). Update the docblock to indicate that the $error argument can be either a PHP 7 Throwable or any Exception type.

The argument can drop the typehint safely in a maintenance release, as that's a widening of types, vs. narrowing.

{
return new ApiProblem(
$this->validateHttpStatusCode($error->getCode()),
$error->getMessage()
);
}

/**
* Ensure we have a valid HTTP status code for an ApiProblem
*
* @param int|string $code
* @return int
*/
private function validateHttpStatusCode($code)
Copy link
Member

Choose a reason for hiding this comment

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

If the createApiProblemFrom*() methods are merged, technically you wouldn't need to separate this into a new method. However, I think it makes the other methods more readable, so I'm fine if this stays as submitted.

Copy link
Member

Choose a reason for hiding this comment

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

Just like the createApiProblem() notes, just re-instate the original method, but drop the typehint from the method signature and update the docblock to indicate what types are allowed; this is necessary to preserve backwards compatibility, and to allow users to re-use these methods in their extending classes.

{
if (!is_int($code)
|| $code < 100
|| $code >= 600
) {
return 500;
}

return $code;
}
}