[Serializer] Give access to the context to support* methods #19371

dunglas · 2016-07-16T16:43:31Z

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

This is a current use case to want to access the context form the supports* methods. This PR fixes this limitation.

Maybe can it be considered a bug fix?

GuilhemN · 2016-07-16T21:17:30Z

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php

    {
+        $context = 4 === func_num_args() ? func_get_arg(3) : [];


Long array syntax should be used

ogizanagi · 2016-07-16T23:07:00Z

I always wondered why it was not already the case and assumed it was by design.

However, IIRC, the Serializer previously used — in older versions of the component — a hash of the supports* methods arguments, in order to cache the normalizer/denormalizer instance to use for those arguments.
Now, should we consider the fact someone may have mimic this behavior in it's own serializer, and that the cache would be compromised by not taking care of the context (which can also create issue in order to be serialized, but that's another topic) ? It may also prevent this to be considered as a bug fix and backported as is in previous branches.

dunglas · 2016-07-17T09:50:54Z

@ogizanagi It's not a problem because we don't use the context in supports* methods of built-in normalizers. It can be broken only if someone use the context and has its own cache mechanism... But it's his responsibility.

theofidry · 2016-07-17T13:41:58Z

@dunglas I would be 👎 for this one. It is a limitation for sure, but having the context in the supports makes the lookup for the (de)normalizer much more dynamic, thus extremely hard to cache. Now let's say you have a set of data that you want to denormalize in a UserInterface object, and provide a different concrete class depending of the context, you can always have a UserDenormalizer supporting UserInterface, have a denormalizer for each case, and manually pick the right denormalizer in UserDenormalizer. If there's too many custom denormalizers, it might be worth to abstract it a bit to make it more easy, but IMO it's not a very common use case.

GuilhemN · 2016-07-17T15:00:13Z

I agree with @dunglas this is not part of the symfony promises imo.

@theofidry if you change the context the serializer output will very likely change too so i don't get what's the problem. Moreover the context shouldn't change that much.

theofidry · 2016-07-17T17:22:42Z

Moreover the context shouldn't change that much.

The context is very likely to change a lot, it already changes natively to handle proxies or cache keys. In ApiPlatform for example, we also have a lot of keys passing through the context (type, normalization_context, denormalization_context, resource_class etc.)

If you change the context the serializer output will very likely change too so i don't get what's the problem.

Yeah the output may change, but so does the context. It makes much harder to predict the result for a given context and now you have to handle the context, which is dynamic and accessible to all normalizers (hence very likely to change) for caching. The context was initially for giving additional info on how to (de)normalize a set of data. Adding it to the supports* is making use of it for which (de)normalizer can use handle it. It's not innocent, and IMO brings more trouble than it's worth.

GuilhemN · 2016-07-17T22:43:17Z

@theofidry if i'm not wrong the things such as proxy classes, cache keys, etc. are generated only when the cache is cleared ? If this is true then the context should remain the same.
I'll investigate more later thanks for your explanation ☺

Anyway imo as the context describes how to (de)serialize an object it should be possible to not execute a normalizer in case it doesn't support some part of the context.

GuilhemN · 2016-07-18T12:51:18Z

Maybe adding a new field dynamic_fields to the context would solve your issue @theofidry ?

GuilhemN · 2016-07-18T20:31:56Z

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php

    {
+        $context = 4 === func_num_args() ? func_get_arg(3) : array();


should be changed to func_num_args() > 3 ? func_get_arg(3) : array() for consistency (this is always written like that) and for clarity

theofidry · 2016-07-18T20:37:53Z

@Ener-Getick could mitigate it, but looks horribly brittle to me. I just feel like this feature is about trying to extend the usage of context for something it is not meant. I'm not the one who gets to decide, but IMO it will only add complexity to something that is already very complex and make it messier.

@dunglas is there any use case in which you want to use this feature?

GuilhemN · 2016-07-18T20:59:01Z

@theofidry I just looked the code @ogizanagi mentioned and it is no longer in the code (the normalizer to use is always determined) so there are absolutely no problem.
It's last occurrence is in 2.6 which is no longer maintained and is not compatible with symfony 3.

So there won't ever be any issues about caching here (except if someone doesn't update to the latest patches but this is not our problem...).

I'm in favor of merging that as a feature (as people were limited but didn't have a weird behaviour).

And I see several use cases. One of them is if you use normalization groups with custom normalizers, you may want to have one normalizer per group.

theofidry · 2016-07-18T21:23:33Z

@Ener-Getick I fail to see how have a dynamic array that can change from one call to another is not going to have an impact on caching. It may not have one in the Symfony code base right now but it will definitely have some in userland (I am not talking of BC, but of usability).

There are many use cases. For example if you use normalization groups with custom normalizers, you may want to have one normalizer per group.

To that I would said my argument still stands:

Now let's say you have a set of data that you want to denormalize in a UserInterface object, and provide a different concrete class depending of the context, you can always have a UserDenormalizer supporting UserInterface, have a denormalizer for each case, and manually pick the right denormalizer in UserDenormalizer. If there's too many custom denormalizers, it might be worth to abstract it a bit to make it more easy, but IMO it's not a very common use case.

So it's not that I don't see the use case (and believe me, I'm the first one abusing of it), but I don't believe it belongs to the core.

GuilhemN · 2016-07-18T21:42:42Z

@theofidry nothing is cached anymore about which normalizer to use so we can't have a problem about that ^^

Your argument doesn't work in case you have to deal with third party normalizers.
And IMO this change is also good for DX as it would make all signature consistent and should be merged at least for that.

Anyway it would be easier to have other people reviews :)

theofidry · 2016-07-18T22:18:54Z

nothing is cached anymore about which normalizer to use so we can't have a problem about that ^^

Nevermind misread your comment. Indeed if there is no caching there anymore should be alright.

mcfedr · 2016-07-27T11:07:25Z

Maybe there should be caching? For a system with many Normalizers there is likely to be some gain from do so - You can imagine a Serializer that uses code generation to make this really fast.

Thinking if the use cases for this outweigh not being able (or so able) to create a cache in the future.

It seems like if the action of a Normalizer needs to change based on the content, it could always delegate in the actual normalize/denormalize methods.

theofidry · 2016-07-27T11:25:55Z

@mcfedr caching at this level has been removed because proved to be more likely to cause bugs than anything else. The Serializer lookup is usually quite fast unless you really do weird things in your supports*(), if you want something faster still, the best thing is to cache the whole result.

dunglas · 2016-08-16T09:30:04Z

ping @symfony/deciders

ogizanagi · 2016-09-21T13:46:02Z

It would be great to have this feature for 3.2 :)

nicolas-grekas · 2016-09-21T14:02:16Z

Changing the interfaces needs a smooth upgrade path. In this case, we have no choice but create new interfaces that extend the current ones, while deprecating them.

dunglas · 2016-09-21T14:08:43Z

@nicolas-grekas It will be very difficult to find a better name than the current one. Maybe can we let interfaces as-is in 4.0+ (with the comment)? Updating every single implementation of these interfaces to use the new ones will also be a very tedious work. But I agree, breaking existing code is a worse solution and we will not do it.

nicolas-grekas · 2016-09-21T14:23:12Z

ContextAware*Interface?

dunglas · 2016-09-21T14:58:53Z

Status: needs work

dunglas · 2016-10-25T10:49:24Z

I've uncommented all the commented code. It should be ok now to merge this PR.

However I'm unsure if it's ok or not to uncomment new arguments in the Serializer class. People may have extended its supports* methods. IMO it's not common but it can be considered as a (small) BC break.

WDYT? /cc @symfony/deciders

xabbuh · 2016-10-25T18:21:20Z

@dunglas That is indeed a BC break and our BC promise forbids such a change (see http://symfony.com/doc/current/contributing/code/bc.html#changing-classes):

Type of Change Change Allowed

Public Methods

Add argument with a default value No

dunglas · 2016-10-26T05:43:49Z

@xabbuh yes I know but see this thread for the why: #19371 (comment)

We have 3 solutions if we want to merge give access to the context in supports* methods:

Merge this PR without the last commit: 1cceb99 it doesn't introduce a BC break but has a bad upgrade path
Merge this PR as is (including the last commit): it introduces a theoretical BC break but provide a smooth upgrade path (a line in the CHANGELOG should be enough for people having this BC to upgrade easily)
Keep the hack only for the Serializer class (and not for chain classes) to prevent the most likely BC but have a smooth upgrade path for other classes

dunglas · 2017-01-28T19:18:07Z

I've used the same strategy than in

symfony/src/Symfony/Component/DependencyInjection/Compiler/Compiler.php

Lines 75 to 86 in 5bd3a00

    
           if (func_num_args() >= 3) { 
        
               $priority = func_get_arg(2); 
        
           } else { 
        
               if (__CLASS__ !== get_class($this)) { 
        
                   $r = new \ReflectionMethod($this, __FUNCTION__); 
        
                   if (__CLASS__ !== $r->getDeclaringClass()->getName()) { 
        
                       @trigger_error(sprintf('Method %s() will have a third `$priority = 0` argument in version 4.0. Not defining it is deprecated since 3.2.', get_class($this), __FUNCTION__), E_USER_DEPRECATED); 
        
                   } 
        
               } 
        
               $priority = 0; 
        
           }

It should be ready now.

Status: Needs review

nicolas-grekas · 2017-02-13T10:29:15Z

src/Symfony/Component/Serializer/Encoder/ChainDecoder.php

+            if (__CLASS__ !== get_class($this)) {
+                $r = new \ReflectionMethod($this, __FUNCTION__);
+                if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
+                    @trigger_error(sprintf('Method %s() will have a second `$context = array()` argument in version 4.0. Not defining it is deprecated since 3.3.', get_class($this), __FUNCTION__), E_USER_DEPRECATED);


can't we remove this block since the class is @final now?

nicolas-grekas · 2017-02-13T10:29:39Z

src/Symfony/Component/Serializer/Encoder/ChainEncoder.php

+            if (__CLASS__ !== get_class($this)) {
+                $r = new \ReflectionMethod($this, __FUNCTION__);
+                if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
+                    @trigger_error(sprintf('Method %s() will have a second `$context = array()` argument in version 4.0. Not defining it is deprecated since 3.3.', get_class($this), __FUNCTION__), E_USER_DEPRECATED);


could be removed

nicolas-grekas · 2017-02-13T10:29:56Z

src/Symfony/Component/Serializer/Encoder/ChainEncoder.php

+            if (__CLASS__ !== get_class($this)) {
+                $r = new \ReflectionMethod($this, __FUNCTION__);
+                if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
+                    @trigger_error(sprintf('Method %s() will have a third `$context = array()` argument in version 4.0. Not defining it is deprecated since 3.3.', get_class($this), __FUNCTION__), E_USER_DEPRECATED);


could be removed

nicolas-grekas · 2017-02-13T10:30:34Z

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php

+            if (__CLASS__ !== get_class($this)) {
+                $r = new \ReflectionMethod($this, __FUNCTION__);
+                if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
+                    @trigger_error(sprintf('Method %s() will have a fourth `$context = array()` argument in version 4.0. Not defining it is deprecated since 3.3.', get_class($this), __FUNCTION__), E_USER_DEPRECATED);


make the class @final instead?

nicolas-grekas · 2017-02-13T10:31:02Z

src/Symfony/Component/Serializer/Serializer.php

+            if (__CLASS__ !== get_class($this)) {
+                $r = new \ReflectionMethod($this, __FUNCTION__);
+                if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
+                    @trigger_error(sprintf('Method %s() will have a third `$context = array()` argument in version 4.0. Not defining it is deprecated since 3.3.', get_class($this), __FUNCTION__), E_USER_DEPRECATED);


make the class @final instead?

Overriding this class is common (even if it's not a good practice IMO). Not sure it is worth it to make it final.

This reverts commit cdbf9b5.

nicolas-grekas

👍

fabpot · 2017-02-13T14:29:29Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

@@ -267,7 +267,7 @@ private function validateAndDenormalize($currentClass, $attribute, $data, $forma
                    throw new LogicException(sprintf('Cannot denormalize attribute "%s" for class "%s" because injected serializer is not a denormalizer', $attribute, $class));
                }

-                if ($this->serializer->supportsDenormalization($data, $class, $format)) {
+                if ($this->serializer->supportsDenormalization($data, $class, $format, $context)) {


@dunglas Can you check that my conflict resolution is the right one. If not, can you fix it directly on master? Thanks.

@fabpot fixed in b5d9b3b.

fabpot · 2017-02-13T14:29:31Z

Thank you @dunglas.

dunglas added the Serializer label Jul 16, 2016

carsonbot added Status: Needs Review Feature Deprecation labels Jul 16, 2016

GuilhemN reviewed Jul 16, 2016
View reviewed changes

GuilhemN reviewed Jul 18, 2016
View reviewed changes

ogizanagi mentioned this pull request Aug 28, 2016

[Serializer] Injecting additional data during serialization #18904

Closed

dunglas force-pushed the serializer_support_context branch from ff31957 to e3cfe63 Compare September 21, 2016 14:58

dunglas force-pushed the serializer_support_context branch from 007c106 to cdbf9b5 Compare October 25, 2016 10:45

nicolas-grekas removed the Deprecation label Oct 25, 2016

javiereguiluz added this to the 3.3 milestone Nov 7, 2016

ogizanagi mentioned this pull request Dec 12, 2016

ArrayDenormalizer::supportsDenormalization, array denormalizer #20837

Open

dunglas force-pushed the serializer_support_context branch 2 times, most recently from 518b79b to 21c3f49 Compare January 28, 2017 19:17

dunglas modified the milestones: 3.3, 3.x Feb 4, 2017

nicolas-grekas reviewed Feb 13, 2017

View reviewed changes

dunglas added 5 commits February 13, 2017 13:50

[Serializer] Give access to the context to support* methods

7a4e9ec

Remove commented code

1de7609

Revert "Remove commented code"

f6d4479

This reverts commit cdbf9b5.

Fix CS

3bf4bf1

Better deprecations

f70762a

dunglas force-pushed the serializer_support_context branch from 21c3f49 to e9b56ec Compare February 13, 2017 12:59

Remove useless checks

661005e

dunglas force-pushed the serializer_support_context branch from e9b56ec to 661005e Compare February 13, 2017 13:01

nicolas-grekas approved these changes Feb 13, 2017

View reviewed changes

nicolas-grekas added the Ready label Feb 13, 2017

fabpot reviewed Feb 13, 2017

View reviewed changes

fabpot closed this in 17b4363 Feb 13, 2017

javiereguiluz mentioned this pull request Feb 26, 2017

[Serializer] Support* methods now can access to the context symfony/symfony-docs#7541

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

xabbuh mentioned this pull request Jul 19, 2019

add $context argument to supportsNormalization() symfony/symfony-docs#11994

Merged

		{
		$context = 4 === func_num_args() ? func_get_arg(3) : array();

[Serializer] Give access to the context to support* methods #19371

[Serializer] Give access to the context to support* methods #19371

Conversation

dunglas commented Jul 16, 2016 • edited by nicolas-grekas Loading

Choose a reason for hiding this comment

ogizanagi commented Jul 16, 2016 • edited Loading

dunglas commented Jul 17, 2016 • edited Loading

theofidry commented Jul 17, 2016

GuilhemN commented Jul 17, 2016 • edited Loading

theofidry commented Jul 17, 2016

GuilhemN commented Jul 17, 2016 • edited Loading

GuilhemN commented Jul 18, 2016

Choose a reason for hiding this comment

theofidry commented Jul 18, 2016

GuilhemN commented Jul 18, 2016 • edited Loading

theofidry commented Jul 18, 2016

GuilhemN commented Jul 18, 2016 • edited Loading

theofidry commented Jul 18, 2016

mcfedr commented Jul 27, 2016

theofidry commented Jul 27, 2016

dunglas commented Aug 16, 2016

ogizanagi commented Sep 21, 2016

nicolas-grekas commented Sep 21, 2016

dunglas commented Sep 21, 2016 • edited Loading

nicolas-grekas commented Sep 21, 2016

dunglas commented Sep 21, 2016

dunglas commented Oct 25, 2016

xabbuh commented Oct 25, 2016

dunglas commented Oct 26, 2016

dunglas commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 13, 2017

dunglas commented Jul 16, 2016 •

edited by nicolas-grekas

Loading

ogizanagi commented Jul 16, 2016 •

edited

Loading

dunglas commented Jul 17, 2016 •

edited

Loading

GuilhemN commented Jul 17, 2016 •

edited

Loading

GuilhemN commented Jul 17, 2016 •

edited

Loading

GuilhemN commented Jul 18, 2016 •

edited

Loading

GuilhemN commented Jul 18, 2016 •

edited

Loading

dunglas commented Sep 21, 2016 •

edited

Loading