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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use separate instance of ItemNormalizer for handling non-resource #2592

Conversation

teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented Mar 8, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #2563, #2591, ...; alternative to #2566 (reverted here)
License MIT
Doc PR N/A

This should ensure the same caching of supports* methods as before for resource classes. For non-resource classes (handled by a separate instance), the supports* methods use the $context, thus not cacheable.

Not sure about the performance impact (positive or negative), but I think this is cleaner as we don't indiscriminately say yes to things we might not be able to handle (provided we're at a low enough priority). And there's no need for use_api_platform and/or special treatment of some formats.

Big thanks to @soyuka and @dunglas for the discussions and for bearing with me! 馃槤

@alanpoulain
Copy link
Member

It seems it is a better way to do it indeed.

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 I like it

@teohhanhui teohhanhui merged commit b3f4713 into api-platform:2.4 Mar 9, 2019
@teohhanhui teohhanhui deleted the refactor/separate-item-normalizer-non-resource branch March 9, 2019 09:33
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.

None yet

4 participants