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

feat(database): add closable connection wrapper for PDO connection #875

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

blackshadev
Copy link
Contributor

@blackshadev blackshadev commented Dec 29, 2024

What

Closes #872

  • add closable connection wrapper for PDO connection

Why

  • As mentioned in feat(console): add task component #857 it is sometimes needed to manually close the PDO connection. With this PR you can manually close (and re-connect) the PDO connection.
  • Additionally, in the future this code can be expanded upon to have multiple active connections, or even other "kinds" of connections besides PDO

Copy link
Member

@innocenzi innocenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great! Can you think of a way to test that the connection is actually closed? 🤔

#[Singleton]
public function initialize(Container $container): Connection
{
// Reuse same connection instance in unit tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is only meant to be used in tests, can we ensure we are actually running in tests? We can check the current environment from the Kernel I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a cary over from the PDOInitializer which had the same. But yeah, good suggestion. I will make it such that it only "caches" when in testing. This is more of a test speed+memory improvement than anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the check.

@blackshadev
Copy link
Contributor Author

That looks great! Can you think of a way to test that the connection is actually closed? 🤔

It depends on your meaning of "test".. I can Add a test for the close functionality: yes. It will just check if the PDO connection has been set to null. I cannot test if the PDO driver really closes the connection, PDO doesn't expose anything for this.

Do you want me to add a unit test for the connect + close functionality?

@innocenzi
Copy link
Member

I think what would be ideal is that the PdoConnection class throws a ConnectionClosed exception when the internal $pdo variable is accessed if it's null. Right now, I think if we close the connection and try a query, we will have a cannot call method on null type exception.

Then, we can write a test that first ensure the connection works by issuing a query, then closing the connection, and finally expecting the exception when issuing another query

@blackshadev
Copy link
Contributor Author

I think what would be ideal is that the PdoConnection class throws a ConnectionClosed exception when the internal $pdo variable is accessed if it's null. Right now, I think if we close the connection and try a query, we will have a cannot call method on null type exception.

Then, we can write a test that first ensure the connection works by issuing a query, then closing the connection, and finally expecting the exception when issuing another query

Implemented the exception and the test using sqllite.

@innocenzi innocenzi requested a review from brendt December 29, 2024 20:56
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.

[Bug]: Connections to the database cannot be closed
2 participants