-
-
Notifications
You must be signed in to change notification settings - Fork 478
Remap collate to collation when appropriate #1471
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
Conversation
|
@aprat84 @dmaicher @natewiebe13 @floviolleau please test |
|
Tested this as I found the same behaviour described in doctrine/dbal#5243, it doesn't change anything. However, changing the |
|
Similar experience here. Using this branch using |
|
Also, it's probably worth updating the older recipes (provided they support Refs:
|
|
@natewiebe13 that should be done in a follow-up PR targeting 2.6.x: this one is not about the public interface of DoctrineBundle, but about adapting to the public interface of I'm noting that this improves your experience only when switching to |
|
@greg0ire I've updated by test project here: https://github.com/natewiebe13/doctrine-dbal-bug This is likely the way most Symfony projects are configured, as it was at one time either suggested through a recipe/recommended through docs. Specifically this: natewiebe13/doctrine-dbal-bug@c834489 What's happening is because of the logic on the parent if statement, the path completely skips the logic you added and directly inputs the configuration into here: https://github.com/doctrine/DoctrineBundle/blob/2.5.x/ConnectionFactory.php#L98 |
|
Ok, so this means this bundle is not involved at all in the bug you are experiencing with this project, right? |
|
Based on the example I provided, that's my thinking as well in terms of the bundle's involvement in configuring the dbal connection. That being said, it is unfortunate that the change in doctrine/dbal is causing this to happen. Do you think it's worth adding a conflicts with dbal > 3.3.0 and then resolving that in 2.6 or something? I'm just wondering if there's a way to make it more obvious for people that run into this without it triggering new issues being created. |
|
I don't think it's worth it, IMO it might just prevent people from upgrading the DoctrineBundle. |
688e079 to
100687e
Compare
|
+1 on this fix @greg0ire After your patch |
5de2f74 to
da6d38f
Compare
| "doctrine/annotations": "^1", | ||
| "doctrine/cache": "^1.11 || ^2.0", | ||
| "doctrine/dbal": "^2.13.1|^3.1", | ||
| "doctrine/dbal": "^2.13.1|^3.3.2", |
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.
Since you're targeting the 2.5 branch: Are we sure we want to do this kind of bump in a bugfix release?
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 it's OK since only 3.3 is supported.
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.
But I'm not sure what's the policy on this. Maybe @ostrolucky does?
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 don't mind
collate was never supposed to be supported and used in the first place, and DBAL 3.3.2 fixed that. Here, we avoid the situation where a user willing to use the correct option ends up with both collate and collation defined, and we remap collate to collation when DBAL 3.3 is detected. DBAL 3.3.0 and 3.3.1 are avoided thanks to a composer version constraint.
But before that, I need to figure out several things:
UPD: Created |
|
Thanks for creating those issues, and if UTF8 does indeed seem like a wrong default, then opening an issue with Symfony Docs would be helpful. In the mean time, how explicitly would you change the following for it to be more sensible and explicit: Do any other keyed elements need to be added? Once we are clear on the recommended default config for utf8-style configuration, do you think I should focus more on the up or down schema generation for further debugging? Should My guess is "yes" given the Based on the comments here and my debugging, it sounds like the |
|
For the moment this is my recommendation doctrine:
dbal:
driver: pdo_mysql
charset: utf8mb4
default_table_options:
charset: utf8mb4
collation: utf8mb4_unicode_ci # or even utf8mb4_0900_ai_ci if using MySQL 8
host: '%env(DATABASE_HOST)%'
port: '%env(DATABASE_PORT)%'
dbname: '%env(DATABASE_NAME)%'
user: '%env(DATABASE_USER)%'
password: '%env(DATABASE_PASSWORD)%'Then you can try inserting emojis in your table 🙂 |
|
@arderyp until the DBAL is able to do the charset/collation migration, maybe use |
|
I am comparing setting if it makes any difference, my database is set to |
No, I thought your tables where already using that, as show by
What throws the error? The server? PDO? |
|
upper case
|
|
Well that's confusing to say the least 🤔 |
|
ok, I am making some progress. If I change my config to I am still getting other down migrations, but that is for the handful of tables that are defined as then manually creating However, there is still something wrong with the code. I;m now even more convinced of my proposal above that the issue may in fact not be with There's also the remaining question, should |
|
in other words, |
Congrats!
Again, not a specialist, but I think I would recommend migrating to
That was my thinking as well, and it's explained by doctrine/dbal#4945 I think.
Good question. Using your MySQL client, can you try creating a table for both cases, then use |
|
@greg0ire sure I'm happy to do that, but can you be more explicit? You want me to run the create at the mysql terminal? Example command? I can't really remember if I created my newer tables manually then generated Doctrine entities from that, or vice versa. |
Yes CREATE TABLE Foo (Bar INT NOT NULL) DEFAULT CHARACTER SET utf8;
CREATE TABLE Bar (Foo INT NOT NULL) DEFAULT CHARACTER SET UTF8;
SHOW CREATE TABLE Foo;
SHOW CREATE TABLE Bar; |
|
|
Interesting. Can you also try Anyway, I think you'll agree with me that |
|
for documentation, this works, and this does not. Sequence: I have not yet determined if the issue lies with |
|
Yes, it seems Anywho, you said "Can you also try utf8mb3"... what's that in reference to? Setting that as my config charset or re-running the table creations using |
Yes, for science. I'm wondering if you will get an error message this time. |
|
for science 🥂 |
|
@greg0ire unfortunately, I feel like I'm reaching the limit of my debugging capabilities here. As far as I've gotten is tracking the issue to here: https://github.com/doctrine/dbal/blob/3.3.x/src/Platforms/MySQL/Comparator.php#L56 My live table schema is having it's table definition mapped properly to its columns: However, the I'm kind of at a dead end, but maybe this will give you and idea for where we should look next. |
@greg0ire this is what I get |
|
Ok so |
Yes indeed. Just the empty |
|
Yes, and I think it is that bug: doctrine/dbal#4945 I don't understand why it affects only |
|
@greg0ire the description of doctrine/dbal#4945 seems to reflect the broken |
|
@greg0ire , given this comment... My mysql server is currently 5.7 but will be updated soon to 8. I'm considering converting all tables/fields to Should I migrate to Given that you know a great deal about this, I consider your insight valuable, if you have the time. Thanks in advance. |
Actually I don't, I just skimmed through the posts I found. I never paid attention to collation until starting to work on this.
|
|
Thanks for the feedback @greg0ire! |
collatewas never supposed to be supported and used in the first place,and DBAL 3.3.2 fixed that.
Here, we avoid the situation where a user willing to use the correct
option ends up with both
collateandcollationdefined, and we remapcollatetocollationwhen DBAL 3.3 is detected. DBAL 3.3.0 and 3.3.1 areavoided thanks to a composer version constraint.
Might fix #1468 , might also fix doctrine/migrations#1240, and might also fix doctrine/DoctrineMigrationsBundle#470
How can I test this?
composer config repositories.greg0ire vcs https://github.com/greg0ire/DoctrineBundle composer require doctrine/doctrine-bundle "dev-collation as 2.5.5"