Implementation of configurable serializers. #45

jasonlewis · 2014-05-03T09:34:39Z

This is a PR in relation to #24 and #18.

Seeing as @joshuajabbour doesn't have the time at the moment to continue working on the PR I've had a crack at an implementation and would love some feedback.

The Implementation

At its core is the SerializerInterface which has 5 method signatures declared. Each serializer implements these methods to return the data in a format of their desire. Included is a basic ArraySerializer, which returns the data in what I consider an untouched form. Resulting JSON from the ArraySerializer would be:

{
    "id": 1,
    "name": "Bob",
    "age": 32,
    "embeds": [
        "posts"
    ]
}

Benefit here is for systems returning pre-keyed data (above would have the user data keyed to user, for example).

The default used serializer would be the (current) DataArraySerializer which keys the top level data to data.

{
    "data": {
        "id": 1,
        "name": "Bob",
        "age": 32
    },
    "embeds": [
        "posts"
    ]
}

The serializer is set on the Manager via the setSerializer method.

$manager = new League\Fractal\Manager;

$manager->setSerializer(new League\Fractal\Serializer\ArraySerializer);

If no serializer is set the default DataArraySerializer is used.

Meta Data

This PR also includes meta data support for resources. Instead of defining a method on transformers I've opted for a (hopefully) better approach by setting the meta data on the resource.

$user = new User('Bob', '23');

$resource = new League\Fractal\Resource\Item($user, new UserTransformer);

$resource->setMeta(array('token' => 'kan1$isdd098hjasd89hsa#$9q'));

The ArraySerializer simply merges with the top level much like it does with the standard data and the DataArraySerializer would key it to meta and merge to the top level.

Tests

I've not done any tests yet merely patched the existing tests that failed due to the new meta data key. Will do tests once some more feedback is received on the approach and implementation.

Cheers.

RSully · 2014-05-03T17:46:35Z

What is the reasoning behind a static setSerializer? Generally I err away from that type of design.

Art4 · 2014-05-03T22:02:02Z

src/Resource/Item.php

+     */
+    public function setMeta(array $meta)
+    {
+        $this->meta = $meta;


Why not use array_merge_recursive() here?

Well it's a setter, an overwrite makes sense to me.

Only reason I didn't do any merging is because you'd most likely be only setting the meta data once since you need the resource instance to do so.

jasonlewis · 2014-05-03T22:52:20Z

Hm, can't remember why I had it as static. There's no reason for it to be static. Will change after breakfast.

philsturgeon · 2014-05-04T15:04:36Z

src/Scope.php

            }
        }

-        return $output;
+        $data = array_merge($data, $serializer->serializeEmbeds($this->availableEmbeds));


Would it make sense to try and build up various aspects of the data variable then just merge once at the end?

I could probably do that myself at the end, but... :)

You mean have like a $meta, $embeds, $pagination, and $cursors variables then do a the merge?

Yeah I guess. Big arrays being merged multiple times could get a little slow.

Absolutely. I'm about to crash for the night so will get on it in the morning. 😄

philsturgeon · 2014-05-04T15:13:50Z

In your example:

$user = new User('Bob', '23');

$resource = new League\Fractal\Resource\Item($user, new UserTransformer);

$user->setMeta(array('token' => 'kan1$isdd098hjasd89hsa#$9q'));

Is the setMeta method really on $user or should it be $resource?

philsturgeon · 2014-05-04T15:14:34Z

And how well does this work with embedded data? Can meta be added to a nested collection or items in a nested collection?

jasonlewis · 2014-05-04T15:14:59Z

My bad, that should be on $resource.

philsturgeon · 2014-05-04T15:15:33Z

Cool. This all looks great.

jasonlewis · 2014-05-04T15:16:07Z

No, that's the only thing that I wasn't sure on. If that were the case you would probably have to do it a bit differently I think. Especially if you wanted all meta data to be merged at the top level.

philsturgeon · 2014-05-04T15:19:44Z

Meta data definitely should be merged at the top level, but stick to the respective level.

{
    "data" : [
        "foo" : "bar",
        "comments" : [
            "data" : [
                "message" : "durp"
            ]
        ]
    ]
}

That is a fairly normal looking output for the default serializer. With meta it would look like this:

{
    "meta" : [
        ...
    ],
    "data" : [
        "foo" : "bar",
        "comments" : [
            "meta" : [
                ...
            ],
            "data" : [
                "message" : "durp"
            ]
        ]
    ]
}

Does that make sense?

Stuff like paginates need to be able to go into embedded resources too eventually, as by default you will only receive a sub-set of data and want links to fetch more.

jasonlewis · 2014-05-04T15:21:53Z

Yeah that makes sense and is a whole lot easier then what I was thinking. So would you prefer the current method of setting the meta data on the resource or defining it in the transformer with a meta method (or something similar)?

rolfnl · 2014-05-04T19:43:40Z

Nice work. I wonder if the use of array_filter in the ArraySerializer is correct. I think by default Fractal (or the basic serializer for that matter) should not change the output data even when values are null, false or empty. No?

philsturgeon · 2014-05-04T19:44:48Z

The default serializer does not put null embeds in. Others can, but that is how it works now. :)

--
Phil Sturgeon

On May 4, 2014 at 8:43:43 PM, Rolf (notifications@github.com) wrote:

Nice work. I wonder if the use of array_filter in the ArraySerializer is correct. I think by default Fractal (or the basic serializer for that matter) should not change the output data even when values are null, false or empty. No?

—
Reply to this email directly or view it on GitHub.

jasonlewis · 2014-05-05T02:08:30Z

Okay so it's no longer using array_filter. I've also changed the setMeta method on the resource instances so that it now return the resource. Allows easier setting of meta data on embeds.

public function embedPosts(User $user)
{
    $posts = $user->posts;

    return $this->collection($posts, new PostTransformer)->setMeta(array('foo' => 'bar'));
}

The meta data would be nested alongside the embedded data. So:

{
    "data": {
        "foo": "bar",
        "posts": {
            "meta": {
                "foo": "bar"
            },
            "data": [
                {
                    "message": "durp"
                }
            ]
        }
    }
}

philsturgeon · 2014-05-06T11:43:14Z

src/Serializer/DataArraySerializer.php

+     */
+    public function serializeData(array $data)
+    {
+        return array('data' => $data);


This all looks great, but I am not sure it will cover the use-case of some users. It definitely will let me have my data arrays nested, but other users with other serializers will be more tricky.

There is no differenciation between serializing a singular item or multiple, meaning JSON-API is not gonna work.

There is no context of what the item is, so instead of data having posts for JSON-API will not work either.

JSON-API wants an array of items, even when only one item is returned. If there was a way to handle item and collection resources differently, you could simply wrap the item in an extra array.

I know that might sound like a bit of an arse, but think, if you can support HAL and JSON-RPC out of the box with this package then your API package users will flip out. :)

I'll have to look into HAL.

As for the context I suppose each of the serializers methods could accept the resource itself or the serializer can implement a setResource method so it can internally inspect the resource. Would that be the best way of going about that do you think?

Unless I sit down and fight with the code I am not sure off the top of my head. I think the original plan was to implement JSON-API and HAL in the core, and have them error if certain properties are not available.

Whatever happens it cannot be directly tied to the transformer. A transformer is just the data structure, which could be implemented for multiple different types of "thing". A user transformer could be "friends" and "owners" for example.

It sounds like we're on the same page though. The resource can handle it once created, with setResourceKey or something. By default it can be "data", or it could be null and the JSON-API serializer can shout at you if its not set.

I think I prefer the shouting approach, as its less silent hidden magic.

What do you think?

I meant to paste an example:

public function embedImages(Moment $moment) { return $this ->collection($moment->getAllImages(), new ImageTransformer) ->setResourceKey('avatars'); }

Yeah that makes sense. So once you have a resource of posts, for example, you'd do something like this.

$resource = new League\Fractal\Resource\Collection($posts, new PostTransformer); $resource->setResourceKey('posts');

That's what you're getting at, right? And then the JSON API serializer will simply throw a tantrum if there's no key. Because it shouldn't try and guess at the key.

Yeah I'm with you now. Will push some changes shortly. 😄

Yeah thats what I’m getting at. :)

Then you can do the same in your controller, and everything should be happy and flexible. We document that they are optional, and its a good idea to set them if you plan on changing formats, but also mention WHY THE HELL WOULD YOU CHANGE FORMATS.

Like those Laravel folks who pretend people are swapping from MySQL to Mongo and back again every other Tuesday. :D

--
Phil Sturgeon

On May 6, 2014 at 1:35:47 PM, Jason Lewis (notifications@github.com) wrote:

In src/Serializer/DataArraySerializer.php:

@@ -0,0 +1,29 @@
+<?php namespace League\Fractal\Serializer;
+
+class DataArraySerializer extends ArraySerializer
+{
+

/**

\* Serialize the top level data.

\*

\* @param array $data

\*

\* @return array

*/

public function serializeData(array $data)

{

return array('data' => $data);
Yeah that makes sense. So once you have a resource of posts, for example, you'd do something like this.

$resource = new League\Fractal\Resource\Collection($posts, new PostTransformer);
$resource->setResourceKey('posts');
That's what you're getting at, right? And then the JSON API serializer will simply throw a tantrum if there's no key. Because it shouldn't try and guess at the key.

—
Reply to this email directly or view it on GitHub.

Repository ALL the things!

jasonlewis · 2014-05-06T12:44:44Z

@philsturgeon Thoughts on changing ResourceInterface to ResourceAbstract so that most of these recurring methods don't have to be implemented twice?

frankdejonge · 2014-05-06T12:47:40Z

@jasonlewis keep in mind that that would require a new major version to be released. Another option would be to have a ResourceAbstract which implements the ResourceInterface so you have a non-BC-breaking change.

jasonlewis · 2014-05-06T12:48:23Z

@FrenkyNet Good point, will do that for now. 👍

philsturgeon · 2014-05-06T12:54:46Z

We’d only need a minor as this is still 0.x and we can break BC as much as we like. Obviously I’d like to minimize it, but not at the sake of complicating the design process.

rolfnl · 2014-05-06T14:13:21Z

Would you like to support HAL with another package, or coded from scratch? I've had good experience with Nocarrier\Hal (https://github.com/blongden/hal#nocarrierhal), compared to others listed at https://github.com/mikekelly/hal_specification/wiki/Libraries#php anyway.

philsturgeon · 2014-05-06T14:16:02Z

I’ve come across this package in my adventures, but I think trying to smash the two together could end up being rather difficult, or maybe a little pointless.

We just need to make a seralizer that knows the rules of the spec, which is not hard to implement given how fractal is (or should be). :)

I was using this for me, but its silly to give it to everyone else.

Allows for generic "modifiers" and "params" to be passed around to nested relationships. The values will be available in the transformers for folks to play with when fetching extra data.

Anahkiasen · 2014-05-09T15:23:20Z

👍 For this

jasonlewis · 2014-05-09T16:12:04Z

Okay so I've worked on this a bit more and have made a few changes to the SerializerInterface, among other things. I've had to tweak the Scope class a bit so that embedded resources are transformed separately. This is so other serializers (like JSON API) can serialize that data by itself.

Anyway. The JsonApiSerializer currently produces a JSON response like this:

{
  "users": [
    {
      "id": 1,
      "name": "Jason"
    }
  ],
  "linked": {
    "posts": [
      {
        "id": 1,
        "user_id": 1,
        "title": "Foo"
      },
      {
        "id": 2,
        "user_id": 1,
        "title": "Bar"
      }
    ]
  }
}

Note that the resource key (in this case, users) has to be set either as the third parameter to a resource constructor or by using the setResourceKey method.

$resource = new League\Fractal\Resource\Item($user, new UserTransformer, 'users');

// Or...
$resource->setResourceKey('users');

JSON API is a real bitch though, especially when Fractal doesn't really deal with much more then just transformed arrays so it's hard to really manipulate the data too much more.

So before I go any further and look at HAL I'd like to have some feedback on it in its current state.

rolfnl · 2014-05-09T17:20:50Z

I hope to check in the coming days. I was away the last couple of days.. by a quick scan I wondered what happens if there's no resourceKey set, I didn't see a check or fallback if the value is null.
Apart from the links, HAL is probably easier to implement. Anyway.. that's for later.

jasonlewis · 2014-05-10T02:37:14Z

Ah, yes, if no resource key is set it's meant to throw a hissy fit. I was talking with Phil about it and basically it would be too much guessing to try and have a fallback when no resource key is set. So, really, if someone (for whatever reason) plans on switching out their serializer they'll need to either have set the resource key in advance or have to go through and set it.

It should throw an exception though if you try and use the JsonApiSerializer without a resource key. Will update that later.

Anahkiasen · 2014-05-10T09:07:46Z

Why not just fallback to data like, well, now ?

jasonlewis · 2014-05-10T09:51:02Z

Hm, no, I still think it's best of it throws an exception. Falling back to data kind of defeats the purpose of the JSON API serializer. If someone switches from the default data serializer they'll need to set the resource key. This, of course, should be mentioned in the docs.

philsturgeon · 2014-05-10T18:33:29Z

Looks good to me. I'll pull this into a local branch and have a play with it.

…develop Conflicts: src/Scope.php src/TransformerAbstract.php tests/ScopeTest.php tests/TransformerAbstractTest.php

philsturgeon · 2014-05-10T21:28:36Z

@jasonlewis can you merge my changes from develop into your version? I renamed a bunch of stuff which was easy to fix, but there some other conflicts that scared me off.

philsturgeon · 2014-05-10T21:28:50Z

Also re-pr against develop if you can.

philsturgeon · 2014-05-10T21:47:37Z

Final answer: I merged most of it and put it in the feature/serializers branch, but there are two failing tests I'd like you to look at.

League\Fractal\Test\ScopeTest::testRunAppropriateTransformerWithItem
Mockery\Exception\InvalidCountException: Method processIncludedResources() from Mockery_7_League_Fractal_TransformerAbstract should be called
exactly 1 times but called 0 times.

League\Fractal\Test\ScopeTest::testRunAppropriateTransformerWithCollection
Mockery\Exception\InvalidCountException: Method processIncludedResources() from Mockery_7_League_Fractal_TransformerAbstract should be called
exactly 1 times but called 0 times.

jasonlewis · 2014-05-11T01:28:23Z

Okay so those two tests are only failing because now it checks if the transformer has any included resources before processing (https://github.com/thephpleague/fractal/pull/45/files#diff-e3a7ce8ca66c42dd21b489b30f7daecfR161)

So it's a non-issue I think. I've rebased onto the feature/serializer, so I can fix it. Would you like the PR to be resubmitted against develop now?

philsturgeon · 2014-05-11T10:23:55Z

Yes please!

Signed-off-by: Jason Lewis <jason.lewis1991@gmail.com>

GrahamCampbell · 2014-05-11T11:55:55Z

composer.json

@@ -20,7 +20,7 @@
        "php": ">=5.3"
    },
    "require-dev": {
-        "phpunit/phpunit": "~4.0",
+        "phpunit/phpunit": "~4",


What? Why have you done that? That means we can load phpunit 6.x for example. That is not desired. ~4.0 was correct.

You're like a hawk.

Yeah this went from 3.7 to 4.0 again, and when it came back it was missing a .0. With PHPUnit 5 probably another year away im not too scared about this, but I'll get it changed back. :)

philsturgeon · 2014-05-11T12:36:26Z

Being taken care of in #47.

This was referenced May 3, 2014

Pluggable serializer for data output formats #24

Closed

Adding meta data to output? #18

Closed

Art4 reviewed May 3, 2014
View reviewed changes

philsturgeon reviewed May 4, 2014
View reviewed changes

philsturgeon reviewed May 6, 2014
View reviewed changes

philsturgeon added this to the 0.8.x milestone May 6, 2014

philsturgeon mentioned this pull request May 6, 2014

Question: Smart Embedding #27

Closed

Phil Sturgeon added 3 commits May 7, 2014 02:15

Cleared out rvmrc files

a543612

I was using this for me, but its silly to give it to everyone else.

Started some theory code for thephpleague#27

547dcb9

Allows for generic "modifiers" and "params" to be passed around to nested relationships. The values will be available in the transformers for folks to play with when fetching extra data.

Make a 0.8 branch for now.

c5ca303

Phil Sturgeon added 3 commits May 7, 2014 11:32

Added license info

bdd52f4

Back to PHPUnit 4.1

7da1039

Merge branch 'develop' of github.com:thephpleague/fractal into develop

ad3cc9b

thylo mentioned this pull request May 8, 2014

Transformer not working for Collection responses dingo/api#30

Closed

This was referenced May 8, 2014

Paginator object formatting dingo/api#19

Closed

Set the data key when using Fractal dingo/api#33

Closed

Phil Sturgeon added 4 commits May 10, 2014 20:31

Renamed embeds to includes in a bunch more places

80997ee

Params will be passed to includeFoo() methods

a8863f2

Merge branch 'develop' of git://github.com/thephpleague/fractal into …

31d6717

…develop Conflicts: src/Scope.php src/TransformerAbstract.php tests/ScopeTest.php tests/TransformerAbstractTest.php

Update README.md

5e4ffb5

jasonlewis mentioned this pull request May 11, 2014

[develop] Serializers #47

Merged

Implemented configurable serializers.

c608090

Signed-off-by: Jason Lewis <jason.lewis1991@gmail.com>

GrahamCampbell reviewed May 11, 2014
View reviewed changes

philsturgeon closed this May 11, 2014

Implementation of configurable serializers. #45

Implementation of configurable serializers. #45

Conversation

jasonlewis commented May 3, 2014

The Implementation

Meta Data

Tests

RSully commented May 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonlewis commented May 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon commented May 4, 2014

philsturgeon commented May 4, 2014

jasonlewis commented May 4, 2014

philsturgeon commented May 4, 2014

jasonlewis commented May 4, 2014

philsturgeon commented May 4, 2014

jasonlewis commented May 4, 2014

rolfnl commented May 4, 2014

philsturgeon commented May 4, 2014

jasonlewis commented May 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonlewis commented May 6, 2014

frankdejonge commented May 6, 2014

jasonlewis commented May 6, 2014

philsturgeon commented May 6, 2014

rolfnl commented May 6, 2014

philsturgeon commented May 6, 2014

Anahkiasen commented May 9, 2014

jasonlewis commented May 9, 2014

rolfnl commented May 9, 2014

jasonlewis commented May 10, 2014

Anahkiasen commented May 10, 2014

jasonlewis commented May 10, 2014

philsturgeon commented May 10, 2014

philsturgeon commented May 10, 2014

philsturgeon commented May 10, 2014

philsturgeon commented May 10, 2014

jasonlewis commented May 11, 2014

philsturgeon commented May 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon commented May 11, 2014