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

fix: Prefer primary config over master. #1258

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

prengerjarno
Copy link
Contributor

Make sure the primary config gets used over the master so it uses the added options when PrimaryReadReplicaConnection is used. #1257

@ostrolucky
Copy link
Member

ostrolucky commented Nov 12, 2020

New primary config was added in 2.2, so this is the branch this should point to.

Also, I'm not really understanding the fix. Why is primary getting overwritten with master if you specify primary?

@prengerjarno
Copy link
Contributor Author

@ostrolucky I'm sorry, its not the config, its the params. Both 'master' and 'primary' params are present to because of the BC support in doctrine/dbal's MasterSlaveConnection. In this case it should use the 'primary' params, but is using the 'master' which is missing the 'driver' option (because it only gets added to 'primary' in PrimaryReadReplicaConnection::__construct).

@ostrolucky
Copy link
Member

If DBAL is adding driver to primary only and not to master, that's a bug in DBAL's BC layer and you need to report issue there.

@stof
Copy link
Member

stof commented Nov 13, 2020

@ostrolucky ->getParams is an API marked as internal.

@ostrolucky
Copy link
Member

Up to dbal 2.11 initiating connection with master would result in getParams giving you back array including master['driver'] and then it was changed because in 2.11 it was marked internal and that's ok? That's not how BC works. Method cannot be broken in minor release like that and claim it's ok because it's internal now. It can be broken in 3.x, not in 2.x.

@ostrolucky ostrolucky added this to the 1.12.13 milestone Nov 13, 2020
@greg0ire greg0ire closed this Nov 14, 2020
@greg0ire greg0ire reopened this Nov 14, 2020
@greg0ire
Copy link
Member

I managed to trigger a build.

@greg0ire greg0ire merged commit 85460b8 into doctrine:1.12.x Nov 14, 2020
@greg0ire
Copy link
Member

Thanks @prengerjarno

@prengerjarno prengerjarno deleted the prefer-primary-over-master branch November 16, 2020 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants