Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom exceptions #106

Merged
merged 3 commits into from
Dec 22, 2013
Merged

Add support for custom exceptions #106

merged 3 commits into from
Dec 22, 2013

Conversation

bakura10
Copy link
Member

This PR brings support for custom exceptions, so that you can trigger custom exceptions in user land code, and map it to HTTP exception that can be interpreted by ZfrRest.

I'm wondering if we couldn't do another approach here, so that this listener is able to interpret ANY exceptions (even those not in the $exceptionMap). For those exception, it would create a generic ServerErrorException, with the message extracted from the exception. The reason is that we'd like to receive response from ZfrRest for any exception. If people forgot to add a specific exception they throw in ZfrRest, then this listener will do nothing. But I'm not sure.

ping @Ocramius

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.51%) when pulling 35642d1 on custom-exception into 175fa75 on master.

@danizord
Copy link
Contributor

I think this can be solved more easily with inheritance:

namespace Application\Exception;

use ZfrRest\Http\Exception\Client\NotFoundException;

class CustomerNotFound extends NotFoundException
{
}

or

namespace Application\Exception;

use ZfrRest\Http\Exception\HttpExceptionInterface;

class SomethingIsWrong implements HttpExceptionInterface
{
    // My custom HTTP Error, e.g http://httpstatusdogs.com/420-enhance-your-calm
}

@bakura10
Copy link
Member Author

This does not work with exceptions thrown by third-party modules or if you throw exceptions from a third-party module (for instance if you throw UnauthorizedException from ZfcRbac). Even with your own exceptions, you may not want tie all your exceptions to ZfrRest.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.51%) when pulling e52eee5 on custom-exception into 175fa75 on master.

@danizord
Copy link
Contributor

Hmm, having said that. The catch-all approach seems more reasonable to me.

@@ -61,6 +61,14 @@ public function __construct($statusCode = null, $message = '', $errors = null)
/**
* {@inheritDoc}
*/
public function setMessage($message)
{
$this->message = $message;
Copy link
Member

Choose a reason for hiding this comment

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

Cast to (string)

@bakura10
Copy link
Member Author

I'm not really satisfied with it. The problem is that I really want this listener to catch all the exception so:

  1. If exception is already a ZfrRest exception => OK.
  2. If the exception is in the exception map, then we can create a ZfrRest exception from it.
  3. Otherwise, we set a 500 error code (server error).

@Ocramius
Copy link
Member

@bakura10 the default exception handler already sets a 500 error code. I think it's simply not our job.

If we can avoid doing more work than needed, then it's ok

@bakura10
Copy link
Member Author

Ok, I've updated. Looks merge able?

@Ocramius
Copy link
Member

@bakura10 looks ok, but my feedback on the code is still missing comments/patching

@bakura10
Copy link
Member Author

Ok, merging for now then.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.51%) when pulling ab086e1 on custom-exception into 175fa75 on master.

@bakura10 bakura10 merged commit ab086e1 into master Dec 22, 2013
@bakura10 bakura10 deleted the custom-exception branch December 22, 2013 22:49
danizord pushed a commit to danizord/zfr-rest that referenced this pull request Apr 14, 2014
Add support for custom exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants