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

SqLite: Fix fatal error when resource closed prior to result destructing #430

Closed
wants to merge 9 commits into from

Conversation

dakujem
Copy link
Contributor

@dakujem dakujem commented Nov 28, 2022

Bug fix; bc break: NO

I'm running into an issue in a certain setup with SqLite driver:
image

See, the resource has already been closed before the destruction of the result object.

Adding is_resource check is an efficient way to prevent it, possibly even the suppression operator @ on line 40 might become redundant after this change.

I'm guessing this is similar to #409.

@dakujem
Copy link
Contributor Author

dakujem commented Nov 28, 2022

I forgot to add that this only happens with PHP 8 and above.

@dakujem
Copy link
Contributor Author

dakujem commented Dec 22, 2022

@dg bump.

@dg
Copy link
Owner

dg commented Aug 5, 2023

is_resource($this->getResultResource()) this will always return false, that's probably not the right fix…

@dakujem
Copy link
Contributor Author

dakujem commented Aug 8, 2023

I fail to understand the reason for rejecting this PR. If it did not happen I would not have been fixing it.
Why do you think is_resource($this->getResultResource()) will always evaluate to false?

@dakujem
Copy link
Contributor Author

dakujem commented Aug 8, 2023

How is this different to #431?

@dg
Copy link
Owner

dg commented Aug 9, 2023

Because getResultResource() returns object.

@dakujem
Copy link
Contributor Author

dakujem commented Aug 10, 2023

... and in 8e7df83 you completely removed the auto-free feature, which renders this PR as well as #431 irrelevant anyway.

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.

2 participants