Pluggable serializer for data output formats

[jfexyz]

This is a start to implementing #15. Or at least starts to. Right now, it's just a reimplementation of the current format using an external serializer.

@philsturgeon Let me know if this is a good direction, or if you have comments, and I can implement a few other serializers...

[jfexyz]

And no, I haven't touched tests yet, or even dove too extensively into the code. This was very much a proof-of-concept while brainstorming. Just wanted to get feedback on the direction before I fleshed it all out any further...

[philsturgeon]

This is looking great! Keep on trucking.

[philsturgeon]

Could you try and get a JSON-API serializer done too, or at least one other "known" format? I figure HAL will be a bit tricky for now.

[RSully]

[In the SerializerInterface] perhaps instead of "output" we could call those methods something like "render", "serialize", or "format"? I don't want them to sound like they write anything to the output buffer.

[RSully]

I'm also a little concerned with the interface being too strict. I'm trying to think of customization options where there could be arbitrary fields.

For example, I might consider extending the DataArraySerializer and just want to add one value to the root object, without affecting how it renders to an array.

I think this is off to a great start, but I wanted to voice those concerns as you go forward :).

[philsturgeon]

serialize makes the most sense.

Adding general metadata will be taken care of when I sort out #18, so don't worry about that.

[RSully]

Certainly, but I figured now might be a good time to think about it given the changes.

[jfexyz]

As for naming, I started with ::serialize, then ::format, but finally settled on output because of the paginator and cursor methods. I can definitely revisit this.

As for more flexibility with the interface, I can see the need for that. Let me think on it. Are you talking about needing access to the whole resource item? If so, any thoughts on how to pass arbitrary extra data into it? Or just not having to redefine the whole serializer? If that, then inheritance should work.

And yes, JSON-API will be done, since that's the whole point of me doing this. :)

[jfexyz]

Okay, here's what I'm thinking, in order to tackle this and start tackling #18 at the same time. Serializer::serialize could just take a single associative array, so instead of passing it four arguments as now, you'd pass it:

SerializerInterface::serialize([
    'data' => $data,
    'embeds' => $embeds,
    'paginator' => PaginatorInterface,
    'cursor' => CursorInterface,
]);

Each serializer implementation could then handle all of the passed keys itself, doing whatever it wants. Maybe with a default of 'process{Key}' for the sake of simple inheritance.

Now, this doesn't solve the issue of passing new data to the Serializer, it just makes the method signature less rigid, so that when #18 is fleshed out more, Serializer should already support it.

Thoughts?

[RSully]

process{Key} sounds OK.

Still feels a little iffy though. Part of the serializer implementation was that anything outside of them wouldn't be responsible for knowing the strings of the keys, though I guess that limitation isn't really a requirement as much as it was a coincidence.

[jfexyz]

It's not that code outside of the serializer is required to know the keys right now, it's just that fractal currently supports (and hard-codes) four keys (well three; data is kinda fundamental). The others are metadata that fractal already compiles, and have nothing to do with serialization.

The way I see #18 possibly solved is that you could register handler(s) with fractal for compiling additional metadata. This is separate from serialization, which IMO should be solely limited to data structuring/encoding. The serializer shouldn't be required to know how to get a paginator from a collection, just how to format it. (The argument could be made that it knows how to get pagination data now, but that's all via an interface, so it's clean.)

Anyway, I'm definitely open to suggestions, so how do you see this working better?

[jfexyz]

Also, thoughts on using https://github.com/wmde/serialization? It's not a complex library, and so not all that required, but the concept of Serializer::isSerializerFor and the included exceptions might be nice to have.

And more importantly, while they don't exist now, I could see generic format serializers being written (and not tied to Fractal). This theoretical pseudo-code could then be written:

class JsonApiSerialzer implements \Serializers\Serializer
{
    // Knows how to structure data in the JSON-API format.
    public function serialize($object);
    public function isSerializerFor($object);
}

class Fractal\JsonApiSerialzer extends \Generic\JsonApiSerialzer
{
    public function serialize($object)
    {
        // Pre-process $object data into structure Generic\JsonApiSerialzer expects,
        // so Fractal doesn't need to alter it's signature to support different serializers...

        // Then have the generic serializer actually output the structure.
        return parent::serialize($object);
    }
}

Just thinking out loud here, not quite sure if/how this would work in practice...

[RSully]

I'll have to think on it. But please, don't let me hold up any progress.

[philsturgeon]

Not sure about that dependency, their docs are non-existent. 

[jfexyz]

Ok, though there isn't much to that dependency, so I'm not sure what docs are missing. Its only purpose is to provide a more loosely-coupled interface (https://github.com/wmde/Serialization/blob/master/src/Serializers/Serializer.php). And it's developed by Wikidata as the base for https://github.com/DataValues/Serialization. But, not at all crucial to moving forward.

@philsturgeon Do you have any architecture thoughts? What's in #24 (comment) is the direction I'm planning, unless/until I get more explicit use cases or potential issues.

[jfexyz]

Okay, here are some commits to bring the code up to speed. There are obviously many other ways to do this. Registering process methods for metadata instead of needing to create a new serializer might be one. Or maybe using an abstract class. But for now, this solves the most basic problem, so I'll await more feedback before going further...

[RSully]

Instead of passing by reference, what do you think about $value = $this->{$method}($value); and then getting rid of the else below (so that $output[$key] = $value; is applied)?

[jfexyz]

I didn't go that route because I wanted to not include null values in the final output by default (and I was considering serializer inheritance).

What if pagination was supplied by fractal, but your serializer wanted to drop it? When $output is a reference, you'd just have to change how processPaginator() works (say in an inherited class). But if you're returning values, the best you could do is return null, which would be included in the final output. And if wanted to drop it, you'd have to override serialize() too.

Maybe this isn't the best way, but that was the reasoning.

[philsturgeon]

I would assume that these serializers are going to be looking to return structured data like a string, JSON, array, object, or maybe a null, but they're highly unlikely to pass back boolean data, right? Just do a === false.

[jfexyz]

I dunno, the more I try to implement this in my app, there are still big problems. Like the serializer really needs to know if it's a collection or item (or other resource), since those could be output differently. And not really having access to child scopes at all might be problematic. Maybe the serializer needs to be set on the manager? Anyway, I have no more time to try this out today, so I'll come back to it when I can...

[philsturgeon]

Heads up, you'll need to pull in changes before going much further as conflicts are afoot.

[philsturgeon]

How is this one coming along @joshuajabbour? I appreciate your efforts on this, and its looking great.

I do wonder about the pass by reference stuff myself, but I think its best you try and flesh something out that solves your use case, then we can "perfect world" it later.

[jfexyz]

I've moved on to other parts of my project for now, and haven't been dealing with the API side. At some point soon I'll come back to it. This got kinda stuck however, and I needed to make progress elsewhere.

The main problem is what I described above, that the scope doesn't seem to be the correct place for the serializer. There are a lot of places that the user doesn't have access to the scope (like child or embed scopes). And even if they could get access, it would require resetting the serializer multiple times, when the most common use case is wanting to change the serializer globally. So I think the architecture needs to be rethought, and the serializer controlled by the Manager.

As for pass by reference stuff, the issues in my other comment need to be addressed somehow. I'm totally open to suggestions.

[philsturgeon]

Manager would definitely be a better place to handle the setting of the serialization yes.

I figure the API would be as simple as Manager::setSerializer(FooSerializer()) and the Scope would just ask the serializer to do its thing with the resource.

[jfexyz]

Ok, then I can def refactor with that in mind.

Any thoughts on how to solve the problem that necessitated the pass-by-reference? I'm definitely thinking there could be a whole other approach to how the SerializerInterface::serialize and the processX methods work...

[philsturgeon]

Kill this line, SensioInsight doesn't like it.

[philsturgeon]

I commented on the thread but possibly not very well.

I think with all of these processFoo methods they return data in whatever the hell form they’re up to or a null. 

You pointed out that various serializers might want to keep those null values, but I would assume that most actually don’t. We could make it an optional flag argument for the serialize() method?

-- 
Phil Sturgeon

On February 26, 2014 at 3:25:41 PM, Joshua Jabbour (notifications@github.com) wrote:

Ok, then I can def refactor with that in mind.

Any thoughts on how to solve the problem that necessitated the pass-by-reference? I'm definitely thinking there could be a whole other approach to how the SerializerInterface::serialize and the processX methods work...

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

[jfexyz]

Okay, I'll take another crack this week. Thanks!

[marlek]

Hey guys, any progress on this? :)

[jfexyz]

Nope, haven't had the time. Hope to get to it someday soon, but I'm not using fractal right now, so it hasn't been high on the list. :(

[ameech]

Hey @joshuajabbour, have you had a chance to work on this anymore?

[jfexyz]

No, and it's doubtful I'll get time soon. I'm not using fractal right now, so it's hard to find the time.

[jasonlewis]

Going to have a look at this @joshuajabbour, would like to see this implemented. I'll have a play with your PR. 😄

[jasonlewis]

So I've been playing around with this implementation. I've changed up the SerializerInterface to have 4 method signatures.

public function serializeData(array $data);

public function serializeEmbeds(array $embeds);

public function serializePaginator(PaginatorInterface $paginator);

public function serializeCursor(CursorInterface $cursor);

Each method should return an array. As an example the DataArraySerializer may return an array like this for the serializeData method.

return array('data' => array_filter($data));

Now the serialize method on Scope needs to call each of the appropriate methods and merge the result into an existing array. So if the resource was a collection and it has a paginator.

$data = array_merge($data, $serializer->serializePaginator($paginator));

This all seems to work fine. The only issue I have is with meta data and how that should be applied.

[jfexyz]

@jasonlewis Thanks for picking this up. I'm not going to be able to work on it for a while, but it's a useful change. Anyway, please note my concern above that the scope isn't the right place to manage the serializer (and phil's comment that the manager is more appropriate). Just want to make sure you're heading down that path...

As for metadata, what do you mean?

[philsturgeon]

This conversation can continue over on #45. Thanks everyone!