-
-
Notifications
You must be signed in to change notification settings - Fork 485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the code compatible with DBAL 3 and 4 by swithing the type when… #1685
base: 5.x
Are you sure you want to change the base?
Conversation
There is the error
|
6f8282e
to
cec9f6a
Compare
…the Mapping is loaded
cec9f6a
to
85202a9
Compare
@VincentLanglet hopefully this is good to go! |
/** | ||
* @psalm-suppress InvalidPropertyAssignmentValue | ||
*/ | ||
$metadata->fieldMappings['roles']['type'] = 'array'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the right way to change the value ?
The property is documented as READ-ONLY
https://github.com/doctrine/orm/blob/3d9af3187f59930972e7fcb90a1d8059a37b8032/src/Mapping/ClassMetadata.php#L334-L339
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
public function setAttributeOverride(string $fieldName, array $overrideMapping): void
should be used instead. (https://github.com/doctrine/orm/blob/3.1.x/src/Mapping/ClassMetadata.php#L1737)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VincentLanglet https://github.com/doctrine/orm/blob/3.1.x/src/Mapping/ClassMetadata.php#L1765-L1767 it is not possible to change the type using this way.
This solution is a hack but to be honest having a second class BaseUser3
just to change the type is not good either. This can end up, with BaseUser4
etc ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any recommendation for such situation @greg0ire ? Is it the best way ?
The doc need to be updated too, in order to remove reference of BaseUser3 https://github.com/sonata-project/SonataUserBundle/blob/a6169f318ad30a46ced640fad58a686e695e86f0/docs/reference/installation.rst#doctrine-orm-configuration |
We probably need to find a way to Migrate the User Data? My current attempt is this Doctrine Migration file: $result = $this->connection->executeQuery('SELECT id, roles FROM fos_user_user');
foreach ($result->fetchAllAssociative() as $item) {
$this->connection->update('fos_user_user', [
'roles' => json_encode(unserialize($item['roles']))
], [
'id' => $item['id']
]);
}
$this->addSql('ALTER TABLE fos_user_user CHANGE roles roles JSON NOT NULL'); |
@VincentLanglet @greg0ire how is the status for this MR? And do we need an update Script for existing data? |
See #1685 (comment) You can do it if you want this to be merged |
@VincentLanglet the Doc update is one thing, should we try to something like my Update Script in a more automated way? Or is the Dev alone with this? (i also don't know if my way is the correct one) |
We can document some helper/migrations about how to move from DBAL 3 to DBAL 4 but this cannot be included inside the SonataUserBundle code since we cannot be sure
|
Make the code compatible with DBAL 3 and 4 by swithing the type when the Mapping is loaded
I am targeting this branch, because because it introduces a non necessary BC break. For reference: #1677
Changelog
To do