Compatibility with doctrine dbal 4

[RobinDev]

Compatibility with doctrine dbal 4

Just switching the deprecated array type to json - cf Upgrade Guide

I am targeting this branch, because it's a patch wich is not introducing bc break.

About BC Break, i am not sure, because it's required to update from unserialized to json encoded.

Changelog

### Changed
- roles mapped type (orm) from `array` to `json`
  

[RobinDev]

About BC Break, i am not sure, because it's required to update from unserialized to json encoded.

I run a migration on a current project using sonataUserBundle, should I add it to this PR ?

<?php

declare(strict_types=1);

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

final class Version1709056840 extends AbstractMigration
{
    public function up(Schema $schema): void
    { 
        $rows = $this->connection->fetchAllAssociative('SELECT id,roles FROM user');

        /** @var array{'id': scalar, 'roles': string}[] $rows */
        foreach ($rows as $row) {
            $id = $row['id'];
            $roles = json_encode(unserialize($row['roles']));
            $this->connection->executeQuery('UPDATE user SET roles = :roles WHERE id = :id', ['roles' => $roles, 'id' => $id]);
        }
    }
}

[VincentLanglet]

WDYT @jordisala1991 ? Should we consider this as a BC break ?

[core23]

I would handle this as a BC break, because without a migration you can't load the roles anymore.

As this bundle does not provide any auto-migration script, there is no way to run composer update on a project without any problems.

[VincentLanglet]

I would handle this as a BC break, because without a migration you can't load the roles anymore.

As this bundle does not provide any auto-migration script, there is no way to run composer update on a project without any problems.

Yeah I was afraid about this.

Maybe we could have two files BaseUser.orm2.xml and BaseUser.orm3.xml and one of the two solution

• Automatically load the right file based on the ORM version
• Adding a configuration option to tell which file should be loaded

Wdyt @RobinDev ?

[RobinDev]

I think your suggestions permit to ship it now and maintain compatibility with dbal 3 for a moment... but it's adding some complexity to maintain.

I would have a preference to target the 6.X branch wich is available via composer.

It will permit for the projects that required an upgrade of dbal to target the next version.

Feel free to modify my PR, i will not be available during the next 8 days.

[RobinDev]

I made an other PR on 6.X #1680

[VincentLanglet]

I think your suggestions permit to ship it now and maintain compatibility with dbal 3 for a moment... but it's adding some complexity to maintain.

I would have a preference to target the 6.X branch wich is available via composer.

There is less complexity maintaining one branch, even with some config loaded conditionally rather than maintaining two branch. We don't plan to release 6.x branch soon, so I would say we don't plan to merge any PR on this branch.

So we have to find another solution...

[VincentLanglet]

Maybe you have a suggestion @jordisala1991 @dmaicher ?

[RobinDev]

I follow your suggestions.

So if an user want to use doctrine/orm ^3 :

• the user must extend is entity with BaseUser3 instead of BaseUser
• the user must manage the migration if the entity was existing (sonata/user-bundle upgrade)

[VincentLanglet]

I follow your suggestions.

So if an user want to use doctrine/orm ^3 :

• the user must extend is entity with BaseUser3 instead of BaseUser
• the user must manage the migration if the entity was existing (sonata/user-bundle upgrade)

Seems like a nice solution without bc break. WDYT @core23 ?

[VincentLanglet]

I fixed the cs, you can rebase https://github.com/sonata-project/SonataUserBundle/pull/1681

[RobinDev]

Done

[VincentLanglet]

Done

Thanks, I'll try to get some opinions from others reviewers.

Ping @phansys @core23 @jordisala1991 @dmaicher @franmomu wdyt ?

[VincentLanglet]

Thanks @RobinDev

[rande]

Hello, coming late to this change.

All this code can be updated to use a Doctrine Listener to change the mapping type without creating a new class and definition, this will make the migration path way easier.

This is why we have the https://github.com/sonata-project/sonata-doctrine-extensions/blob/2.x/src/Mapper/DoctrineCollector.php in the sonata-project/doctrine-extensions project

[rande]

Ping @RobinDev @VincentLanglet - Do you want me to do a MR on this to avoid introducing a BC break ?

[VincentLanglet]

Ping @RobinDev @VincentLanglet - Do you want me to do a MR on this to avoid introducing a BC break ?

Sure !

[rande]

Done: https://github.com/sonata-project/SonataUserBundle/pull/1685

[Hanmac]

Are we sure about that array type?

https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#array says:

This type is deprecated since 3.4.0, use json instead.

right now, I get:

Unknown column type "array" requested.

Also we might need some better way to migrate the roles data?