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

Add an additional condition before reconnect() #47

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

michalsn
Copy link
Member

@michalsn michalsn commented Sep 18, 2024

Description
This PR checks if the connection has been established before we call the reconnect() method.

When a connection to the database is lost, the $this->db->connID remains valid in terms of its existence, but the actual connection might be closed or invalid.

There are no actual tests for this because I don't know how I can test it - we have a valid DB connection during the tests. Although I confirmed this was a bug and this change fixes it.

Needs #46

Fixes #45

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10923540520

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.433%

Totals Coverage Status
Change from base Build 10923488005: 0.0%
Covered Lines: 487
Relevant Lines: 557

💛 - Coveralls

@kenjis
Copy link
Member

kenjis commented Sep 19, 2024

Isn't this a bug in the framework?
I think reconnect() should not cause the error.

@michalsn
Copy link
Member Author

I'm not sure how to treat this.

The reconnect() method should only be used after the connection to the database has been established. It used to be that the connection was established immediately, but I think at some point we made a change so that it was only established when we wanted to use the connection (for the first time) - hence the possibility of an error.

I think it is better to show the error than to hide it. In the sense of silently omitting the reconnect() call - because that's what I think it would come down to in order to get around this problem on the framework side.

@kenjis
Copy link
Member

kenjis commented Sep 19, 2024

When a connection to the database is lost, the $this->db->connID remains valid in terms of its existence, but the actual connection might be closed or invalid.

Does this PR fix the error when a connection to the database is lost?

@michalsn
Copy link
Member Author

Does this PR fix the error when a connection to the database is lost?

Yes, because in that case $this->db->connID is not false.

@kenjis
Copy link
Member

kenjis commented Sep 19, 2024

Okay. I got your opinion:

  1. When the connection is already made (and lost), we should call reconnect().
  2. When the connection is not made, we should not call reconnect(). (The DB connection connects automatically when needed).

@michalsn michalsn merged commit 8377361 into codeigniter4:develop Sep 19, 2024
13 checks passed
@michalsn
Copy link
Member Author

Thank you!

@michalsn michalsn deleted the fix-reconnect branch September 19, 2024 07:20
@kenjis kenjis added the bug Something isn't working label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev: Postgresql queue:jobs
3 participants