DoctrineObjectConstructor calls Doctrine with null identifier columns

[Vincentv92]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	yes
RFC?	yes

Steps required to reproduce the problem

1. Managed entity by doctrine with a single @ORM\Id notation
2. From XML deserialisation to managed doctrine object
3. XML contains an ID field which does not exists yet.

composer show | grep jms
jms/metadata                         2.5.0     Class/method/property metadata management in PHP
jms/serializer                       3.12.0    Library for (de-)serializing data of any complexity; supports XML, JSON, and YAML.
jms/serializer-bundle                3.8.0     Allows you to easily serialize, and deserialize data of any complexity
composer show | grep doctrine
doctrine/annotations                 1.12.1    Docblock Annotations Parser
doctrine/cache                       1.10.2    PHP Doctrine Cache library is a popular cache implementation that supports many different drivers such as redis, memcache, apc, mongodb and others.
doctrine/collections                 1.6.7     PHP Doctrine Collections library that adds additional functionality on top of PHP arrays.
doctrine/common                      3.1.1     PHP Doctrine Common project is a library that provides additional functionality that other Doctrine projects depend on such as better reflection support, proxies and much more.
doctrine/dbal                        2.10.4    Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection and management.
doctrine/doctrine-bundle             2.2.3     Symfony DoctrineBundle
doctrine/event-manager               1.1.1     The Doctrine Event Manager is a simple PHP event system that was built to be used with the various Doctrine projects.
doctrine/inflector                   2.0.3     PHP Doctrine Inflector is a small library that can perform string manipulations with regard to upper/lowercase and singular/plural forms of words.
doctrine/instantiator                1.4.0     A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                       1.2.1     PHP Doctrine Lexer parser library that can be used in Top-Down, Recursive Descent Parsers.
doctrine/orm                         2.8.2     Object-Relational-Mapper for PHP
doctrine/persistence                 2.1.0     The Doctrine Persistence project is a set of shared interfaces and functionality that the different Doctrine object mappers share.
doctrine/sql-formatter               1.1.1     a PHP SQL highlighting library
symfony/doctrine-bridge              v4.4.20   Provides integration for Doctrine with various Symfony components

Expected Result

• null object or the fallback strategy come into play, in my case an unmanaged Doctrine entity.

This seems related on changes in the following PR's:
#1224
#1253

The issue lies in DoctrineObjectConstructor whereby the call to the entity manager is malformed:
$entityManager->find("App\Entity\Order", [ 'ordernumber' => null ], $lockMode, $lockVersion)
Whereby ordernumber cannot be null, since this is the identifying column as defined by Doctrine schema

serializer/src/Construction/DoctrineObjectConstructor.php

Line 104 in 92698b2

if (is_array($data) && !array_key_exists($propertyMetadata->serializedName, $data)) {

I believe there is elseif missing, because we are deserializing from an SimpleXMLElement the property exists, but cannot be accessed by $data[$propertyMetadata->serializedName] which will return in a null value, which causes the incorrect call to Doctrine find.
So this can be changed to (string) $data->{$data[$propertyMetadata->serializedName]}; if it's an SimpleXMLElement but thats not really nice looking.
When adding this to the elseif on line 108:

            } elseif (get_class($data) === 'SimpleXMLElement') {
                return $this->fallbackConstructor->construct($visitor, $metadata, $data, $type, $context);
            }

It works as expected, but I can't figure out if this causes more issues or should be approached different.

Actual Result

• Doctrine Exception

Doctrine\ORM\ORMException^ {#410
  #message: "The identifier ordernumber is missing for a query of App\Entity\Order"
  #code: 0
  #file: "./vendor/doctrine/orm/lib/Doctrine/ORM/ORMException.php"
  #line: 310
  trace: {
    ./vendor/doctrine/orm/lib/Doctrine/ORM/ORMException.php:310 { …}
    ./vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php:431 { …}
    ./var/cache/dev/ContainerQaPHb7b/srcApp_KernelDevDebugContainer.php:4359 {
      ContainerQaPHb7b\EntityManager_9a5be93->find($className, $id, $lockMode = null, $lockVersion = null)^
      ›
      ›     return $this->valueHolder4c4f6->find($className, $id, $lockMode, $lockVersion);
      › }
      arguments: {
        $className: "App\Entity\Order"
        $id: array: [ 'ordernumber' => null ]
        $lockMode: null
        $lockVersion: null
      }
    }
    ./vendor/jms/serializer/src/Construction/DoctrineObjectConstructor.php:120 { …}
    ./vendor/jms/serializer/src/GraphNavigator/DeserializationGraphNavigator.php:190 { …}
    ./vendor/jms/serializer/src/XmlDeserializationVisitor.php:224 { …}
    ./vendor/jms/serializer/src/GraphNavigator/DeserializationGraphNavigator.php:141 { …}
    ./vendor/jms/serializer/src/Serializer.php:252 { …}
    ./vendor/jms/serializer/src/Serializer.php:180 { …}
    ./src/test/Command/ReadXMLCommand.php:61 { …}
    ./vendor/symfony/console/Command/Command.php:255 { …}
    ./vendor/symfony/console/Application.php:1027 { …}
    ./vendor/symfony/framework-bundle/Console/Application.php:97 { …}
    ./vendor/symfony/console/Application.php:273 { …}
    ./vendor/symfony/framework-bundle/Console/Application.php:83 { …}
    ./vendor/symfony/console/Application.php:149 { …}
    ./bin/console:42 { …}```

[Toilal]

I wrote a pull request. I'm not sure it will work properly with multiple ids where a single id is null, but it would require an integration with a real database to check this.

[Vincentv92]

Thanks @Toilal the way you constructed the find call to doctrine won't be malformed so I guess this is up to doctrine if accepts the find arguments or will throw an exception.

[Toilal]

Yes, that's exactly what I mean but I don't know if Doctrine actually accept some undefined values among declared composed ids.

[goetas]

Yes, that's exactly what I mean but I don't know if Doctrine actually accept some undefined values among declared composed ids.

i have the same concern. can anyone try it please?

@VincentLanglet out of curiosity, how does it look your xml or json?

[VincentLanglet]

I think you wanted to tag @Vincentv92, @goetas ?