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

Use composition for Normalizers #2579

Closed

Conversation

joelwurtz
Copy link
Contributor

@joelwurtz joelwurtz commented Mar 4, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #2355
License MIT
Doc PR /

This is a beginning for #2355 to use composition over inheritance in the various normalizer. Goal is to be decoupled from the ObjectNormalizer so someone can still use JSON LD / Hydra / Json API with a custom normalizer or a different implementation.

There is a lot of questions that i wonder, that's why it's far from being complete but it shows where i want to go.

Some questions i have:

  • Should we have a new class instead ? (easier for depreciation)
  • Does every normalizer should use composition (like i began to do) or should we add a generic decoupling instead (not sure if it will be easier)
  • Some behavior may be not available in the sub normalizer (like object to populate) is there a way to enforce this or should we just tell the user to take care of this with documentation ?

@joelwurtz
Copy link
Contributor Author

Also began to split the AbstractItemNormalizer, as you can see i extract the context initlization into its own normalizer and also the data transformation into its own normalizer.

@dunglas
Copy link
Member

dunglas commented Mar 4, 2019

Let's see where it brings us. For now I'm not sure to fully understand the changes, but the final goal is 100% legit, and we'll improve the design (next step, or parallel step, would be to the same thing in the Symfony Serializer)

@joelwurtz
Copy link
Contributor Author

For now I'm not sure to fully understand the changes

Me neither xD, there is a lot of code that is duplicated from ObjectNormalizer and actually trying to see the little differences betweens (like iri convertion / relation normalizatiing and others stuffs)

I think a may have a way to completly remove the AbstractItemNormalizer, but this may require new methods in some interfaces.

I will try to do a first shot by creating a new way of normalization (withtout the AbstractItemNormalizer) without touching the existing implementation so we can spot / benchmark / tests behaviors in parallel.

@dunglas
Copy link
Member

dunglas commented Mar 4, 2019

Maybe would it be easier to start refactoring the Symfony component first, then use its new extensions points that need to be created anyway. WDYT?

@joelwurtz
Copy link
Contributor Author

joelwurtz commented Mar 4, 2019

I prefer to refactor it here first, since api platform normalizers are heavily based on symfony serializer system and features, having a look first at the blockers and usage when extending it to the maximum will give us a better look on how to refactor better symfony.

Also i think we can get something nice without touching symfony (but maybe i'm wrong).

@joelwurtz joelwurtz force-pushed the feature/normalizers-composition branch from 768d8e8 to a07efa7 Compare March 23, 2019 22:30
@joelwurtz
Copy link
Contributor Author

joelwurtz commented Mar 24, 2019

So made a lot of revert, and begin to work in a new class, i will handle deprecation at the end if final design is accepted (mainly deprecate the old class to use the new one)

ATM there is a good exemple for the JsonLdItemNormalizer

So overall design will look like this: Every feature is splitted into its own normalizer:

  • ResourceClassNormalizer handle the extraction of the resource class and check if the given object can be normalized / denormalized as a resource class
  • DataTransformerNormalizer handle data transformations using the DataTtransformerInterface
  • No more custom AbstractItemNormalizer, it directly use the ObjectNormalizer of symfony
  • Each "format" normalizers (json ld / json api / ...) add the necessary metadata to the data (like before)

Each of this normalizer use a sub normalizer to handle data, so in fact to handle json ld data it need a normalizer chain like this:

ResourceClassNormalizer -> JsonLdItemNormalizer -> DataTransformerNormalizer -> ObjectNormalizer (Symfony)

Also i believe this will allow to remove all code that need to check about handling non resource class, as someone will be able to inject its own chain of normalizer and give necessary info into the context if needed.

fabpot added a commit to symfony/symfony that referenced this pull request Mar 27, 2019
…ributes (joelwurtz)

This PR was squashed before being merged into the 4.2 branch (closes #30711).

Discussion
----------

[Serializer] Use object class resolver when extracting attributes

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no (not sure)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Current ObjectNormalizer was not using the object class resolver when extracting attributes, i found this when trying to refactor api platform normalizers and dealing with doctrine proxy objet having __initialize__ property not wanted cf api-platform/core#2579

I don't think this is BC break, but maybe some people using this object class resolver can get different behavior ?

Commits
-------

1d8b5af [Serializer] Use object class resolver when extracting attributes
@teohhanhui
Copy link
Contributor

@joelwurtz Just a heads up, I've changed the normalization of non-resource in #2679

@joelwurtz
Copy link
Contributor Author

Thanks for the heads up, look better that way

@teohhanhui
Copy link
Contributor

Another heads up, denormalization of non-resources also uses composition now in #2797.

Base automatically changed from master to main January 23, 2021 21:59
@stale
Copy link

stale bot commented Nov 5, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 5, 2022
@soyuka
Copy link
Member

soyuka commented Oct 17, 2023

same reason I closed #2355 we're now watching how the symfony/symfony#51718 work progresses! thanks for the work!

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

Successfully merging this pull request may close these issues.

None yet

4 participants