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

Transformer use Symfony Serializer #1522

Merged
merged 38 commits into from
Aug 10, 2023

Conversation

Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Aug 5, 2022

Subject

This makes the Transformer use Symfony Serializer to normalize/denormalize the Content Array.

Under 4.4 Symfony i had the following restrictions:

  • it wasn't easy to exclude attributes, you only could ignore them either global via name, but don't class specific like i wanted, the current solution is to use a group name
  • the group name also has the benefit that it orders the attributes, without it, it seems to use the order inside the php class
  • You can't denormalize for an Interface, or abstract class. so when it tired to load the block children, it had no idea what to do. And i didn't want to use a discriminator map and type attribute inside the array. I solved this using my Own Denormalizer that is looking for an Interface and Context parameters.

I will add more Testcases soon with doing a functional test using the Symfony Kernel.

I am targeting this branch, because i don't know if that could count as BC or not.

Closes #1503.

Changelog

### Changed
- Uses Symfony Serializer to transform Page and Block Models into array for json

### Deprecated
- Not implementing PageInterface::removeChild
- Not implementing PageInterface::removeBlock
- Not passing Serializer to Transformer as 5th argument

@Hanmac Hanmac changed the title Transformer use Symfony Serializer Draft: Transformer use Symfony Serializer Aug 5, 2022
@VincentLanglet
Copy link
Member

Adding a mandatory param to a constructor is a BC break.
The BC way is to use null as a default and if null is provided either do new Service(), either use the old logic.

Here I'm not sure it's worth it to provide a BC PR, it might be easier to target directly 4.x
WDYT @jordisala1991 ?

@jordisala1991
Copy link
Member

I think it might be nice to have this branch for testing purposes on real apps that are actually using 3.x, but the real PR should target 4.x

@eerison
Copy link
Contributor

eerison commented Aug 6, 2022

Adding a mandatory param to a constructor is a BC break. The BC way is to use null as a default and if null is provided either do new Service(), either use the old logic.

Here I'm not sure it's worth it to provide a BC PR, it might be easier to target directly 4.x WDYT @jordisala1991 ?

I guess just add the serializer as optional, and if it's not passed get from the container, it's fine

@eerison
Copy link
Contributor

eerison commented Aug 6, 2022

I think it might be nice to have this branch for testing purposes on real apps that are actually using 3.x, but the real PR should target 4.x

Do you think it's too much for a minor?

@VincentLanglet
Copy link
Member

Adding a mandatory param to a constructor is a BC break. The BC way is to use null as a default and if null is provided either do new Service(), either use the old logic.
Here I'm not sure it's worth it to provide a BC PR, it might be easier to target directly 4.x WDYT @jordisala1991 ?

I guess just add the serializer as optional, and if it's not passed get from the container, it's fine

You dont have the container either in this class

@Hanmac Hanmac changed the base branch from 3.x to 4.x August 6, 2022 17:18
@Hanmac
Copy link
Contributor Author

Hanmac commented Aug 6, 2022

For the last issue, should i just suppress it?

if i don't it just complains somewhere else in the Denormalizer

@VincentLanglet
Copy link
Member

For the last issue, should i just suppress it?

if i don't it just complains somewhere else in the Denormalizer

The interface ContextAwareDenormalizerInterface is deprecated since Symfony 6.1
https://github.com/symfony/serializer/blob/6.1/Normalizer/ContextAwareDenormalizerInterface.php#L21

Since like it will be added directly to https://github.com/symfony/Serializer/blob/6.1/Normalizer/DenormalizerInterface.php#L59

So I would recommend to not use the interface.
Then we will see the issue you get

@Hanmac
Copy link
Contributor Author

Hanmac commented Aug 6, 2022

especially this one i don't know how to fix:

supportsDenormalization() invoked with 4 parameters, 2-3 required.

@VincentLanglet
Copy link
Member

For

Method Sonata\PageBundle\Serializer\InterfaceDenormalizer::supportsDenormalization() has parameter $context with no value type specified in iterable type array.

You need to add phpdoc.

For

Method Symfony\Component\Serializer\Normalizer\DenormalizerInterface::supportsDenormalization() invoked with 4 parameters, 2-3 required.
We will ignore the error (@phpstna-ignore-next-line) with a message to explain why

For

@psalm-suppress MoreSpecificImplementedParamType

I think we have to make the serializerAwareInterface generic in Symfony

@VincentLanglet
Copy link
Member

@Hanmac there is a DenormalizerAwareInterface.

Dont you think you should both implement DenormalizerAwareInterface and SerializerAwareInterface ?
This will avoid to override the phpdoc of setSerializer and avoid the psalm issue.

@Hanmac
Copy link
Contributor Author

Hanmac commented Aug 6, 2022

@Hanmac there is a DenormalizerAwareInterface.

Dont you think you should both implement DenormalizerAwareInterface and SerializerAwareInterface ? This will avoid to override the phpdoc of setSerializer and avoid the psalm issue.

hm i was using ArrayDenormalizer as inspiration, seems it changed after 4.4 for some reason

@Hanmac
Copy link
Contributor Author

Hanmac commented Aug 6, 2022

hm where does it want me to do the psalm thing?

src/Serializer/InterfaceDenormalizer.php Outdated Show resolved Hide resolved
src/Serializer/InterfaceDenormalizer.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Member

hm where does it want me to do the psalm thing?

I would say it's because you're using \* and not \**

@SonataCI
Copy link
Collaborator

SonataCI commented Aug 6, 2022

Could you please rebase your PR and fix merge conflicts?

@SonataCI
Copy link
Collaborator

SonataCI commented Aug 7, 2022

Could you please rebase your PR and fix merge conflicts?

@Hanmac
Copy link
Contributor Author

Hanmac commented Aug 7, 2022

tests are broken, but not by me

@VincentLanglet
Copy link
Member

tests are broken, but not by me

What do you mean ? Tests are green

VincentLanglet
VincentLanglet previously approved these changes Aug 3, 2023
@VincentLanglet
Copy link
Member

Just a linter error to fix

VincentLanglet
VincentLanglet previously approved these changes Aug 3, 2023
@Hanmac
Copy link
Contributor Author

Hanmac commented Aug 7, 2023

also happy anniversary for this PR 🥳

src/Entity/Transformer.php Outdated Show resolved Hide resolved
src/Entity/Transformer.php Show resolved Hide resolved
@VincentLanglet
Copy link
Member

Can you update the ## Changelog part @Hanmac ?

@Hanmac
Copy link
Contributor Author

Hanmac commented Aug 10, 2023

i'm not very good at writing such change logs

like do we need to add the other deprecations too?

@VincentLanglet
Copy link
Member

VincentLanglet commented Aug 10, 2023

i'm not very good at writing such change logs

like do we need to add the other deprecations too?

It would be something like

Changed:
- Use serializer to do ....

Deprecated:
- Not implementing PageInterface::foo
- Not implementing PageInterface::bar
- Not passing Serializer to FooService as 5th argument (or 4th or 3rd...)

@VincentLanglet VincentLanglet merged commit bb9a92c into sonata-project:4.x Aug 10, 2023
23 checks passed
@VincentLanglet
Copy link
Member

VincentLanglet commented Aug 10, 2023

Thanks !

Since I go in vacations at the end of the week, I prefer to delay the release.

@Hanmac Hanmac deleted the transformerSerializer branch August 10, 2023 13:51
@VincentLanglet
Copy link
Member

https://github.com/sonata-project/SonataPageBundle/releases/tag/4.6.0

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

Successfully merging this pull request may close these issues.

Proposal to change transformer for symfony serializer
5 participants