This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Probably out of scope and irrelevant but ... we recently added the VarExporter component which already includes an Instantiator. Are we duplicating efforts here? Thanks.
@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.
throw new MissingConstructorArgumentsException(sprintf('Cannot create an instance of %s from serialized data because its constructor requires parameter "%s" to be present.', $class, $constructorParameter->name));
The reason will be displayed to describe this comment to others. Learn more.
In case of missing constructor arguments, you probably should fall back on using the property access component to arbitrarily set properties onto the object. Problem is that your code change with time, but serialized entities stored somewhere don't.
The reason will be displayed to describe this comment to others. Learn more.
Just forget the previous comment, sorry for the noise. But it raise an interesting question: the instanciator sets some values using the constructor, then the API using it (for example, the normalizer because it's today the only API using it I know of) will set some others, it seems like each component responsibility is unclear, they both walk on each other foot.
The reason will be displayed to describe this comment to others. Learn more.
How will the normalizer now which properties have already been set or not if the instanciator becomes an opaque interface ? It seems that it will then be forced to re-inject all properties ? Maybe not since you pass an array reference, you can remove constructor-set values from the array, but it's a very weird interface then, you have two different outputs: the object, and the modified array. It's not cognitively-easy. I know do understand your answer below thought.
Actually we don't have a simpler way to pass both object and new data array, so we use the reference to handle the data. It's probably not the best way to do it but it can be improved.
I think we should more focus on code duplication in Serializer before focusing on attended behavior even if they do use references.
The reason will be displayed to describe this comment to others. Learn more.
Yes I finally understood why you're doing it this way now. This is weird because the instanciator and the normalizer then share some bits of internal logic then, whereas having an external interface main benefit is "divide and conquer", making every component simpler and avoid cognitive surcharge when reading bare algorithm (also make it simpler to maintain). I agree this is not an easy one, thanks for taking the time to answer.
We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.