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

[Serializer] Add ->hasCacheableSupportsMethod() to CacheableSupportsMethodInterface #27105

Merged
merged 1 commit into from May 3, 2018

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Copy link
Member

commented Apr 30, 2018

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Enhances the interface introduced in #27049 to make it possible to dynamically define if "supports" methods are cacheable.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Apr 30, 2018

@nicolas-grekas nicolas-grekas requested a review from dunglas Apr 30, 2018

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2018

It's really weird for an interface called CacheableSupportsMethodInterface to be implemented by normalizers that don't have cacheable supports method. That's so counter-intuitive that it's confusing. Could it be a separate interface instead?

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:ser-cache branch from 6e2a792 to 0129e98 Apr 30, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2018

@teohhanhui why not a new name indeed. Do you have a suggestion? Naming is hard :) About a separate interface, I don't understand what you're suggesting. One interface is enough, isn't it?

@emodric

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2018

@nicolas-grekas Apart from the typo (CacheableSupportsMethodInteface -> CacheableSupportsMethodInterface), this still doesn't work unfortunately (ref #27049 (comment)). If it means anything, my normalizers do not extend AbstractNormalizer and only implement NormalizerInterface directly.

What is your aim here? Should I implement this interface in my normalizers and return false, because it doesn't work with that too.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2018

What does "doesn't work" mean? My aim it at making the cacheable mechanism BC, by making it opt-in.

@emodric

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2018

It means, my normalizers are still not used and serialization falls back to ObjectNormalizer, as per my previous comment on #27049

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

@emodric I fail to understand why. Can you debug the situation after applying this PR please?

@dunglas

This comment has been minimized.

Copy link
Member

commented May 1, 2018

Can’t we mark supportsNormalization methods @final instead to avoid the BC break? It means that someone wanting to create a “dynamic” method would have to use decoration instead of inheritance (a good practice anyway).

@emodric

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

I'll try. Give me a couple of days.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

I think there's a much simpler and BC-safe way of doing this. Have subclasses of the built-in normalizers that implement the new interface. Then it's truly opt-in and has no potential of any BC breaks.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

And we could of course use them by default in the declared services.

@dunglas

This comment has been minimized.

Copy link
Member

commented May 1, 2018

@teohhanhui we had this idea, but it only fixes the problem when using the component directly. FrameworkBundle registers the existing classes as services, and replacing them by the subclasses would be another BC break...

@dunglas

This comment has been minimized.

Copy link
Member

commented May 1, 2018

@emodric can you provide the code of your custom normalizer (even privately, using a private Gist for instance, or PM on Slack)? Does it extend a builtin normalizer, or the abstract class directly?

@emodric

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

I'm not near a computer today, but as soon as I'm back, I'll do it. I'll try to create a test app to provoke the issue, too.

In the meantime, my normalizers implement NormalizerInterface directly, without using AbstractNormalizer or extending any other normalizer. They are tagged with serializer.normalizer with default priority.

supportsNormalization dynamically checks for supported data by checking instance of provided data as well as a property in the provided object.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

@dunglas:

FrameworkBundle register the existing classes as services, and replacing them by the subclasses would be another BC break...

How so? Liskov Substitution Principle means we can substitute them with any subclasses any time, no?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

Then it's truly opt-in and has no potential of any BC breaks.

@teohhanhui what is not truly opt-in in the current PR? Also, where is the potential BC break? Please advise I don't understand. Also, how would you handle the ArrayNormalizer case (see attached patch for the way this PR does it?) Would you mind opening a PR embedding your proposal? It might be easier to understand each other this way. Thanks.

@dunglas

This comment has been minimized.

Copy link
Member

commented May 1, 2018

Liskov Substitution Principle means we can substitute them with any subclasses any time, no?

Unfortunately we cannot rely on it to replace Symfony native services. If the user uses get_class($service) === 'Foo\Bar' in his code (instead of using instanceof), it will break... And it's very frequent.

@dunglas

dunglas approved these changes May 1, 2018

@sroze

sroze approved these changes May 1, 2018

Copy link
Member

left a comment

👍 (I understand the reluctance for such thing but don't see any better way to solve the problem)

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

@dunglas:

If the user use get_class($service) === 'Foo\Bar' in his code (instead of using instanceof), it will break... And it's very frequent.

That really is the fault of the client code, or does the Symfony BC promise even cover that? :x

@emodric

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

@dunglas Here's a Symfony Standard app (it was easier to setup this way, rather than using Flex) with @nicolas-grekas ser-cache branch configured, with a sample normalizer which shows the bug: https://github.com/emodric/symfony-serializer-bug

Run the app with the built in server with php bin/console server:run -d web and access the homepage.

The output is {"fooBar": "foo", "fooBaz": "bar"}, while I'd expect it to be {"foo_bar": "foo", "foo_baz": "bar"} showing that sample normalizer is not being used, instead the fallback to ObjectNormalizer is being done. Moreover, supportsNormalization method of my sample serializer is not even called.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

@emodric thanks for the reproducer, it definitely helped, this is now fixed!

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:ser-cache branch from 0129e98 to d102963 May 1, 2018

$this->normalizerCache[$format][$type][$k] = false;
} elseif ($normalizer->supportsNormalization($data, $format)) {
$this->normalizerCache[$format][$type][$k] = true;
return $normalizer;
break;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 1, 2018

Author Member

This was the bug spotted by @emodric: we cannot return here, as the non-cacheable normalizers weren't checked yet.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:ser-cache branch from d102963 to 04b3692 May 1, 2018

@emodric

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

Thanks @nicolas-grekas ! Tested it with my test suite and now it works 👍

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

What about:

VarySupportsNormalizerInterface

  • varySupportsNormalization
  • varySupportsDenormalization

(So that there's more granular control and we can cache separately for normalization / denormalization?)

VarySupportsSerializerInterface

  • varySupports
@dunglas

This comment has been minimized.

Copy link
Member

commented May 2, 2018

Actually, we need to add different interfaces for Normalizers and Denormalizers.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented May 2, 2018

Talking on Slack with @dunglas, here is the plan we have in mind:

  • we keep one interface for both normalizers and denormalizers, because it makes DX simpler (only one method to implement), and also because it doesn't prevent anything: the uncommon case of having a cacheable supportsNormalization and non-cacheable one for denormalization is achievable by creating two separate classes for normalization and denormalization. Yes, it adds boilerplate. But the uncommon case should not force more boilerplate to the common case.
  • we introduce TypeNormalizerInterface and TypeDenormalizerInterface in 4.2, with supportsTypeNormalization/supportsTypeDenormalization methods that take only $type, $format = null as arguments.

This way, we have the best flexibility for all cases.

About the name here, despite the proposal (thanks for it), I'd still keep this PR as is.

PR ready on my side.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

The name CacheableSupportsMethodInterface is misleading if it can be either cacheable or not cacheable. (When the name says "cacheable", it better mean just that.)

@dunglas

This comment has been minimized.

Copy link
Member

commented May 3, 2018

Thank you @nicolas-grekas.

@dunglas dunglas merged commit 04b3692 into symfony:master May 3, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

dunglas added a commit that referenced this pull request May 3, 2018

feature #27105 [Serializer] Add ->hasCacheableSupportsMethod() to Cac…
…heableSupportsMethodInterface (nicolas-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Serializer] Add ->hasCacheableSupportsMethod() to CacheableSupportsMethodInterface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Enhances the interface introduced in #27049 to make it possible to dynamically define if "supports" methods are cacheable.

Commits
-------

04b3692 [Serializer] Add ->hasCacheableSupportsMethod() to CacheableSupportsMethodInterface
@dunglas

This comment has been minimized.

Copy link
Member

commented May 3, 2018

@teohhanhui can you propose a better name in a new PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.