8000 Remove dead code from `MasterSlaveConnection::connect()` by jnvsor · Pull Request #2622 · doctrine/dbal · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove dead code from MasterSlaveConnection::connect() #2622

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 27, 2017
Merged

Remove dead code from MasterSlaveConnection::connect() #2622

merged 1 commit into from
Jan 27, 2017

Conversation

jnvsor
Copy link
Contributor
@jnvsor jnvsor commented Jan 27, 2017

Currently connect unsets the slave connection right before overwriting it with the master anyway.

@deeky666
Copy link
Member

@jnvsor can you please explain why this is dead code?

@jnvsor
Copy link
Contributor Author
jnvsor commented Jan 27, 2017 •
edited
Loading

Actually you're right, it's not dead it's duplicate. (Should I rename the commit/PR?)

Specifically, it unsets $this->connections['slave'] under the condition that !$this->keepSlave

2 lines later it sets $this->connections['slave'] to the newly acquired $this->connections['master'] when !$this->keepSlave: If the first line executed, so will the second. Either way If the connection object no longer has any references it will be gc'd

Unless $this->connectTo('master') does something as unintuitive as querying the slave database there won't be any difference. (I checked, It doesn't)

@deeky666 deeky666 self-requested a review January 27, 2017 23:12
@deeky666 deeky666 self-assigned this Jan 27, 2017
@deeky666 deeky666 added this to the 2.6 milestone Jan 27, 2017
@deeky666 deeky666 merged commit 91d1eeb into doctrine:master Jan 27, 2017
@deeky666
Copy link
Member

Got it! Thanks for clearing that up. I'll accept it as is, as this code path is rather hard to test.

@Ocramius Ocramius changed the title MasterSlaveConnection::connect: Remove dead code Remove dead code from MasterSlaveConnection::connect() Jul 22, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@deeky666 deeky666 deeky666 approved these changes

Assignees

@deeky666 deeky666

Labels
Connections Improvement
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jnvsor @deeky666 @Ocramius
0