Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring for SOC #26

Open
ghola opened this issue Jun 10, 2016 · 3 comments
Open

Refactoring for SOC #26

ghola opened this issue Jun 10, 2016 · 3 comments

Comments

@ghola
Copy link
Contributor

ghola commented Jun 10, 2016

Normalizing/denormalizing (converting from object to/from array) and encoding/decoding (converting from array to/from string) are separate concerns. With the introduction of support for binary data the code inflated by quite a bit and has become had to manage. As such, I opted to follow the pattern used in the Symfony Serializer when I refactored the serializer to be used in my library. Here are the three main files that resulted:

  1. ObjectNormalizer deals only with transforming an object to and from an array.
  2. JsonEncoder deals with the actual json encoding/decoding, binary data encoding/decoding, and zero fraction support.
  3. JsonSerializer just uses the previous two.

In my code I stripped out the bit of functionality that deals with scalars because I'm only dealing with objects, but you get the point.

It wouldn't be a great effort to apply the same to zumba/json-serializer, but it would break BC compatibility so it would need to bump to version 3.x. You could also consider transforming this library to leverage the Symfony Serializer and just implement an encoder and a normalizer for it. This could potentially open a larger user base for it, as IMO the current normalizers used by the Symfony Serializer are useless.

What do you think?

@jrbasso
Copy link
Member

jrbasso commented Jun 11, 2016

I like the separation. When I worked on the namespace renaming (#19) was exactly to split the single class in multiple classes. Also to open for multiple normalizers (see #16 and #25).

Regarding the integration with symfony, I don't know... Seems on the 3.1 version they have a Json serializer. Since they have their own version making this library compatible will probably not add a lot of value, unless you aiming to use something that their library doesn't support. In addition, supporting Symfony's serializer interfaces could restrict some functionalities on this library. Ie, the output also needs to be compatible with Symfony. Makes sense?

@ghola
Copy link
Contributor Author

ghola commented Jun 13, 2016

Atm, the Symfony serializer package just offers a framework on which one can build it's own custom implementations. It does offer some normalizers and encoders out of the box but in my experience they offer very limited use cases. For example none of their object normalizers is fully capable of actually normalizing objects. And I'm not talking about edge cases like DateTime or magic properties ... they can't correctly handle straightforward POCOs because they rely of constructors, getters and setters instead of going straight for the class members. Why they do this is beyond me, because the most classic example of serialization is that done by an ORM, and all ORMs go directly for the class members and avoid messing with methods and constructors which can create bizarre side effects.

When searching for a serializer for the PHP Service Bus, my first stop was the JMS serializer. Then I noticed it cannot work without mappings and I deemed it too cumbersome to use. My next stop was the Symfony serializer but soon realized that it's full of bugs when dealing with objects. It was a huge disappointment to see that the most popular JSON serializers out there were actually unusable. It was only afterwards that I stumbled upon this library.

As such, my suggestion was to fill this void in the capabilities of the Symfony package while at the same time letting some of the Symfony popularity rub onto this library (more popular = more contributors = more eyes on the code = better implementation, in theory).

But I do see how some of the Symfony interfaces might not be completely suitable. For example the deserialize method requires the class type to deserialize into and this library does not require that since the serialization includes the type.

Ultimately, going the Symfony way is your decision, but I'm glad that you agree that refactoring it into smaller components would be beneficial, and I dare say that even if Symfony was not the way to go it would still be useful to heavily borrow from their approach (Encoders, Normalizers, Chains).

@jrbasso
Copy link
Member

jrbasso commented Jun 16, 2016

@ghola Splitting the class as you said makes totally sense for me. This will definitely be more extendable and maintainable.

Glad to hear that after trying multiple json serializer you considered our one of the best. 💯

Regarding Symfony support, we can treat it separately from this split. I believe we can have some sort of integration, but I haven't look deeply in Symfony's code to see how it can be done or what needs to be changed to be compatible in terms of input/output.

I created the #27 for us to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants