Skip to content

Exception interface #278

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

Merged
merged 1 commit into from
Jul 25, 2016
Merged

Exception interface #278

merged 1 commit into from
Jul 25, 2016

Conversation

abacaphiliac
Copy link
Contributor

  • add \RuntimeException wrapper.
  • update all extensions of \RuntimeException to extend the wrapper.
  • SPL-exception wrappers now implement a common exception interface for this package. this allows consumers to handle all custom exception types thrown by catching the common interface rather than by catching each specific extension or their SPL parents.
  • add tests to validate existing exception hierarchy, in preparation for adjustment.
  • add tests for changes to exception hierarchy.
  • document exception interface in \JsonSchema\Constraints\ConstraintInterface.

as a consumer, this change will allow me to handle exceptions from this package like this:

try {
  $validator->check($value, $schema);
} catch (\JsonSchema\Exception\ExceptionInterface $exception) {
  // Handle all JsonSchema exceptions.
}

instead of this:

try {
  $validator->check($value, $schema);
} catch (\JsonSchema\Exception\InvalidArgumentException $exception) {
  // Handle all JsonSchema invalid-argument exceptions.
} catch (\JsonSchema\Exception\JsonDecodingException $exception) {
  // Handle JsonSchema decoding exception.
}
// Additional catch-blocks for all \RuntimeException children.

or worse:

try {
  $validator->check($value, $schema);
} catch (\InvalidArgumentException $exception) {
  // Handle all invalid-argument exceptions, not just JsonSchema exceptions.
} catch (\RuntimeException $exception) {
    // Handle all runtime-argument exceptions, not just JsonSchema exceptions.
} catch (\Exception $exception) {
    // Handle all exceptions, not just JsonSchema exceptions.
}

@bighappyface
Copy link
Collaborator

@abacaphiliac please rebase on master and push the changes to trigger the TravisCI build

@abacaphiliac
Copy link
Contributor Author

@bighappyface thanks for having me do that. a couple of my tests did not pass in php53 and php54 because they use JSON constants that aren't available until php55. those 3 tests are now skipped in the php53 and php54 builds.

@bighappyface
Copy link
Collaborator

@abacaphiliac thanks for the rebase. Would you please now squash these three commits down to one?

…r adjustment. add \RuntimeException wrapper. update all extensions of \RuntimeException to extend the wrapper. spl-exception wrappers now implement a common exception interface for this package. this allows consumers to handle all custom exception types thrown by catching the common interface rather than by catching each specific extension or their spl parents.
@abacaphiliac
Copy link
Contributor Author

@bighappyface done : )

i haven't tried this myself, but i've read that github now allows the project maintainer to configure squash requirements on merge requests, for a clean history. when enabled, it should give you a dropdown select on confirm merge to confirm squash and merge. have you tried it? are you able adjust settings in this project?

for reference:

@bighappyface
Copy link
Collaborator

@abacaphiliac thanks for squashing down your commits.

Unfortunately I can not alter the settings of this repo to enable the squash option; however, while that github feature is nice, I believe a contributor is responsible for the condition of their own branch and PR.

I have time set aside next week to add a contributors file to set the appropriate expectations.

@bighappyface
Copy link
Collaborator

+1

@mirfilip @jojo198 @Maks3w what are your thoughts on this?

It looks like @abacaphiliac took the time to setup tests to verify the inclusion of the exception interface and wrapper do not introduce BC-breaking changes and I agree with the common interface approach.

Does this look like 2.1.0 material to you?

public function testDefaultMessage()
{
$exception = new JsonDecodingException();
self::assertNotEmpty($exception->getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

These series of test are best done as a dataProvider

@Maks3w
Copy link
Contributor

Maks3w commented Jun 29, 2016

This can be ok. Anyway:

  1. I prefer don't declare LogicExceptions (like InvalidAr....) as I think they shouldn't be catched. As per PHP documentation "LogicExceptions should end with a change in your code".
  2. Runtime exceptions usally are catched so its preferible document them in the code with @throws tags.
  3. About to have a common interface I think is not very different from directly catch Exception or RuntimeException when you just want to catch any kind of unexpected situation

@bighappyface
Copy link
Collaborator

+1

@bighappyface bighappyface merged commit c269bda into jsonrainbow:master Jul 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants