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?

@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?

@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.

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

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

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

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).

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

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.

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

@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...

@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?

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.

@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?

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.

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:

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

@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.

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

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

What about:

VarySupportsNormalizerInterface

• varySupportsNormalization
• varySupportsDenormalization

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

VarySupportsSerializerInterface

• varySupports

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

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.

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

Thank you @nicolas-grekas.

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