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

Error in createResourceIdInvalidException #100

Closed
Ilyes512 opened this issue Dec 2, 2020 · 3 comments
Closed

Error in createResourceIdInvalidException #100

Ilyes512 opened this issue Dec 2, 2020 · 3 comments
Labels

Comments

@Ilyes512
Copy link

Ilyes512 commented Dec 2, 2020

The DefaultExceptionFactory::createResourceIdInvalidException method contains an error.

    // WoohooLabs\Yin\JsonApi\Exception\DefaultExceptionFactory 

    /**
     * @param mixed $id
     */
    public function createResourceIdInvalidException($id): JsonApiExceptionInterface
    {
        return new ResourceIdInvalid($id);
    }

Source: https://github.com/woohoolabs/yin/blob/master/src/JsonApi/Exception/DefaultExceptionFactory.php#L163-L169

The method clearly wants to receive the $id value, but when we look at the ResourceIdInvalid-exception class you can see it wants the type of the id instead. When given a incorrect id (for example an integer id 100). It will throw an error because it expected a string.

    // WoohooLabs\Yin\JsonApi\Exception\ ResourceIdInvalid

    public function __construct(string $type)
    {
        parent::__construct("The resource ID must be a string instead of $type!", 400);
        $this->type = $type;
    }

Source: https://github.com/woohoolabs/yin/blob/master/src/JsonApi/Exception/ResourceIdInvalid.php#L17-L21

This can be solved easily in two ways:

  1. Change the initiation of the exception to new ResourceIdInvalid(gettype($id));
  2. Update all calls to createResourceIdInvalidException by adding the gettype there (Only 4 in this lib it self)

Let me know what you think.

@kocsismate
Copy link
Member

Thank you very much for the report! I'll have a look at it in the following days.

@kocsismate
Copy link
Member

I fixed the issue in master by following your first suggestion. I believe this solution has the advantages of

  • being less of a BC break
  • being the correct thing to do, and the other similar DefaultExceptionFactory methods should follow this change in the next major version

I'll release a new patch version as soon as you confirm that the commit fixed your issue.

@Ilyes512
Copy link
Author

Looks good! See #97 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants