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

Unset parameters for sharded databases as well #896

Merged
merged 2 commits into from Dec 11, 2018

Conversation

cinamo
Copy link
Contributor

@cinamo cinamo commented Dec 11, 2018

Creating a new database for the global database of a sharded configuration fails, because the command tries to connect to the database that doesn't exist yet.

The cause of this is that the dbname, url and path parameters are not unset for sharded databases, while they are unset for 'normal' connections.

@cinamo cinamo changed the title Fix #895 Unset parameters for sharded databases as well Unset parameters for sharded databases as well Dec 11, 2018
@cinamo
Copy link
Contributor Author

cinamo commented Dec 11, 2018

@kimhemsoe The issue is, in the provided test there's no url or path parameter set, which is why that test succeeds. The default Symfony 4 .env configuration uses a url parameter for the global connection, though...

Would you recommend I add a url parameter in the existing test? Or do you have another suggestion maybe? Thanks!

@kimhemsoe
Copy link
Member

Yes, a test showing what you are trying to fix would be nice.

@kimhemsoe kimhemsoe added this to the 1.10.1 milestone Dec 11, 2018
@cinamo
Copy link
Contributor Author

cinamo commented Dec 11, 2018

@kimhemsoe Updated a test that fails on the non-fixed version (but it's all dependent on the exact configuration anyway)

@stof
Copy link
Member

stof commented Dec 11, 2018

Do we need the same kind of fix for MasterSlaveConnection too ?

@cinamo
Copy link
Contributor Author

cinamo commented Dec 11, 2018

@stof It seems that case is covered by the default behavior (the connection parameters are taken from master and then reset in the same fashion as a ‘normal’ connection), so I don’t think so.

@kimhemsoe kimhemsoe added Bug and removed Needs Tests labels Dec 11, 2018
@kimhemsoe kimhemsoe self-assigned this Dec 11, 2018
@kimhemsoe kimhemsoe merged commit 5c13e81 into doctrine:master Dec 11, 2018
@xabbuh
Copy link
Member

xabbuh commented Jan 7, 2019

@alcaeus Should this be backported to the 1.10.x branch as it is part of that milestone?

@alcaeus
Copy link
Member

alcaeus commented Jan 7, 2019

@xabbuh Yes - we've had a minor hiccup with the history. I'll fix it today before releasing.

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 this pull request may close these issues.

None yet

5 participants