cloudcreativity / laravel-json-api

JSON API (jsonapi.org) package for Laravel applications.
http://laravel-json-api.readthedocs.io/en/latest/
Apache License 2.0
780 stars 109 forks source link

Can not throw error in v2 due to type mismatch #538

Closed jelhan closed 4 years ago

jelhan commented 4 years ago

I'm trying to upgrade one of my application to v2 but struggling with throwing errors. My code is very much the same as in the documentation:

use Neomerx\JsonApi\Exceptions\JsonApiException;
use CloudCreativity\LaravelJsonApi\Document\Error\Error;

$error = Error::fromArray([
    'title' => 'Payment Required',
    'detail' => $ex->getMessage(),
    'status' => '402',
]);

throw new JsonApiException($error, 402);

But I'm facing an argument type mismatch. It's telling me that Neomerx\JsonApi\Exceptions\JsonApiException::addError() expects an instance of Neomerx\JsonApi\Contracts\Document\ErrorInterface but instance of CloudCreativity\LaravelJsonApi\Document\Error\Error given.

JsonApiException::addError() is called by JsonApiException constructor if the first argument is neither an instance of Neomerx\JsonApi\Exceptions\ErrorCollection nor an array. The first argument provided to the constructor is forwarded: https://github.com/neomerx/json-api/blob/c911b7494496e79f9de72ee40d6a2b791caaf95b/src/Exceptions/JsonApiException.php#L65-L81

I tried to find a test covering it. I found two tests, which are using JsonApiException. But none of them is passing a CloudCreativity\LaravelJsonApi\Document\Error\Error:

lindyhopchris commented 4 years ago

Hey! Thanks for reporting this and apologies - this is completely my fault!

Basically the plan was to put in a JSON API Exception to this package, rather than using the Neomerx one - with the advantage being the new JSON API exception would implement the Symfony HTTP Exception interface. The advantage to that being that it would play nicely with Laravel's exception handler, unlike the Neomerx one. (Basically my strategy going forward is to hide the Neomerx things as an internal implementation detail that devs don't need to worry about.)

What's happened is I've released v2 but forgotten to put the new exception class in. I'll look at that this week and get a new release done with the exception class added.

Sorry for the inconvenience!

jelhan commented 4 years ago

Thanks a lot for your quick feedback. I was also confused that there is still this close coupling with Neomerx's API in userland as I read your post about the goals for v2.

I'll look at that this week and get a new release done with the exception class added.

Don't feel rushed. Enough other refactoring needs to be done to finish the upgrade. :wink: Let me know if there is something I can help with.

jelhan commented 4 years ago

I'm for now using Neomerx\JsonApi\Document\Error with Neomerx\JsonApi\Exceptions\JsonApiException. This is working fine in v2 and is even covered by tests. I get that it's not the long-term way to do it but it doesn't block the upgrade and it's only used in one place of my code.

lindyhopchris commented 4 years ago

I've added the new exception class to the 2.x branch, so will be in the next release (2.1.0).