Skip to content

[Serializer] Add a more global interface with all method implement by the implemantation #43325

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.

Sign up for GitHub

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

Jump to bottom
Closed
lyrixx opened this issue Oct 5, 2021 · 16 comments
Closed

[Serializer] Add a more global interface with all method implement by the implemantation #43325

lyrixx opened this issue Oct 5, 2021 · 16 comments
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) Serializer

Comments

@lyrixx
Copy link
Member

lyrixx commented Oct 5, 2021 •
edited
Loading

It's very common in a service to inject the normalizer, and to use normalize() or denormalize()
However, theses methods does not exist on SerializerInterface.

So, if we want to use normalize(), denormalize(), and serializer,we need to inject 3 services: NormalizerInterface, DenormalizerInterface, and SerializerInterface. Not really handy !

To be honest, I always inject the SerializerInterface, because I know methods exists on the implementations. But on every projects, I have to ignore error reported by static analysis.

So, what do you think of creating an interface that embrace all three others?

Side question, with PHP 8.1 we have intersection types, how does behave the containers with the following signature

public function __construct(NormalizerInterface&DenormalizerInterface&SerializerInterface $serializer) ?
@lyrixx lyrixx added Serializer RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Oct 5, 2021
@ro0NL
Copy link
Contributor

ro0NL commented Oct 5, 2021

What's the usecase?

@lyrixx
Copy link
Member Author

lyrixx commented Oct 5, 2021

So, if we want to use normalize(), denormalize(), and serializer,we need to inject 3 services: NormalizerInterface, DenormalizerInterface, and SerializerInterface. Not really handy !

avoid this (or avoid ignoring phpstan error)

@ro0NL
Copy link
Contributor

ro0NL commented Oct 5, 2021

Understood. I meant what usecase requires all 3 services?

Generally I'd prefer wiring an intersection type, rather than introducing a new one.

@stof
Copy link
Member

stof commented Oct 5, 2021

Side question, with PHP 8.1 we have intersection types, how does behave the containers with the following signature

I don't think the DI component supports intersection types yet for autowiring. @nicolas-grekas can you confirm ?

@nicolas-grekas
Copy link
Member

it doesn't, we never thought about it yet

@lyrixx
Copy link
Member Author

lyrixx commented Oct 5, 2021 •
edited
Loading

Understood. I meant what usecase requires all 3 services?

you are right, it's not common for all interfaces, but normalizer + denormalizer is common to me. Think about the layer between your data (POPO) and Elasticsearch. You want to normalizer before sending to ES, then denormalize when getting the data back.

We can generalize the use case: it occurs when you want to use the serializer to format the data you send / get from a database (or API, you get the point:) )

You could say: Why not using serialize / deserialize directly: some time, the lib does the json_encode / json_decode on his side.

@ro0NL
Copy link
Contributor

ro0NL commented Oct 5, 2021

+1 for combining normalizers with its counter denormalize part

AFAIK it's the common case: AbstractNormalizer implements NormalizerInterface, DenormalizerInterface

@lyrixx
Copy link
Member Author

lyrixx commented Oct 5, 2021

What would be the name of the interface ?

@ro0NL
Copy link
Contributor

ro0NL commented Oct 5, 2021

i'd opt for deprecating DenormalizerInterface, favoring NormalizerInterface. But NormalizerInterface&DenormalizerInterface is a valid type also :}, if not cumbersome.

Maybe intersection support in DI is sufficient ...

@lyrixx
Copy link
Member Author

lyrixx commented Oct 12, 2021

Actually, I don't what to do

  • 🅰️ wait for PHP 8.1 (NormalizerInterface&DenormalizerInterface and support in the DIC)
  • 🅱️ make a PR to symfony to deprecate an interface, and add more method to another one. I don't really like it because it will annoy many people.

@ro0NL
Copy link
Contributor

ro0NL commented Oct 12, 2021

Then a new type 🤷

@lyrixx
Copy link
Member Author

lyrixx commented Oct 12, 2021

But I can not find a name :/

@ro0NL
Copy link
Contributor

ro0NL commented Oct 12, 2021 •
edited
Loading

to me normalization comes with de-normalization, hence i think NormalizerInterface is the best name. Or both separate as currently :D

NormalizerDenormalizeInterface
Normalizer2Interface

i've no opinion what would be more annoying, a new symbol vs a new method. The latter seems better to discover 🤷

@nicolas-grekas nicolas-grekas mentioned this issue Oct 13, 2021
Merged
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 13, 2021 •
edited
Loading

I propose #43479 to fix this issue.

derrabus added a commit that referenced this issue Oct 13, 2021
@derrabus
feature #43479 [DependencyInjection] autowire union and intersection …
21528c6 
…types (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection] autowire union and intersection types

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #43325
| License       | MIT
| Doc PR        | -

This PR allows autowiring an argument of type `NormalizerInterface&DenormalizerInterface` if both individual types have a corresponding autowiring alias, and if both aliases point to the very same service.

This works the same with union types.

Commits
-------

aba31f9 [DependencyInjection] autowire union and intersection types
@ogizanagi
Copy link
Contributor

#43479 is merged, agree to close this one?

@lyrixx
Copy link
Member Author

lyrixx commented Oct 14, 2021

Sure 👍🏼 it'll totally fix my issue. thanks @nicolas-grekas

@fabpot fabpot closed this as completed Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed) Serializer
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @nicolas-grekas @lyrixx @stof @ro0NL @ogizanagi