Repository: pimcore/pimcore (language: php)

Issue:
[Assets] Use JSON to serialize `customSettings`
### Improvement description

- [ ] Serialize data using JSON instead of PHP `serialize()`
- [ ] Use JSON column-type in DB schema for easier querying

Open question: BC-break? 🤔 


Discussion / hints:
Suggestion: use a column to define if the serialize data is new and decide how to unserialize based on that column. Also provide a migration in some way to migrate the existing serialized data.

Also, we could use Symfony serializer. 
@dvesh3 probably we don't even need this extra column, as the PHP serialized value always starts with `a:` - that could be also the indicator how to deal with the data ... but I'm not sure whether MySQL/MariaDB likes it to have other data than JSON in a JSON column 🤔 
> @dvesh3 probably we don't even need this extra column, as the PHP serialized value always starts with `a:` - that could be also the indicator how to deal with the data ... but I'm not sure whether MySQL/MariaDB likes it to have other data than JSON in a JSON column 🤔

Liked the idea, but unfortunately the JSON column type (as `customSettings_2`) doesn't allow to add anything than valid JSON
![image](https://github.com/pimcore/pimcore/assets/6014195/5b84fd0b-91ef-4007-92dd-815831473581)

It doesn't look BC breaky, supposingly dev shouldn't directly access the table column data and do string manipulation there, and that [getter/setters](https://github.com/pimcore/pimcore/blob/e04935c09918a762572e6e17e0e8461c7b37ed33/models/Asset.php#L1260-L1316) are just conveniently provided and [documented](https://pimcore.com/docs/platform/Pimcore/Assets/Working_with_PHP_API/#custom-settings).

Should we create a new column and
- add bc layers to prioritize the read/write of this new column and falling back to former one when not set any value
- or already migrate every existing value to json in one go?
> > [@dvesh3](https://github.com/dvesh3) probably we don't even need this extra column, as the PHP serialized value always starts with `a:` - that could be also the indicator how to deal with the data ... but I'm not sure whether MySQL/MariaDB likes it to have other data than JSON in a JSON column 🤔
> 
> Liked the idea, but unfortunately the JSON column type (as `customSettings_2`) doesn't allow to add anything than valid JSON ![image](https://private-user-images.githubusercontent.com/6014195/331138062-5b84fd0b-91ef-4007-92dd-815831473581.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTU4NTg2MTcsIm5iZiI6MTcxNTg1ODMxNywicGF0aCI6Ii82MDE0MTk1LzMzMTEzODA2Mi01Yjg0ZmQwYi05MWVmLTQwMDctOTJkZC04MTU4MzE0NzM1ODEucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDUxNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA1MTZUMTExODM3WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ODk0ZjE1OGNjYjExMDIzYTc3Njg3MDRmOTNmNTBmZGEwNzUzNjM0YTJhNmQ1MTU1NzZiZGNiNmFmOWU0YzFjMyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.R4Yaa5QfqBGEoeRfAlh9AxZE5fn5ivN6BrNhZ8PGthU)
> 
> It doesn't look BC breaky, supposingly dev shouldn't directly access the table column data and do string manipulation there, and that [getter/setters](https://github.com/pimcore/pimcore/blob/e04935c09918a762572e6e17e0e8461c7b37ed33/models/Asset.php#L1260-L1316) are just conveniently provided and [documented](https://pimcore.com/docs/platform/Pimcore/Assets/Working_with_PHP_API/#custom-settings).
> 
> Should we create a new column and
> 
> * add bc layers to prioritize the read/write of this new column and falling back to former one when not set any value
> * or already migrate every existing value to json in one go?

I would vote for migrating everything to JSON in one go as the two column approach would also be some kind of BC break.

So in my opinion either do it in one go now or do it in one go in the next major version.

@brusch what's your opinion on that?
@markus-moser let's do it in one go in the next major version. 
Should check all places where PHP serialize is used and swap them all out in one go, if this isn't the only one.

Fix patch:
diff --git a/bundles/CoreBundle/config/serializer.yaml b/bundles/CoreBundle/config/serializer.yaml
index b2ea7624785..79ded95a043 100644
--- a/bundles/CoreBundle/config/serializer.yaml
+++ b/bundles/CoreBundle/config/serializer.yaml
@@ -16,9 +16,11 @@ services:
 
     Pimcore\Serializer\Serializer:
         public: true
-        arguments: ['@?', '@?']
+        arguments: [ '@?', '@?' ]
 
-    pimcore.serializer: '@Pimcore\Serializer\Serializer'
+    pimcore.serializer:
+        alias: Pimcore\Serializer\Serializer
+        public: true
 
     pimcore.serializer.encoder.json:
         class: Symfony\Component\Serializer\Encoder\JsonEncoder
diff --git a/bundles/CoreBundle/src/Migrations/Version20250312132759.php b/bundles/CoreBundle/src/Migrations/Version20250312132759.php
new file mode 100644
index 00000000000..9f43fefd025
--- /dev/null
+++ b/bundles/CoreBundle/src/Migrations/Version20250312132759.php
@@ -0,0 +1,136 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * Pimcore
+ *
+ * This source file is available under two different licenses:
+ * - GNU General Public License version 3 (GPLv3)
+ * - Pimcore Commercial License (PCL)
+ * Full copyright and license information is available in
+ * LICENSE.md which is distributed with this source code.
+ *
+ *  @copyright  Copyright (c) Pimcore GmbH (http://www.pimcore.org)
+ *  @license    http://www.pimcore.org/license     GPLv3 and PCL
+ */
+
+namespace Pimcore\Bundle\CoreBundle\Migrations;
+
+use Doctrine\DBAL\Exception;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+final class Version20250312132759 extends AbstractMigration
+{
+    private const string ASSET_TABLE = 'assets';
+    private const string SETTINGS_COLUMN = 'customSettings';
+    private const string ID_COLUMN = 'id';
+
+    public function getDescription(): string
+    {
+        return 'Migrate customSettings asset column from php serialized to json';
+    }
+
+    /**
+     *
+     * When migrating from serialized to json, we need to convert the data in the columns first.
+     * Afterward, we need to change the column type.
+     *
+     * @throws \JsonException
+     * @throws Exception
+     */
+    public function up(Schema $schema): void
+    {
+       $this->migrateAssets();
+       $this->alterColumn();
+    }
+
+    /**
+     *
+     *  When migrating from json to serialized, we need to change the column type first,
+     *  to get rid of the json_valid check.
+     *  Afterward, we need to convert the data in the columns.
+     *
+     * @throws \JsonException
+     * @throws Exception
+     */
+    public function down(Schema $schema): void
+    {
+        $this->alterColumn(false);
+        $this->migrateAssets(false);
+    }
+
+    /**
+     * @throws Exception
+     * @throws \JsonException
+     */
+    private function migrateAssets(bool $up = true): void
+    {
+        $assets = $this->connection->fetchAllAssociative(
+            sprintf(
+            'select %s, %s from assets',
+                $this->connection->quoteIdentifier(self::ID_COLUMN),
+                $this->connection->quoteIdentifier(self::SETTINGS_COLUMN)
+            )
+        );
+
+        foreach($assets as $asset) {
+            $this->migrateAsset($asset, $up);
+        }
+    }
+
+    /**
+     * @throws \JsonException
+     */
+    private function migrateAsset(array $assetData, bool $up = true): void
+    {
+        foreach($assetData as $column => $value) {
+            if (
+                !is_string($value) ||
+                empty($value) ||
+                !$this->isTargetColumn($value, $up)
+            ) {
+                continue;
+            }
+
+            $data = $up ?
+                unserialize($value, ['allowed_classes' => false]) :
+                json_decode($value, true, 512, JSON_THROW_ON_ERROR);
+
+            $data = $up ?
+                json_encode($data, JSON_THROW_ON_ERROR) :
+                serialize($data);
+
+
+            $this->addSql(
+                sprintf(
+                    'UPDATE %s SET %s = ? WHERE id = ?',
+                    $this->connection->quoteIdentifier(self::ASSET_TABLE),
+                    $this->connection->quoteIdentifier($column)
+                ),
+                [
+                    $data,
+                    $assetData['id']
+                ]
+            );
+        }
+    }
+
+    private function isTargetColumn(string $value, bool $up): bool
+    {
+        return ($up === true && preg_match('/^a:\d+:\{.*\}$/', $value)) ||
+            ($up === false && preg_match('/^\{.*\}$/', $value));
+    }
+
+    private function alterColumn(bool $up = true): void
+    {
+        $this->addSql(
+            sprintf(
+                'ALTER TABLE %s MODIFY COLUMN %s %s',
+                $this->connection->quoteIdentifier(self::ASSET_TABLE),
+                $this->connection->quoteIdentifier(self::SETTINGS_COLUMN),
+                $up ? 'json' : 'longtext'
+            )
+        );
+    }
+}
diff --git a/doc/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md b/doc/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md
index f5a928c94f1..f375241511a 100644
--- a/doc/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md
+++ b/doc/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md
@@ -21,6 +21,9 @@ Please make sure to add translations for log levels.
 
 - `filter_priority` configuration changed. LogLevels now start at 1 (emergency) - 8 (debug) instead of 0 (emergency) - 7 (debug). Please adjust your configuration accordingly.
 
+#### [Assets]
+- `customSettings` column in `assets` table is now a JSON column. 
+
 #### [Bundle]
 - Removed compatibility layer static `$bundleManager`
 
diff --git a/lib/Tool/Serialize.php b/lib/Tool/Serialize.php
index 81e64cd2ce2..85f65927edb 100644
--- a/lib/Tool/Serialize.php
+++ b/lib/Tool/Serialize.php
@@ -17,6 +17,7 @@
 namespace Pimcore\Tool;
 
 use Pimcore;
+use Pimcore\Serializer\Serializer;
 use Throwable;
 
 final class Serialize
@@ -48,6 +49,20 @@ public static function getAdminSerializer(): \Symfony\Component\Serializer\Seria
         return Pimcore::getContainer()->get('pimcore_admin.serializer');
     }
 
+    public static function getSerializer(): Serializer{
+        return Pimcore::getContainer()->get('pimcore.serializer');
+    }
+
+    public static function toJson(array $data, int $options = 0): string
+    {
+        return self::getSerializer()->encode($data, 'json', ['json_encode_options' => $options]);
+    }
+
+    public static function fromJson(string $json): array
+    {
+        return self::getSerializer()->decode($json, 'json');
+    }
+
     /**
      * @internal
      *
diff --git a/models/Asset.php b/models/Asset.php
index bc1bee26074..13fdcdb08a3 100644
--- a/models/Asset.php
+++ b/models/Asset.php
@@ -1303,7 +1303,7 @@ public function setCustomSettings(mixed $customSettings): static
                 $this->customSettingsCanBeCached = false;
             }
 
-            $customSettings = Serialize::unserialize($customSettings);
+            $customSettings = Serialize::fromJson($customSettings);
         }
 
         if ($customSettings instanceof stdClass) {
diff --git a/models/Asset/Dao.php b/models/Asset/Dao.php
index 6d1ebfeb6c0..8557d0d8d45 100644
--- a/models/Asset/Dao.php
+++ b/models/Asset/Dao.php
@@ -118,7 +118,7 @@ public function update(): void
         foreach ($asset as $key => $value) {
             if (in_array($key, $this->getValidTableColumns('assets'))) {
                 if (is_array($value)) {
-                    $value = Serialize::serialize($value);
+                    $value = Serialize::toJson($value);
                 }
                 $data[$key] = $value;
             }
@@ -493,7 +493,7 @@ public function areAllowed(array $columns, User $user): array
 
     public function updateCustomSettings(): void
     {
-        $customSettingsData = Serialize::serialize($this->model->getCustomSettings());
+        $customSettingsData = Serialize::toJson($this->model->getCustomSettings());
         $this->db->update('assets', ['customSettings' => $customSettingsData], ['id' => $this->model->getId()]);
     }
 


Test patch: