sonata-project / SonataUserBundle

Symfony SonataUserBundle
https://docs.sonata-project.org/projects/SonataUserBundle
MIT License
341 stars 487 forks source link

Compatibility with doctrine dbal 4 #1677

Closed RobinDev closed 6 months ago

RobinDev commented 8 months ago

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

### Added
- BaseUser3 with roles mapped type as `json` to be compatible with ORM 3
RobinDev commented 8 months ago

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 commented 8 months ago

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

core23 commented 8 months ago

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 commented 8 months ago

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

Wdyt @RobinDev ?

RobinDev commented 8 months ago

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 commented 7 months ago

I made an other PR on 6.X #1680

VincentLanglet commented 7 months ago

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 commented 7 months ago

Maybe you have a suggestion @jordisala1991 @dmaicher ?

RobinDev commented 7 months ago

I follow your suggestions.

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

VincentLanglet commented 7 months ago

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 commented 7 months ago

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

RobinDev commented 7 months ago

Done

VincentLanglet commented 7 months ago

Done

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

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

VincentLanglet commented 6 months ago

Thanks @RobinDev

rande commented 5 months ago

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 commented 5 months ago

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

VincentLanglet commented 5 months ago

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

Sure !

rande commented 5 months ago

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

Hanmac commented 4 months ago

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?