SonataPageBundle icon indicating copy to clipboard operation
SonataPageBundle copied to clipboard

Proposal to change transformer for symfony serializer

Open eerison opened this issue 3 years ago • 32 comments

The idea of this proposal is keep the code simple and using more Symfony features.

The idea of Transformer is convert the object to json or the other way round, But IMO It can be handled "easily" for symfony serializer componente

eerison avatar Jul 28 '22 03:07 eerison

Do you plan to try a PR ?

VincentLanglet avatar Jul 28 '22 07:07 VincentLanglet

sounds good, do you want to try it? how exactly does doctrine type="json" work there? you probably don't need to serialize the object yourself, but just need the normalizer part into an array, so doctrine can encode it into json, right?

Hanmac avatar Jul 28 '22 08:07 Hanmac

Do you plan to try a PR ?

Yep, if we agree I can provide a PR!

the idea is: first coverage the class, to make sure that we not forget anything, Definitely we need this before touch in Transformer, almost no test: https://app.codecov.io/gh/sonata-project/SonataPageBundle/blob/4.x/src/Entity/Transformer.php

Sounds good, do you want to try it?

Well I can try it, But if you want to do, I'm fine with this too, I can check others things that are necessary for 4.0 release. (I'm still on #1497 😄 )

how exactly does doctrine type="json" work there? you probably don't need to Serialize the object yourself, but just need the Normalize part into an array, so doctrine can encode it into json, right?

well we can do like this, convert to array and let the database handle this for json, maybe we can check this with Phpstan somehow too, But well we need to test :)

eerison avatar Jul 28 '22 08:07 eerison

if really needed, i saw https://symfony.com/doc/4.4/components/serializer.html#built-in-normalizers especially JsonSerializableNormalizer

This means that nested JsonSerializable classes will also be normalized.

it might help us to normalize the Block with the Child Blocks?

EDIT: but it doesn't support denormalize :(

Hanmac avatar Jul 28 '22 09:07 Hanmac

if really needed, i saw https://symfony.com/doc/4.4/components/serializer.html#built-in-normalizers especially JsonSerializableNormalizer

This means that nested JsonSerializable classes will also be normalized.

it might help us to normalize the Block with the Child Blocks?

EDIT: but it doesn't support denormalize :(

Well I use this to parse object -> json and json -> object, and it works quite well

something like this: https://symfony.com/doc/4.4/components/serializer.html#deserializing-in-an-existing-object

eerison avatar Jul 28 '22 09:07 eerison

if really needed, i saw https://symfony.com/doc/4.4/components/serializer.html#built-in-normalizers especially JsonSerializableNormalizer

This means that nested JsonSerializable classes will also be normalized.

it might help us to normalize the Block with the Child Blocks? EDIT: but it doesn't support denormalize :(

Well I use this to parse object -> json and json -> object, and it works quite well

something like this: https://symfony.com/doc/4.4/components/serializer.html#deserializing-in-an-existing-object

you don't want to deserialize but to just denormalize, and i don't know if that option exist there too

EDIT: i stand corrected, it exist for https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php too

Hanmac avatar Jul 28 '22 09:07 Hanmac

if really needed, i saw https://symfony.com/doc/4.4/components/serializer.html#built-in-normalizers especially JsonSerializableNormalizer

This means that nested JsonSerializable classes will also be normalized.

it might help us to normalize the Block with the Child Blocks? EDIT: but it doesn't support denormalize :(

Well I use this to parse object -> json and json -> object, and it works quite well something like this: https://symfony.com/doc/4.4/components/serializer.html#deserializing-in-an-existing-object

you don't want to deserialize but to just denormalize, and i don't know if that option exist there too

EDIT: i stand corrected, it exist for https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php too

it's enough, isn't it?

Screenshot 2022-07-28 at 13 16 58

eerison avatar Jul 28 '22 11:07 eerison

my thought was that you don't need the serializer when you just normalizing/denormalizing

Hanmac avatar Jul 28 '22 11:07 Hanmac

my thought was that you don't need the serializer when you just normalizing/denormalizing

well But I need to convert from object to array -> save into the database and array to object -> to use into the code

or am i missing something?

eerison avatar Jul 28 '22 11:07 eerison

my thought was that you don't need the serializer when you just normalizing/denormalizing

or do you mean the Selializer class?

eerison avatar Jul 28 '22 11:07 eerison

i meant the serializer class, but we still might use it for the nice functions

object to array -> save into the database and array to object -> to use into the code

is what the normalizer/denormalizer does

Hanmac avatar Jul 28 '22 11:07 Hanmac

i meant the serializer class, but we still might use it for the nice functions

object to array -> save into the database and array to object -> to use into the code

is what the normalizer/denormalizer does

well maybe you could provider a simple example how we should use this!

eerison avatar Jul 28 '22 11:07 eerison

there is an example of just using normalizer with denormalize https://symfony.com/doc/4.4/components/serializer.html#camelcase-to-snake-case

Hanmac avatar Jul 28 '22 12:07 Hanmac

also should we use make own Serializer instance or should we use SerializerInterface and let Symfony define it? https://symfony.com/doc/current/serializer.html#using-the-serializer-service

Hanmac avatar Jul 28 '22 12:07 Hanmac

hmmm good question!

I would use SymfonyInterface and pass the instace that we configurate! why?

because we use snake_case but the guy configure CamelCase in his config, then it'll convert to propertyName instead of property_name

eerison avatar Jul 28 '22 12:07 eerison

there is an example of just using normalizer with denormalize https://symfony.com/doc/4.4/components/serializer.html#camelcase-to-snake-case

hmmm nice, maybe we could depends of ObjectNormalizerInterface and not the SerializerInterface.

eerison avatar Jul 28 '22 12:07 eerison

the DenormalizerInterface probably doesn't exist as service like the SerializerInterface does

Hanmac avatar Jul 28 '22 13:07 Hanmac

the DenormalizerInterface probably doesn't exist as service like the SerializerInterface does

well I would choose the option to have this injected in the service instead of use the new ObjectNormalizer into the class/code!

eerison avatar Jul 28 '22 13:07 eerison

@eerison if you can try to make an MR for the coverage, then i can later try to make an MR for the serializer (if you didn't already)

Hanmac avatar Jul 28 '22 15:07 Hanmac

@eerison if you can try to make an MR for the coverage, then i can later try to make an MR for the serializer (if you didn't already)

well I'm still working on #1497

eerison avatar Jul 28 '22 15:07 eerison

@VincentLanglet assuming i want to write testcases for the Transformer functions: Using the Transformer as real objects and the manager as mocks, do i need to mock the Page/Block/Snapshot entities too? Or can i use the tests/App/Entity ones to check if the Snapshot has the same values as the Page/Block?

Hanmac avatar Aug 01 '22 07:08 Hanmac

I guess you can mock the DB, you just need to check if after convert from object to array (and the other way round 😄 ), it's as expected!

Maybe you can create one db test, just to make sure that passing array to content, it'll be saved as json

eerison avatar Aug 01 '22 08:08 eerison

for the (first) transformer test, i don't need the DB, i just mock the manager, i was just thinking about using the fake Entity objects so i don't need to mock getContent and setContent stuff and all the other getter/setter when the Transformer is coping from Page to Snapshot and vice versa.

Hanmac avatar Aug 01 '22 08:08 Hanmac

for the (first) transformer test, i don't need the DB, i just mock the manager, i was just thinking about using the fake Entity objects so i don't need to mock getContent and setContent stuff and all the other getter/setter when the Transformer is coping from Page to Snapshot and vice versa.

well if you can cover the functionality with a fake entity I'm fine with this :)

eerison avatar Aug 01 '22 08:08 eerison

@VincentLanglet assuming i want to write testcases for the Transformer functions: Using the Transformer as real objects and the manager as mocks, do i need to mock the Page/Block/Snapshot entities too? Or can i use the tests/App/Entity ones to check if the Snapshot has the same values as the Page/Block?

i dunno this bundle to give good advice without a PR to rely on. You can start with the simpler solution you have in mind

VincentLanglet avatar Aug 01 '22 12:08 VincentLanglet

Hey @Hanmac I don't know if it helps somehow, But into the block bundle there is a definition for serializer, maybe it help somehow: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Resources/config/serializer/Model.BaseBlock.xml

or maybe we should define a new group there for fake/group block(container block), I guess make sense it be defined in block-bundle because we are handle block normalizer and denormalizer, what do you think?

eerison avatar Aug 03 '22 07:08 eerison

Hey @Hanmac I don't know if it helps somehow, But into the block bundle there is a definition for serializer, maybe it help somehow: https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Resources/config/serializer/Model.BaseBlock.xml

or maybe we should define a new group there for fake/group block(container block), I guess make sense it be defined in block-bundle because we are handle block normalizer and denormalizer, what do you think?

i will check this out later, first i do am MR for Transformer testcases

Hanmac avatar Aug 03 '22 08:08 Hanmac

there was https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Resources/config/serializer/Model.Block.xml and https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Resources/config/serializer/Model.Page.xml i might recycle for my possible change

Hanmac avatar Aug 03 '22 09:08 Hanmac

AFAIK , those are for the JMS serializer, used on 3.x for the API (already removed on 4.x). Not sure if that works for symfony/serializer.

jordisala1991 avatar Aug 03 '22 09:08 jordisala1991

@eerison you use this Snapshots right? can you explain to me what the SnapshotPageProxy is for?

and also if it would be okay to change the format of the Json Content? For example, the https://github.com/symfony/symfony/blob/6.1/src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php may use a different format for the DateTime values. or should i try to keep the U format?

Hanmac avatar Aug 03 '22 12:08 Hanmac