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] Instantiator - Add an interface and default implementation to instantiate objects #30956

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@joelwurtz
Copy link
Contributor

joelwurtz commented Apr 7, 2019

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

Related to #30818, Replace #30925

This add a new interface and default implementation to instantiate objects

OBJECT_TO_POPULATE was not keep here, as i don't think it should be the responsability of an instantiator to handle that. And if we want to have this responsability we can always add a new implementation with a decoration system.

I try to look at var exporter instantiator also and unfortunetaly @nicolas-grekas i cannot use this, since the behavior of this component is to not use the constructor, in the serializer we want to use it, or at least it used to do that and we cannot change this behavior.

But we can use var exporter implementation in a future PR (will not be the default however just another way of doing it) here if someone does not want to call the constructor.

WIP

@joelwurtz joelwurtz force-pushed the joelwurtz:feature/instantiator branch from cfc934e to 74eb51f Apr 7, 2019

@joelwurtz joelwurtz force-pushed the joelwurtz:feature/instantiator branch from d115b49 to c6168b5 Apr 7, 2019

$parameterData = $data[$key];
if (null === $parameterData && $constructorParameter->allowsNull()) {
$params[] = null;
// Don't run set for a parameter passed to the constructor

This comment has been minimized.

Copy link
@stof

stof Apr 7, 2019

Member

this won't work as you never return the modified $data outside the class with this extracted implementation.

This comment has been minimized.

Copy link
@joelwurtz

joelwurtz Apr 7, 2019

Author Contributor

You are right, i remove this comment and the unset part ATM, i have 2 ideas to fix this, but it should not be done here:

  • We could pass $data as reference (not a fan)
  • We could add a filter on properties list that remove properties set with constructor (would be the best)
@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Apr 8, 2019

Probably out of scope and irrelevant but ... we recently added the VarExporter component which already includes an Instantiator. Are we duplicating efforts here? Thanks.

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

joelwurtz commented Apr 8, 2019

@javiereguiluz Like i said on top, i was aware of that, but unfortunetaly we cannot use it as a default implementation since the var export implementation try to skip the constructor where we want to use it here, as it's the current behaviour of existing normalizers.

Like If someone was using constructor to set default values for its class, using the var exporter would break its behaviour.

One thing that would be possible however is to create a Denormalizer that use the Var Exporter Component, but it's out of scope for this 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.