-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
Fix creating wrapped connection twice #1087
Conversation
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.
Seems this doesn't fully solve original problem. Also I would say tests are required here.
$params['charset'] = 'utf8'; | ||
$driver = $connection->getDriver(); | ||
$wrapperClass = Connection::class; | ||
if (isset($params['wrapperClass'])) { |
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.
Not a fan of inlining this logic here, that's what's DriverManager for. Other places using it are still having old behaviour and in case we update DriverManager, it can break ConnectionFactory, since it unsets wrapperClass option.
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 agree this should be ported to DriverManager, but fixing this here was the faster choice for Symfony users. Once this is fixed in DBAL 2.9, we can remove it here and bump the doctrine/dbal dependency.
Test added. About your other comments @ostrolucky I didn't understand what you're suggesting instead. To me, this effectively solves the reported issue. And I can't think of any alternative implementation. Do you? |
My point was it should be fixed in Besides this being ugly fix (34, poorly tested LOC with half code copy pasted; which could be < 10 in DBAL), it still instantiates original Connection class twice so by avoiding fixing it in DBAL we are implementing subpar solutions. I believe #1070 causes other unreported issues as well. Since we are no longer touching recipe which is generated only at beginning of installing bundle, it changes charset for everybody not specifying one from usually default Perhaps we can also just ignore this problem for a little while longer, since MySQL 8 fixed this itself by defaulting to |
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.
Test case is incorrect. It tests that driver was instantiated once, while it should test connection wrapper is instantiated once only. Driver was never a problem.
Indeed, fixed. |
Thanks @nicolas-grekas! |
As explained in #1070 (comment)