New resource-based API

[tobyzerner]

Simpler, 2-3x faster, and doesn't violate the LSP like the old serializer-based API (#115).

Still to come:

• Rewrite tests
• Redo error handling
• Finalize meta/links handling methods
• Clean up code/tests
• Finalize README
• Update CHANGELOG

[tobyzerner]

That's what I like to see 😍

[f3ath]

Yay! This was going to be the next target of my hate. So glad it's dead ;)

[tobyzerner]

Example usage:

class PostResource extends AbstractResource
{
    protected $type = 'posts';

    protected $post;

    public function __construct($post)
    {
        $this->post = $post;
    }

    public function getId()
    {
        return $this->post->id;
    }

    public function getAttributes(array $fields = null)
    {
        return ['body' => $this->post->body];
    }

    public function author()
    {
        return new Relationship(new UserResource($this->post->author));
    }
}

class UserResource extends AbstractResource
{
    protected $type = 'users';

    protected $user;

    public function __construct($user)
    {
        $this->user = $user;
    }

    public function getId()
    {
        return $this->user->id;
    }

    public function getAttributes(array $fields = null)
    {
        return ['name' => $this->user->name];
    }
}

for ($i = 1; $i <= 100; $i++) {
    $users[] = (object) ['id' => $i, 'name' => 'Toby'];
}

for ($i = 1; $i <= 50; $i++) {
    $posts[] = (object) ['id' => $i, 'body' => 'hello', 'author' => $users[array_rand($users)]];
}

$resources = array_map(function ($post) {
    return new PostResource($post);
}, $posts);

$document = new Document($resources);
$document->setInclude(['author']);

echo $document;

[f3ath]

how about this

public function getSparseAttributes(array $fields); // and this guy can be implemented in the abstract class
public function getAttributes();

[tobyzerner]

Hmm I think that probably creates more pain than it saves when you do want to implement it, because you then have two functions that are calculating attributes:

public function getAttributes() {
    return [
        'foo' => $this->calculateExpensiveAttribute()
    ];
}
public function getSparseAttributes(array $fields) {
    $attributes = [];

    if (in_array('foo', $fields)) {
        $attributes['foo'] = $this->calculateExpensiveAttribute();
    }

    return $attributes;
}

vs. what we have now:

public function getAttributes(array $fields = null) {
    $attributes = [];

    if ($fields === null || in_array('foo', $fields)) {
        $attributes['foo'] = $this->calculateExpensiveAttribute();
    }

    return $attributes;
}

For implementations that don't care about sparse fieldsets, I'm pretty sure you can leave out the $fields argument completely (as in the getAttributes() method of my first example) since it has a default of null?

[f3ath]

Yep, I see your point. Agreed.

[franzliedke]

Good stuff. :)

[crhayes]

I just started using this library and wanted to drop an opinion here 😄

I like this change, although I like the name Serializer more than Resource. I think it better describes what the class is doing (i.e. serializing the resource into another format). The data object that is being serialized is the resource.

For instance, api/users/me queries for a User resource, and then serializes it using the UserSerializer.

I'm guessing maybe this change is to be more in line with the wording in the JSON API spec?

[franzliedke]

What about Representation? That would fit the REST concept very well.

[crhayes]

@franzliedke I like Resource better than Representation. TBH I've come around to Resource. I also think this bit of code makes a lot of sense:

return new Relationship(new UserResource($this->post->author));

Since you have a relationship with another resource. It reads well!

@tobscure Anything I can do to help move this along? I'm excited to start using this when it's ready :)

[tobyzerner]

Yeah I'm quite happy with Resource, it makes semantic sense as @crhayes pointed out. The class isn't just serialising or representing a JSON-API resource; it is a JSON-API resource.

@crhayes Feel free to start using it now, and let me know if you uncover any issues. I don't expect any further API changes. I just want to rewrite the test suite from scratch before releasing.

[f3ath]

Abstract class forces the object to belong to a certain hierarchy. If your domain object is already a part of another hierarchy (e.g. class DiscountShoppingCart extends ShoppingCart) you won't be able to extend from the abstract class in the same time. But it is possible for a ShoppingCart to implement ResourceInterface.

[f3ath]

It also forces your clients to prefer inheritance over composition, you don't want to do that. https://www.thoughtworks.com/insights/blog/composition-vs-inheritance-how-choose

[tobyzerner]

Right, understood. But then that leads me back to the method naming... I feel like the methods as they are will be pretty likely to conflict if implemented into an existing hierarchy. Like in your shopping cart example - a shopping cart could easily have "attributes" and "meta" which are not the same as JSON-API attributes/meta.

I agree we shouldn't rename them to avoid conflict with any specific library, but what about to avoid conflict in general?

[f3ath]

You can't solve every potential corner case. I assume the best strategy is to stay focused on ResourceInterface as a part of JSON API domain.

[tobyzerner]

By the logic you've argued, should we also be using a RelationshipInterface?

[f3ath]

The original problem was LSP and it seems getting solved as far as I can tell. How to implement the architecture properly in general - I don't know, that's what I'm trying to answer by building my library. It's tough ;)

[tobyzerner]

I think technically, yes, it should be an interface. Because there are two separate concerns:

1. Representing the relationship information in the context of a tree of resources (this is the job of an interface - we just need to be able to getData, getMeta, and getLinks, we don't care how it's constructed)

2. Serializing into a JSON-API relationship object (we just need to be able to setData, setMeta, and setLinks, and then run jsonSerialize) - these are the kinds of objects in your library

You could also argue the same for Error and Link objects.

But pragmatically, it's much easier to just combine these responsibilities into the same class, so we can skip the step of mapping a (1) object to a (2) object during the serialization process. Even though it's technically not quite correct, I think I'm happy to leave it as is - otherwise I will end up rewriting your library in PHP 5, which I'm too lazy to do right now :P

[kamilkurczak]

how we support if relationship is empty? now we return {data: id: {null, type: type}} but it should be {data: null}

[tobyzerner]

What happens if you return Relationship::fromData(null)?

[shadowhand]

I've reviewed these changes a couple of times and am in favor of this change. I have some reservations about the Error class. Like AbstractResource it may make more sense to have an AbstractError base class that defines required methods. This was a pain point for us with the current version of the package, so we created our own AbstractError class.

[cjunge-work]

It also forces your clients to prefer inheritance over composition, you don't want to do that.

I think this is the key point regarding name conflicts, as with composition the implementing class will be "composing" the target class.
By that I mean, if there are name conflicts, then the implementing class knows what they are, and knows what to do, so the implementing class with do the transformation down to the composed class.

[shadowhand]

@cjunge-work I see your point. I guess that makes sense, though having an interface might be beneficial here. At least people would have the option to implement their own specific error classes instead of using the builder.

[antonkomarev]

@tobscure any news on it?

[antonkomarev]

I'm proposing to name method more verbose (because we will pass Resource there):

$document = Document::fromResource($resource);

[tobyzerner]

You can also pass in an array of resources. fromData matches with the top-level data key in the resulting document.

[burzum]

@tobyzerner @antonkomarev is this project any longer maintained? :( I've updated the master branch with the polymorphic PR of Anton, updated phpunit to 8.5, set min php to 7.2 and applied the PSR12 coding standard. https://github.com/burzum/json-api-php/tree/develop

[antonkomarev]

I'm using my own local implementation based on this one. It's enough for now. But surely it will be good to see this lib to move forward.

[burzum]

@antonkomarev I'm trying to add typed return types and run into one issue, seems like a bug to me because according to the doc block it should only return array in Resource.php.

    public function toIdentifier(): array
    {
        if (! $this->data) {
            return; // Changing this to [] breaks the test.
        }

        $array = [
            'type' => $this->getType(),
            'id' => $this->getId()
        ];

        if (! empty($this->meta)) {
            $array['meta'] = $this->meta;
        }

        return $array;
    }

I think it should then return an empty array or throw an exception?

Is there a better place than this PR to discuss these issues? I would like to push the lib a little forward to up to date php.

Also the doc block here said string but actually should be int and return 0 instead of empty string I think. Document.php:

    protected function getPage($key): int
    {
        $page = $this->getInput('page');

        return isset($page[$key]) ? $page[$key] : 0;
    }

By the way my changes are based on the master branch, not this PRs branch. Is this PR already OK to use? If yes I'll have to merge it...

[antonkomarev]

Since toIdentifier method in Collection class calling $resource->toIdentifier() method inside of array_map - this method shouldn't return null or void then.

[burzum]

@antonkomarev well, something does make it return null, this is at least what the tests reveal.

What is holding this PR here back from being merged?