tobyzerner / json-api-php

JSON-API (http://jsonapi.org) responses in PHP.
MIT License
436 stars 79 forks source link

Replace method_exists by is_callable to allow calling relations that … #107

Closed therealcljohn closed 7 years ago

therealcljohn commented 7 years ago

…have been defined at runtime using __call method. This is usefull in a modularized scenario where modules define new relations on core serializers.

Signed-off-by: Clemens John clemens.john@floh1111.de

franzliedke commented 7 years ago

Hmm, does this mean that this will always return true when __call is implemented? Not sure that's what's wanted here.

therealcljohn commented 7 years ago

Hi @franzliedke, thanks for your reply! Yes is_callable will always return true if __call is implemented.

I proposed this change because I have the requirement to add relations at runtime and this can only be done using __call. Unfortunately method_exist returns false on relations added using __call.

For cases where __call is not implemented (default) is_callable is considered the prefered method to check if a method is accessible because method_exist would also return true if a method is implemented but not accessible (private). So this should be ok.

In case where someone uses __call in his searializer (this is not the default case) and messes his code up there is a fallback error handling below which checks if the returned value is an instance of Relationship and not null. So this should be ok too.

Best whishes

franzliedke commented 7 years ago

How about extracting this check into a method called hasRelationship() or something similar? That can then be overwritten by subclasses that also implement __call().

therealcljohn commented 7 years ago

Good idea! Ill propose an update for this soon :)

tobyzerner commented 7 years ago

I think it might be preferable for you to just override getRelationship in a base serializer rather than expanding the API surface? This is what Flarum does to allow extensions to add custom relationships at runtime:

    public function getRelationship($model, $name)
    {
        if ($relationship = $this->getCustomRelationship($model, $name)) {
            return $relationship;
        }

        return parent::getRelationship($model, $name);
    }

Thoughts?

therealcljohn commented 7 years ago

Hi @tobscure nice that works for me too. I'm closing this, thank you!