Skip to content
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

False collation on current 2.6.x-dev 89b7ed6 version #1480

Closed
alexander-schranz opened this issue Mar 2, 2022 · 19 comments · Fixed by #1481
Closed

False collation on current 2.6.x-dev 89b7ed6 version #1480

alexander-schranz opened this issue Mar 2, 2022 · 19 comments · Fixed by #1481
Labels
Milestone

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Mar 2, 2022

Bug Report

I'm trying to find out why my table schema is not correclty utf8mb4 after using @dev dependencies.

Q A
Version 2.6.x-dev 89b7ed6
MySQL 5.7.37 / 8.0.27
PHP 8.1.2

Summary

On current stable version all is correctly in my database utf8mb4. I have no collation set manually in doctrine.yaml. But after ugprading to @dev version the collation is created not longer as utf8mb4 which was the default since I think dbal 3 for mysql.

Current behaviour

The schema has a strange mixed collation now:

Bildschirmfoto 2022-03-02 um 13 33 18

How to reproduce

composer create-project sulu/skeleton
cd skeleton
composer config minimum-stability dev
composer update # installs doctrine-bundle 2.6.x-dev 89b7ed6
docker compose up
bin/console sulu:build dev
# check database e.g. `ac_activities` table

Expected behaviour

On 2.5.6 all works like expected:

Bildschirmfoto 2022-03-02 um 13 36 43

Maybe related to #1478 /cc @greg0ire

@ostrolucky
Copy link
Member

I don't think we changed anything in doctrine-bundle that could have affected this. You should find first which dependency caused this (most likely dbal).

@alexander-schranz
Copy link
Contributor Author

@ostrolucky already tested that. If I'm downgrading only doctrine-bundle to 2.5.6 and all other dependencies are unchanged the collation is correctly created.

The first run is with 2.6.x-dev 89b7ed6
The second one with 2.5.6

all other dependencies are the same:

doctrine

@dmaicher
Copy link
Contributor

dmaicher commented Mar 2, 2022

Strange... I also cannot spot anything that would explain this issue in the diff

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Mar 2, 2022

I would think that maybe doctrine/dbal still requires both to be define collate and collation:

Now only the new option is defined: f1423b2#diff-df25e842a7fa5aa6cfb5504f8a15789ce8988d620bcd4ace17d2000222feb507

But need to test that out if I added the other option if it works then.

Manually defining the default table options did atleast use the utf8mb4 this way did create the correct schema:

doctrine:
    dbal:
        default_table_options:
            charset: utf8mb4
            collate: utf8mb4_unicode_ci

So it need have todo with the table default options.

@greg0ire
Copy link
Member

greg0ire commented Mar 2, 2022

Do you have any charset option defined in your database_url as well?

Maybe related to #1478 /cc @greg0ire

It does look related, maybe you can try pinning to a commit before and after my PR, and see what happens?

@alexander-schranz
Copy link
Contributor Author

@greg0ire we have nothing define also not in the doctrine.yaml:

https://github.com/sulu/skeleton/blob/2.x/config/packages/doctrine.yaml
https://github.com/sulu/skeleton/blob/2cb154888d20fd71022b8636a46015649cf05166/.env#L44

Defining it is just the current workaround.

@greg0ire
Copy link
Member

greg0ire commented Mar 2, 2022

Here is why I asked: symfony/recipes#1059

Are you going through this code path:

} else {
$params['charset'] = 'utf8';
}
?

@alexander-schranz
Copy link
Contributor Author

On commit f1423b2a640b6ac545b6e0c02575427a4dc1e9dc (2.5.6) it works and on next commit 1b75234e580e1682a57468d6af7ac5c9d4688d0c it fails:

commit 1b75234e580e1682a57468d6af7ac5c9d4688d0c (HEAD)
Merge: 543ec0f f1423b2
Author: Grégoire Paris <postmaster@greg0ire.fr>
Date:   Sun Feb 13 22:48:32 2022 +0100

    Merge pull request #1477 from doctrine/2.5.x

    Merge 2.5.x up into 2.6.x

1b75234

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Mar 2, 2022

Its strange on 2.5.6 the $driver is: Doctrine\DBAL\Driver\PDO\MySQL\Driver
on that commit $driver is: Doctrine\DBAL\Logging\Driver

@alexander-schranz
Copy link
Contributor Author

That is why on that commit the utf8mb4 part is skipt.

@alexander-schranz
Copy link
Contributor Author

Changing:

-                if ($driver instanceof AbstractMySQLDriver) {
+                if ($driver->getDatabasePlatform() instanceof AbstractMySQLPlatform) {

Seems to work.

@greg0ire
Copy link
Member

greg0ire commented Mar 2, 2022

I think what broke this was #1456

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Mar 2, 2022

Yes I tested the 2.5.x on that works and 2.6.x does not work. I also think it related to the logging middleware handling. But could not yet found a solution for it which does not make any tests failing.

@alexander-schranz
Copy link
Contributor Author

We got a report from a doctrine-bundle 2.5.6 project which did have the same problem only on production. After debugging I have found out on production there is Sentry enabled which is also decorating the Driver and they end so in the same scenario with the tracingdriver of sentry:

https://github.com/getsentry/sentry-symfony/blob/master/src/Tracing/Doctrine/DBAL/TracingDriverForV3.php

@greg0ire
Copy link
Member

greg0ire commented Mar 3, 2022

One more reason to strongly deprecate this codepath.

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Mar 3, 2022

@greg0ire time to bring back ensure production config command which requires than serverVersion and charset being set ;)

@Mika56
Copy link

Mika56 commented Mar 3, 2022

Confirming we're only seeing this issue when SentryBundle is enabled.
Workaround is enabling SentryBundle only in prod in your config/bundles.php file

@alexander-schranz
Copy link
Contributor Author

@Mika56
Copy link

Mika56 commented Mar 3, 2022

@alexander-schranz That did not fix the problem for us. Our migrations were trying to MODIFY every string column in the down() part

@ostrolucky ostrolucky added the Bug label Mar 5, 2022
@ostrolucky ostrolucky added this to the 2.5.7 milestone Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants