[RFC] Extending JSON API Serializer

[quetzyg]

Hi,

This PR changes the visibility of some methods of the JsonApiSerializer class, so that it's easier to extend from.

PS: Made a new PR to comply with the rules (i.e. do not PR from master branch).

[prolic]

Why do you extend? Just aggregate and implement the interface.

[quetzyg]

Hi @prolic,

Well, for starters, why implement a fullblown serializer, when what I need is already there, but just needs a minor tweak? And also, why not? And then there's the DRY thing, too.

Basically, I need to inject meta data into some of the items in relationships. That data comes from a pivot table that manages many-to-many relations.

Check here to see what I mean.

So, the current issue here is that besides having to make the change I want to the parseRelationships() method, I also have to implement exact copies of injectData(), isCollection(), isNull(), isEmpty(), fillRelationships() and shouldIncludeLinks() because their visibility on the parent is private.

And while injectData() is a public method, if you don't have your own implementation, the private fillRelationships() and parseRelationships() from the parent will be called instead.

I reckon the pull request should make more sense now.

[prolic]

You can implement the interface and proxy all methods to the aggregated original serializer. This is done in seconds. Than you can implement needed changes in your new class. Extending classes is a bad thing in most situation, that's also why there are private functions.

I would rather suggest making the JsonApiSerializer class final.

Further reading: http://ocramius.github.io/blog/when-to-declare-classes-final/

[quetzyg]

There are some valid points in that article you mentioned, and I do get that all the methods I had to reimplement are specific to the JsonApiSerializer class (hence being private). But do note that there's no actual Serializer contract/interface being implemented.

You could say that extending the SerializerAbstract class works as a contract, since it forces you to implement all the abstract methods, but then, so does an abstract method in a trait. Like I said, it works, but doesn't feel right.

Also, if we want to prevent the inheritance chain of doom, wouldn't the JsonApiSerializer and DataArraySerializer have to be extending the SerializerAbstract class or implementing a SerializerInterface instead of extending the ArraySerializer?

Anyway, I'll try your suggestion with proxying the methods and see how it goes.

[quetzyg]

Can you elaborate more on your suggestion, @prolic ? I've had a look at proxy design patterns, I also found this, and this seems to overcomplicate things when what I want is a simple change in the logic of a method.

To put it simply, how would you implement a new serializer the way you suggest?

[prolic]

The proxy manager is for other use cases.

Take a look at this gist. You can use it and change the methods you need.

Best

[quetzyg]

Thanks for your reply @prolic, although from what I can see, that's just a wrapper around the JsonApiSerializer and it's public methods.

The issue I'm facing is with the logic in the parseRelationships() method, which is private.
Currently the objects being created in the resulting relationships array only have a type and id property, but I also need to have a meta property with arbitrary data.

And I don't see other way of doing it without extending the class.

[prolic]

But the private method must be called from a public one. Otherwise it would never be called.

[prolic]

Just rewrite the public method, private methods are only an implementation detail.

[quetzyg]

But the private method must be called from a public one. Otherwise it would never be called.

Yes, I understand that, but there's no way to change the current behaviour of the private method from the outside, is there? Look at the code and tell me how do I add another property to go along with type and id, via a public method call.

Just rewrite the public method, private methods are only an implementation detail.

As it stands, this implementation detail is limiting us to a subset of the JSON API spec. Mind you, I'm well aware that the complete spec isn't yet implemented in the JsonApiSerializer, but what you suggest doesn't make sense (to me, anyway).

Do make an effort to understand that not all design patterns and ideologies apply to everything.
And if they do in this particular case, please explain why with concrete examples and fair reasoning, not links to blog posts and saying just because.

Can anyone else comment on this? @philsturgeon, @jasonlewis ?

[prolic]

1. I am not the component maintainer, don't care what I say.
2. Extend classes as you wish. It's bad practice, you can either learn to make better decision or keep bad design, extending everything you find. There is a reason, why lot's of classes are declared final and methods are private, properties are private, and so on.
3. Not all design pattern apply always, but this time it clearly does. The component maintainer can decide that they want to accept your PR, but I would rather make all classes final so extending is forbidden.
4. I'm out.

[sstok]

Hmm, the only thing @quetzyg want is to inject "meta" in the relationship part.
So adding an extension point there (public overwritable method)/setter callback that allows to filter the relationship part should solve this use-case, no?

I agree that just making all the private methods protected is overkill and makes it harder to change the class in the future (as now you have an BC promise for this class that can't be broken without a major version bump).

Using a proxy or decorator (to be exact) is also an option but will require much more work, and will require a deep knowledge about the returned structure.

[quetzyg]

Appreciated @willishq!