Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Argument objects#19277
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| }elseif ($allowed && !$ignored && (isset($data[$key]) ||array_key_exists($key,$data))) { | ||
| $params[] =$data[$key]; | ||
| // don't run set for a parameter passed to the constructor | ||
| unset($data[$key]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why removing the unset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bad copy/paste, should be kept
dunglas commentedJul 3, 2016
This case was omitted when adding deserialization support for complex relations (in 3.1). IMO it should be merged in 3.1. Can you add a test for this? |
| * @throws RuntimeException | ||
| */ | ||
| protectedfunctioninstantiateObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes) | ||
| protectedfunctioninstantiateObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes,$format =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry I realized that this is a BC break... It should be removed. But it's annoying because some normalizers may depend of the format (normalizers of API Platform for instance).
An alternative approach:
- Rename this method
instantiateComplexObject(with the new parameter, it can be movec before$contextfor consistency) - Add back
instantiateObjectwith the current signature and callinstantiateComplexObjectinside it with format asnull - Deprecate
instantiateObject
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
will do
theofidry commentedJul 3, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hm, looks like this also occurs for setters but this is a bigger problem: the serializer leave the set part to the property accessor, so it doesn't have access to the mutator and can't guess the type. |
| * @throws RuntimeException | ||
| */ | ||
| protectedfunctioninstantiateObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes) | ||
| protectedfunctioninstantiateComplexObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes,$format =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It can now be:
instantiateComplexObject(array &$data,$class,$format, array &$context, \ReflectionClass$reflectionClass,$allowedAttributes)
To mimic theSerializerInterface::deserialize prototype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
my bad I kept the old signature, will change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do you think about usinginstantiateClass to have something shorter but still clear ?
theofidry commentedJul 3, 2016
@dunglas not sure where the problem lies right now, will have to dig a bit into it later although I hope there won't be any big change. |
theofidry commentedJul 4, 2016
@dunglas tests are passing |
| } | ||
| /** | ||
| * @see instantiateComplexObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing@trigger_error() call for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@xabbuh done
| * @deprecated Since 3.1, will be removed in 4.0. Use instantiateComplexObject instead. | ||
| */ | ||
| protectedfunctioninstantiateObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
since version 3.2 and@trigger_error(sprintf('...'), E_USER_DEPRECATED);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm quite surprised this went undetected by the phpunit bridge...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@theofidry phpunit bridge only discovers deprecated errors, not general silenced errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@wouterj but I have a test with the@legacy group, so I guess I expected it to warn me or throw me an error if no deprecation notice were discovered
theofidry commentedJul 6, 2016
Updated |
| if ($constructor->isConstructor()) { | ||
| return$reflectionClass->newInstanceArgs($params); | ||
| }else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should not be updated imo as this is less clear...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The more indent you have the harder it is to read. And if the choice is to useif/else/elseif blocks, then everything should be in an else instead of the return done L311.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is only one line here and i agree that most of the timeelse +return is only redundant but here it highlights the fact that one of this line will be executed and that the second one is not just a default.
Edit: BTW, we could reverse the conditionif ($constructor) to have less lines indented as you said :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Edit: BTW, we could reverse the condition if ($constructor) to have less lines indented as you said :)
I like this one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But not sure this would be accepted as this is a small detail and could create many merge conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah actually I think I'll revert everything for that part, it's just CS and although I think it makes a big difference it terms of readability, I fear that it will cause lots of conflicts later on. One solution would be do go back in earlier versions and fix that version after version, but it's rather tedious and slow.
| $reflectionClass =new \ReflectionClass($class); | ||
| $object =$this->instantiateObject($normalizedData,$class,$context,$reflectionClass,$allowedAttributes); | ||
| $object =$this->instantiateComplexObject($normalizedData,$class,$context,$reflectionClass,$allowedAttributes,$format); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unfortunatelly this looks like a bc break as people may depend on the call toinstantiateObject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure to see what you mean, before it was simply failing so people could not used it. The method has been changed because there's a BC break in the method signature (we are adding$format), but the behaviour is the same as the previous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In case someone extendsinstantiateObject it won't be called anymore. I'll provide you an example of how this could be managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You could do something like:
protectedfunctioninstantiateObject(...,$allowedAttributes/**, $format*/){$format =null;if (func_num_args() >=6) {$format =func_get_arg(5); }// ...}protectedfunctioninstantiateComplexObject(...){static$deprecationTriggered =false;if (false ===$deprecationTriggered) {$deprecationTriggered =true;$class =get_class($this);if (0 !==strpos($class,'Symfony\\') &&self::class !== (new \ReflectionMethod($this,'instantiateObject'))->getDeclaringClass()->getName()) { @trigger_error(...,E_USER_DEPRECATED); } }return$this->instantiateObject(...);}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
taking$format as the 6th argument ininstantiateObject would still be a problem if you extended it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is forbidden by the PHP interpreter itself:https://3v4l.org/S48gf
Si there is no problem here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
When you override a protected method, you must respect its contract... It's not a BC break for me.
If you override it this way there is not BC as the added param is optional.
In all honesty I would be happy to say yes to this solution it's the easiest way to deal with it really. The only thing I'm worried about is that it may introduce a BC break (but that's up to you to decide if it is or not) and the fact that the user may not see the change (i.e. see that he has to inject the format now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Anyway, it's not a BC break because the parent function is still called with the good number of arguments. Seehttps://3v4l.org/XpYGK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nvm, even with my example it wouldn't be a problem as you would call the parent function with the right number of parameters. Not doing so is looking for trouble and would be the developer fault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This solution is often used in symfony so i'd say it's not part of the symfony bc promises.
dunglas commentedJul 10, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Status: Reviewed 👍 when tests for PHP <7 will be fixed |
dunglas commentedJul 11, 2016
Thank you@theofidry. |
This PR was merged into the 3.2-dev branch.Discussion----------[Serializer] Argument objects| Q | A| ------------- | ---| Branch? | 3.1| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | TODO| Fixed tickets | none| License | MIT| Doc PR | TODOAssuming with have the two following entities:```phpnamespace AppBundle\Entity;class Dummy{ public function __construct(int $id, string $name, string $email, AnotherDummy $anotherDummy) { $this->id = $id; $this->name = $name; $this->email = $email; $this->anotherDummy = $anotherDummy; }}class AnotherDummy{ public function __construct(int $id, string $uuid, bool $isEnabled) { $this->id = $id; $this->uuid = $uuid; $this->isEnabled = $isEnabled; }}```Doing the following will fail:```php$serializer->denormalize( [ 'id' => $i, 'name' => 'dummy', 'email' => 'du@ex.com', 'another_dummy' => [ 'id' => 1000 + $i, 'uuid' => 'azerty', 'is_enabled' => true, ], ], \AppBundle\Entity\Dummy::class);```with a type error, because the 4th argument passed to `Dummy::__construct()` will be an array. The following patch checks if the type of the argument is an object, and if it is tries to denormalize that object as well.I'm not sure if it's me missing something or this is a use case that has been omitted (willingly or not), but if it's a valuable patch I would be happy to work on finishing it.Commits-------988eba1 fix tests98bcb91 Merge pull request#1 from dunglas/theofidry-feature/param-object7b5d55d Prevent BC in instantiateObjecte437e04 fix reflection type3fe9802 revert CS5556fa5 fixd4cdb00 fix CS93608dc Add deprecation messagef46a176 Apply patchf361e52 fix tests4884a2e f1e64e999 Address commentse99a90b Add tests7bd4ac5 Testtheofidry commentedJul 11, 2016
You're welcome ;) |
This PR was merged into the 3.2 branch.Discussion----------[Serializer] Fix argument object denormalization| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20670| License | MIT| Doc PR | N/AFixes#20670. I've seen#19277 (diff) so I think it's the right thing to do, but I didn't follow the thread at the time, so I may have missed something.Ping@theofidry,@dunglas.Commits-------27de65a [Serializer] Fix argument object denormalization
Uh oh!
There was an error while loading.Please reload this page.
Assuming with have the two following entities:
Doing the following will fail:
with a type error, because the 4th argument passed to
Dummy::__construct()will be an array. The following patch checks if the type of the argument is an object, and if it is tries to denormalize that object as well.I'm not sure if it's me missing something or this is a use case that has been omitted (willingly or not), but if it's a valuable patch I would be happy to work on finishing it.