ResourceIdentifier::fromArray returning null not being handled gracefully

[emil-nasso]

If you need to fetch a relationship from a request, you would normally do it like this:

$resource = $jsonApi->getRequest()->getResourceToOneRelationship('spaceship');

This returns a ToOneRelationShip-object, if the relationship is found, or null if it is not.

The getResourceToOneRelationship method in the Request class looks like this

public function getResourceToOneRelationship($relationship)
    {
        $data = $this->getResource();
        if (isset($data["relationships"][$relationship]["data"]) === false) {
            return null;
        }
        return new ToOneRelationship(ResourceIdentifier::fromArray($data["relationships"][$relationship]["data"]));
    }

The last line calls the fromArray method. The problem is that the fromArray method does this check:

if (isset($array["type"]) === false || isset($array["id"]) === false) {
            return null;
        }

So an invalid relationship returns null, but the ToOneRelationship object gets created anyway. I think it would be clearer if getResourceToOneRelationship returned null in this case.

To catch the case where the input is malformed, you would have to write code like this with the current code:

 $resource = $jsonApi->getRequest()->getResourceToOneRelationship('spaceship');
        if (isset($resource)) {
           $resourceIdentifier = $resource->getResourceIdentifier();
            if ( isset($resourceIdentifier) ) {
                //get the id from the resourceIdentifier
            }
       }

If you don't, you will very likely get an Fatal Error in php.

I would propose that getResourceToOneRelationship was changed to make sure that ResourceIdentifier:fromArray considers the data valid, something like this:

public function getResourceToOneRelationship($relationship)
    {
        $data = $this->getResource();

        if (isset($data["relationships"][$relationship]["data"]) === false) {
            return null;
        }

        $resourceIdentifier = ResourceIdentifier::fromArray($data["relationships"][$relationship]["data"]);

        if (isset($resourceIdentifier) === false) {
            return null;
        }

        return new ToOneRelationship($resourceIdentifier);
    }

With this in place, getResourceToOneRelationship would return null if the relationship didn't exist, or one of the fields type or id was missing.

The getResourceToManyRelationship would need to be fixed too. I'm not 100% sure on how though. Skip non valid relationships? Return null from the method if one of the identifiers are invalid? Rise some kind of exception? Exceptions could perhaps be used in the getResourceToOneRelationship case too? Some kind of malformed input exception.

I didn't consider this fully thought through, so i didn't create a PR. What do you think about all this?

[emil-nasso]

This functionality is required when you send null as the data for a relationship. In that case, you want to clear the relationship completely. I'm working on a few things related to that. I will create a PR soon.

[kocsismate]

Thank you very much for your work! I will come back from holiday on Sunday, so I can only deal with the issues early next week :)

[kocsismate]

I am closing this issue as I think I resolved it in this commit (78c316d), but feel free to reopen it and tell me if you think otherwise!