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.

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

Finalize only if db is available #78

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Finalize only if db is available #78

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
722490c
Finalize only if db is available
lekoala Jun 26, 2024
1ba9f7b
use isActive
lekoala Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion code/SQLite3Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function __construct(SQLite3Connector $database, SQLite3Result $handle)

public function __destruct()
{
if ($this->handle) {
if ($this->handle && $this->database->getSelectedDatabase()) {
Copy link
Member

Choose a reason for hiding this comment

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

Would isActive() be more appropriate to call here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes is active should work just as well but brings no additional benefit except being probably more correct semantically

https://github.com/silverstripe/silverstripe-sqlite3/blob/1e8d7357be321b3eb32a596538078fd65d0d357e/code/SQLite3Connector.php#L184C21-L188

if you see, unloading the db remove the databaseName and close the connection. isActive only checks if there is a dbConn object... and there would be a closed one. :-) so either way should work just fine. i don't think there is a way to check if the dbConn was indeed closed.

Copy link
Member

Choose a reason for hiding this comment

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

Yup that's what I was thinking too. If you change it to isActive() I'll be happy to merge. I like semantically correct ;p

$this->handle->finalize();
}
}
Expand Down
Loading