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
Default to real-utf8 when no charset is provided #1070
Conversation
0eb0541
to
0ec46e1
Compare
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.
Shouldn't this be contributed directly in the DBAL, inside DriverManager::getConnection()
?
0ec46e1
to
5b88b4b
Compare
That is not required, and that is a project with policies that are totally decoupled from Symfony. |
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'm going to create an issue in DBAL to make this piece of code redundant, that way it can be done at a slower pace later.
I would love to have this out of use-land config. For 2.0 would have been better, but 2.0.1 I think works in practice to finally remove the config need. |
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.
That's a great idea!
I wasn't aware of this problem, so doing this in 2.0.1 makes sense to me. Thanks @nicolas-grekas! |
} | ||
} | ||
|
||
$connection = new $connection($params, $driver, $connection->getConfiguration(), $connection->getEventManager()); |
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'd rather use DriverManager::getConnection
again rather than assuming the constructor argument here
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.
That's the "contract" of all connection classes: the wrapper_class parameter already requires this signature.
@ggeorgiev-sfly both things were expected when writing the PR, and approved, because we made the (wrong) assumption that no heavy lifting would take place here… it doesn't look like it's the case: https://github.com/doctrine/dbal/blob/7be002cc478ebf70dc9c261b9e8dec225e9d8c2a/lib/Doctrine/DBAL/Connection.php#L163-L195 😅 @nicolas-grekas @alcaeus maybe we should revert this? |
@greg0ire it would be better to understand what happens. |
My understanding is that @ggeorgiev-sfly overrode the wrapper class thanks to this option: https://github.com/doctrine/dbal/blob/7be002cc478ebf70dc9c261b9e8dec225e9d8c2a/lib/Doctrine/DBAL/DriverManager.php#L171-L178 But because of https://github.com/doctrine/DoctrineBundle/pull/1070/files#diff-5b0dbf30dd0e8ce451fd72020914e62cR60 , the constructor of the wrapper class is now called twice. While this looks fine for https://github.com/doctrine/dbal/blob/7be002cc478ebf70dc9c261b9e8dec225e9d8c2a/lib/Doctrine/DBAL/Connection.php , it may not always be fine for any user-defined wrapper class. |
Creating a connection to the backend in the constructor of the wrapped class is a critical issue with that constructor. I'm not sure we should consider this as legit. |
@ggeorgiev-sfly can you please give more details about what you are doing here? We suspect there might be a better place for you to do what you need than in the constructor of the connection. |
Thanks for the quick replies, here are some details regarding my case:
|
Fixed in #1087 @ggeorgiev-sfly btw, if you have ideas to improve either the bundle or dbal, please send some PRs :) |
@ggeorgiev-sfly can you please confirm the PR fixes your issue? Thanks! |
@alcaeus @nicolas-grekas |
The database url can contain the charset too, so we need to parse it before anything. |
I like to offer an alternative solution #1098 by setting the default charset in the DI |
Configuring charset in a portable way is a real pain for the community.
See e.g. symfony/recipes#674, which triggered symfony/recipes#689, then https://github.com/symfony/recipes/pull/691/files, but the situation is still not fixed.
This is the only way to fix the issue in a way that would make sense: use real-utf8 by default for all platforms (ie utf8mb4 on Mysql).
Here is the patch doing so, targeting 2.0.1.
I'd happily submit on 1.12 if you think that's possible.