Skip to content

optimization - don't normalize scalar data#2029

Closed
SergeyMilegov wants to merge 2 commits into
api-platform:masterfrom
SergeyMilegov:master
Closed

optimization - don't normalize scalar data#2029
SergeyMilegov wants to merge 2 commits into
api-platform:masterfrom
SergeyMilegov:master

Conversation

@SergeyMilegov

@SergeyMilegov SergeyMilegov commented Jun 19, 2018

Copy link
Copy Markdown
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
License MIT

Please update this template with something that matches your PR

@soyuka

soyuka commented Jun 19, 2018

Copy link
Copy Markdown
Member

tests would need a update, 👍 for the optimization though

@teohhanhui

Copy link
Copy Markdown
Contributor

-1

It's a feature, not a bug. With the caching (Symfony 4.1 and #1988), there should not be any overhead after the first time anyway, if there's no normalizer registered for that particular property.

@soyuka

soyuka commented Jun 19, 2018

Copy link
Copy Markdown
Member

It's a feature, not a bug. With the caching (Symfony 4.1 and #1988), there should not be any overhead after the first time anyway, if there's no normalizer registered for that particular property.

We're still skipping some function calls no?

@teohhanhui

Copy link
Copy Markdown
Contributor

This will break the use case of registering normalizers for scalar types. It just cannot work.

@bendavies

Copy link
Copy Markdown
Contributor

@dunglas

dunglas commented Jun 19, 2018

Copy link
Copy Markdown
Member

I agree with @teohhanhui, 👎 on my side.

@SergeyMilegov

SergeyMilegov commented Jun 20, 2018

Copy link
Copy Markdown
Author

normalizers for scalar types...

WAT?
I made it to prevent normalization for scalar types because it is the most useless staff I can imagine.

@SergeyMilegov

SergeyMilegov commented Jun 20, 2018

Copy link
Copy Markdown
Author

It's a feature, not a bug. With the caching (Symfony 4.1 and #1988), there should not be any overhead after the first time anyway, if there's no normalizer registered for that particular property.

What if I am using symfony 3.3...

@teohhanhui

Copy link
Copy Markdown
Contributor

@SergeyMilegov Normalizers for scalar types are not useless. Just because you don't use them, it doesn't mean that there are no valid use cases for them. 😄

@teohhanhui teohhanhui closed this Jun 20, 2018
@SergeyMilegov

Copy link
Copy Markdown
Author

Normalizers for scalar types are not useless. Just because you don't use them, it doesn't mean that there are no valid use cases for them. smile

But how can I disable it if I don't need it?

@teohhanhui

Copy link
Copy Markdown
Contributor

I think that's a good question. Do you have any suggestions? It'd be good to have in the Symfony Serializer too, I think.

@SergeyMilegov

Copy link
Copy Markdown
Author

Symfony doesn't normalize properties: Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::getAttributeValue
It is ApiPlatform's feature.
So you can add parameter to the config and apply my changes depends on this parameter.

@teohhanhui

Copy link
Copy Markdown
Contributor

The AbstractObjectNormalizer is only an implementation. In fact, we could say that normalizing / denormalizing scalar values is a supported use case in Symfony Serializer, if we look at for example: https://github.com/symfony/symfony/blob/v4.1.0/src/Symfony/Component/Serializer/Serializer.php#L214

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

Successfully merging this pull request may close these issues.

5 participants