Unable to use `#[Version]` with custom type

[alamirault]

Bug Report

Q	A
BC Break	no
Version	2.10.2 (and latest)

Summary

I try to use #[Version] to have optimistic locking, however it does not work with custom type.

Current behavior

My new type supports Ramsey/Uuid and it's register like this in the bundle

doctrine_mongodb:
    types:
        Ramsey\Uuid\Uuid: App\Doctrine\ODM\Types\UuidType

When I try to update the entity, I have the following error

Doctrine\ODM\MongoDB\LockException: A lock failed on a document.
vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/LockException.php:24
vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php:410
vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/UnitOfWork.php:1239
vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/UnitOfWork.php:3125
vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/UnitOfWork.php:472
vendor/doctrine/mongodb-odm/lib/Doctrine/ODM/MongoDB/DocumentManager.php:585
tests/Integration/Document/FooTest.php:30

How to reproduce

I created a reproducer: https://github.com/alamirault/reproducer-version-custom-type

• composer install
• update mongodb_url and mongodb_database in config/services.yaml
• php bin/phpunit

Expected behavior

#[Version] attribute must work with custom types

Proposal

I dig a bit and saw we call Type::convertPHPToDatabaseValue($value) with current version (Uuid instance) and next version (Uuid instance) which call Type::getTypeFromPHPVariable($variable).

mongodb-odm/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

Lines 376 to 384 in aba7b30

	if ($this->class->isVersioned) {
	$versionMapping = $this->class->fieldMappings[$this->class->versionField];
	$currentVersion = $this->class->reflFields[$this->class->versionField]->getValue($document);
	$type = Type::getType($versionMapping['type']);
	assert($type instanceof Versionable);
	$nextVersion = $type->getNextVersion($currentVersion);
	$update['$set'][$versionMapping['name']] = Type::convertPHPToDatabaseValue($nextVersion);
	$query[$versionMapping['name']] = Type::convertPHPToDatabaseValue($currentVersion);
	}

This static method getTypeFromPHPVariable doesn't try to find type because variable is an object.

mongodb-odm/lib/Doctrine/ODM/MongoDB/Types/Type.php

Lines 157 to 183 in aba7b30

	/**
	* Get a Type instance based on the type of the passed php variable.
	*
	* @param mixed $variable
	*
	* @throws InvalidArgumentException
	*/
	public static function getTypeFromPHPVariable($variable): ?Type
	{
	if (is_object($variable)) {
	if ($variable instanceof DateTimeInterface) {
	return self::getType('date');
	}
	if ($variable instanceof ObjectId) {
	return self::getType('id');
	}
	} else {
	$type = gettype($variable);
	switch ($type) {
	case 'integer':
	return self::getType('int');
	}
	}
	return null;
	}

Possible solutions:

Update `Type::getTypeFromPHPVariable` to try `self::getType($variable::class)` when is an object

Something like this

    public static function getTypeFromPHPVariable($variable): ?Type
    {
        if (is_object($variable)) {
            if ($variable instanceof DateTimeInterface) {
                return self::getType('date');
            }

            if ($variable instanceof ObjectId) {
                return self::getType('id');
            }

           // Try to found type from object variable as described in phpdoc "Get a Type instance based on the type of the passed php variable."
            try {
                return self::getType($variable::class);
            } catch (InvalidArgumentException $exception) {
                // no-op
            }
        } else {
            $type = gettype($variable);
            switch ($type) {
                case 'integer':
                    return self::getType('int');
            }
        }

        return null;
    }

Update DocumentPersister

At the point we already know version field type. So instead guess type from $currentVersion and $nextVersion, we can convert with retrieved type.
It's maybe be a BC Break if $type->getNextVersion doesn't return a value supported by itself

        if ($this->class->isVersioned) {
            $versionMapping = $this->class->fieldMappings[$this->class->versionField];
            $currentVersion = $this->class->reflFields[$this->class->versionField]->getValue($document);
            $type           = Type::getType($versionMapping['type']);
            assert($type instanceof Versionable);
            $nextVersion                             = $type->getNextVersion($currentVersion);
            
            // Actual value is:
            // $update['$set'][$versionMapping['name']] = Type::convertPHPToDatabaseValue($nextVersion);
            // $query[$versionMapping['name']]          = Type::convertPHPToDatabaseValue($currentVersion);

            // Proposal
            $update['$set'][$versionMapping['name']] = $type->convertToDatabaseValue($nextVersion);
            $query[$versionMapping['name']]          = $type->convertToDatabaseValue($currentVersion);
        }

[GromNaN]

Thank you for this detailed issue and diagnostic. I opened 2 PR that implement your suggestions:

• Convert the value of the "version" field using its known type #2813 (bug fix)
• Improve detection of the type from a PHP variable #2814 (new feature)

But I don't quite understand the idea of using a UUID for the version. I think an incremental int is quite sufficient. Perhaps a random and sortable, ObjectId would best fit the need.

• Enable optimistic locking using an ObjectId as version field #2815

[alamirault]

Thank you @GromNaN !

Use ObjectId looks good indeed