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

[2.0] [Feature-request] Provide InitializedObjectConstructor as default #775

Closed
scaytrase opened this issue May 22, 2017 · 9 comments
Closed

Comments

@scaytrase
Copy link
Contributor

It is really hard to understand that you cannot to deserialize into existing object without additional configuration or using doctrine

Also this is weird that such useful thing lives within fixtures.

If it is applicable for 1.x branch (since it is somehow backward compatible), it would be nice

@goetas
Copy link
Collaborator

goetas commented May 23, 2017

InitializedObjectConstructor is just a specific usecase of the of the ObjectConstructorInterface.

InitializedObjectConstructor works only on the first element of the deserialization object graph, and in this case is more convenient just for test proposes.

Since InitializedObjectConstructor works only for a specific usecase, and has a lot fo drawbacks, I do not plan to integrate it into the core. If your usecase is similar to schmittjoh/JMSSerializerBundle#575, i suggest you to have a look at the symfony/form component and the approach suggested in schmittjoh/JMSSerializerBundle#575 (comment)

@goetas goetas closed this as completed May 23, 2017
@scaytrase
Copy link
Contributor Author

@goetas the problem of using forms is that you have to write form classes instead of describing the serialized presentation of entities. It's like suggestion to manually map DB data to entities but not using the Doctrine2 hydration metadata

@goetas
Copy link
Collaborator

goetas commented May 23, 2017

I know than it is not an optimal solution... at the moment the serializer is not really powerful to update objects (as example, can not easily append items to a collection or deal with different data-formats where normalization is needed).

On the other hand, symfony form is really good at it, but does not offer serialization, that is why I like to combine the two of them.

The provided object constructors are domain independent...

  • UnserializeObjectConstructor creates an object instance for any class, using PHP reflection
  • DoctrineObjectConstructor creates/retrieves an object instance from the doctrine registry of managed entities

InitializedObjectConstructor is really domain-specific and hacky (it works only for the first object of the deserialization process...). Requires you to pass the instance via context, making your code coupled to the construction logic.
If you are really interested in working on a similar solution, something more interesting can be a registry of constructed objects.... as:

class InstantiatedObjectsObjectConstructor implements ObjectConstructorInterface
{

    private $registry = array();
    private $fallbackConstructor;

    public function __construct(ObjectConstructorInterface $fallbackConstructor)
    {
        $this->fallbackConstructor = $fallbackConstructor;
    }

    public function add($id, $object)
    {
        $this->registry[$id] = $object;
    }

    public function construct(VisitorInterface $visitor, ClassMetadata $metadata, $data, array $type, DeserializationContext $context)
    {
     $id = $data['id']; // or something better

      if (isset($this->registry[$id])) {
          return $this->registry[$id];
      } else {
          return  $this->fallbackConstructor->construct(...);
      }
    }

}

This is not a great solution, but is more flexible and can work for a more deep object graph.

@scaytrase
Copy link
Contributor Author

UnserializeObjectConstructor creates an object instance for any class, using PHP reflection

This is the problem since there can be a domain logic and defaults in the constructor, so this forbids any domain logic here. Passing the instance via context seems legit too me, do not see anything weird here (excluding excplicit named method would be nice). Doctrine has sort of "Hints" for this which looks much more weird than named context features.

Just an idea - can we define a constructor type not at global level, but on the metadata one? I.e I see nothing bad in instantiating Value-Objects with current unserializing constructor, but higher-class business logic object can be passed as is.

offtop question: do we involve any kind of naming strategy here beforehand?
https://github.com/schmittjoh/serializer/blob/master/src/Construction/DoctrineObjectConstructor.php#L94

@goetas
Copy link
Collaborator

goetas commented May 23, 2017

offtop question: do we involve any kind of naming strategy here beforehand?
https://github.com/schmittjoh/serializer/blob/master/src/Construction/DoctrineObjectConstructor.php#L94

actually there should be a bug (not verified) about it, see #734

Just an idea - can we define a constructor type not at global level, but on the metadata one? I.e I see nothing bad in instantiating Value-Objects with current unserializing constructor, but higher-class business logic object can be passed as is.

this sounds a good idea... probably something as

/** 
 * @Constructor("unserialize|doctrine|any_other_name")
 */ 
protected $user;

@scaytrase
Copy link
Contributor Author

Can we reopen and leave this as RFC idea before 2.0 (or later release)?

@goetas
Copy link
Collaborator

goetas commented May 23, 2017

Crating a new RFC for 1.x branch sounds good (and keeping this closed). The suggested approach (@Constructor annotation) looks to be backward compatible.

@kinow
Copy link
Contributor

kinow commented Sep 5, 2018

I'm using InitializedObjectConstructor in a project with nested objects, and apparently it's working fine. Any idea if this could be working now after some change since the last comment in this ticket @goetas ? I was going to open another ticket to ask for this class to be promoted from the test to src folder, but found this ticket while looking for similar/existent tickets.

Thanks

@goetas
Copy link
Collaborator

goetas commented Sep 10, 2018

as said in #775 (comment)

InitializedObjectConstructor is really domain-specific and hacky (it works only for the first object of the deserialization process...). Requires you to pass the instance via context, making your code coupled to the construction logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants