[Loggable] ORM entity relies on deprecated field type

[mbabker]

DBAL 3.4 deprecated the array and object types. The Gedmo\Loggable\Entity\MappedSuperclass\AbstractLogEntry schema uses the array type for its data column. This is going to need migrating somehow.

(While the actual change isn't too bad to make here, it requires a data migration for all users of this package from a serialized string to its replacement (JSON column is the suggestion), which isn't a straightforward thing depending on the types of changes being logged)

[chapterjason]

I already tried to overwrite the LogEntry and set the type to Types::JSON, which results in the following issue:

This is the content of the data field in a LogEntry.

Array
(
    [content] => <div>test1</div>
    [publishedAt] => 
    [reviewedAt] => 
    [slug] => 
    [state] => draft
    [title] => test1
    [updatedAt] => Array
        (
            [date] => 2022-08-09 20:45:59.585784
            [timezone_type] => 3
            [timezone] => UTC
        )

    [publishedBy] => 
    [reviewedBy] => 
    [updatedBy] => Array
        (
            [uid] => ...
        )

)

Using revert results in the following error:

Cannot assign array to property App\Entity\Post::$updatedAt of type ?DateTimeImmutable

This means the LogEntryRepository::mapValue requires some rework. But I don't know how to dynamically convert all possible fields of the dbal type registry without a lot of custom implementation.

[mbabker]

I knew the data migration was going to be tricky in the first place because of how the log entries get written. They actually can also have objects if you're logging those types of fields (we have odolbeau/phone-number-bundle installed and log a couple of phone number fields, those are logged as a serialized libphonenumber\PhoneNumber instance).

So this almost turns into a bigger "re-evaluate how the logging extension operates" item because not only do you have to think through the data serialization, you have to handle data in all sorts of different states (DateTimeInterface objects look to be written as an array, serialized objects, and scalar values) when migrating between types, and whatever format the entry's written in has to be something that can be reversed back onto an entity for the revert workflow (which I honestly didn't realize exists until your comment).

[chapterjason]

Experimented a bit and got it working for my use case, tests are also running. Not sure if I miss any cases. (except for data migration)

chapterjason@d114aa4

From d114aa4f138014b032b4a9474a7fd49d88b012bf Mon Sep 17 00:00:00 2001
From: chapterjason <[email protected]>
Date: Wed, 10 Aug 2022 00:25:14 +0200
Subject: [PATCH] enhance: Switch to json data field type

---
 .../MappedSuperclass/AbstractLogEntry.php     |  4 +--
 .../Entity/Repository/LogEntryRepository.php  |  6 ++++
 src/Loggable/LoggableListener.php             | 31 ++++++++++++-------
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/src/Loggable/Entity/MappedSuperclass/AbstractLogEntry.php b/src/Loggable/Entity/MappedSuperclass/AbstractLogEntry.php
index dba288d61..1ff767160 100644
--- a/src/Loggable/Entity/MappedSuperclass/AbstractLogEntry.php
+++ b/src/Loggable/Entity/MappedSuperclass/AbstractLogEntry.php
@@ -73,9 +73,9 @@ abstract class AbstractLogEntry
     /**
      * @var array|null
      *
-     * @ORM\Column(type="array", nullable=true)
+     * @ORM\Column(type="json", nullable=true)
      */
-    #[ORM\Column(type: Types::ARRAY, nullable: true)]
+    #[ORM\Column(type: Types::JSON, nullable: true)]
     protected $data;
 
     /**
diff --git a/src/Loggable/Entity/Repository/LogEntryRepository.php b/src/Loggable/Entity/Repository/LogEntryRepository.php
index b00475fc0..46e51577a 100644
--- a/src/Loggable/Entity/Repository/LogEntryRepository.php
+++ b/src/Loggable/Entity/Repository/LogEntryRepository.php
@@ -9,6 +9,7 @@
 
 namespace Gedmo\Loggable\Entity\Repository;
 
+use Doctrine\DBAL\Types\Type;
 use Doctrine\ORM\EntityRepository;
 use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Query;
@@ -132,6 +133,11 @@ public function revert($entity, $version = 1)
     protected function mapValue(ClassMetadata $objectMeta, $field, &$value)
     {
         if (!$objectMeta->isSingleValuedAssociation($field)) {
+            $fieldType = $objectMeta->getTypeOfField($field);
+            $type = Type::getType($fieldType);
+
+            $value = $type->convertToPHPValue($value, $this->_em->getConnection()->getDatabasePlatform());
+
             return;
         }
 
diff --git a/src/Loggable/LoggableListener.php b/src/Loggable/LoggableListener.php
index 3bc717685..c4278c986 100644
--- a/src/Loggable/LoggableListener.php
+++ b/src/Loggable/LoggableListener.php
@@ -10,6 +10,7 @@
 namespace Gedmo\Loggable;
 
 use Doctrine\Common\EventArgs;
+use Doctrine\DBAL\Types\Type;
 use Doctrine\Persistence\Event\LoadClassMetadataEventArgs;
 use Doctrine\Persistence\ObjectManager;
 use Gedmo\Loggable\Mapping\Event\LoggableAdapter;
@@ -249,20 +250,26 @@ protected function getObjectChangeSetData($ea, $object, $logEntry)
                 continue;
             }
             $value = $changes[1];
-            if ($meta->isSingleValuedAssociation($field) && $value) {
-                if ($wrapped->isEmbeddedAssociation($field)) {
-                    $value = $this->getObjectChangeSetData($ea, $value, $logEntry);
-                } else {
-                    $oid = spl_object_id($value);
-                    $wrappedAssoc = AbstractWrapper::wrap($value, $om);
-                    $value = $wrappedAssoc->getIdentifier(false);
-                    if (!is_array($value) && !$value) {
-                        $this->pendingRelatedObjects[$oid][] = [
-                            'log' => $logEntry,
-                            'field' => $field,
-                        ];
+            if ($meta->isSingleValuedAssociation($field)) {
+                if ($value) {
+                    if ($wrapped->isEmbeddedAssociation($field)) {
+                        $value = $this->getObjectChangeSetData($ea, $value, $logEntry);
+                    } else {
+                        $oid = spl_object_id($value);
+                        $wrappedAssoc = AbstractWrapper::wrap($value, $om);
+                        $value = $wrappedAssoc->getIdentifier(false);
+                        if (!is_array($value) && !$value) {
+                            $this->pendingRelatedObjects[$oid][] = [
+                                'log' => $logEntry,
+                                'field' => $field,
+                            ];
+                        }
                     }
                 }
+            } else {
+                $typeName = $meta->getTypeOfField($field);
+                $type = Type::getType($typeName);
+                $value = $type->convertToDatabaseValue($value, $om->getConnection()->getDatabasePlatform());
             }
             $newValues[$field] = $value;
         }

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mbabker]

#2825 should be a fully complete-ish solution for this one, it's just going to need a lot of eyes (especially from the MongoDB ODM users since I'm relying heavily on the existing tests to make sure it works).