-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/sqlite3: SQLite3::close, SQLite3Stmt::close and SQLite3Result::fi... #10878
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I agree in theory, but I worry there are people doing:
if ($db->close()) { ... } else { ... }
will have the code logic completely changed. But then waiting for PHP 9 for this changes means we would need to change the return types again :/
UPGRADING
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems more accurate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
ext/sqlite3/sqlite3.stub.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment needs to be removed and I think also replaced with /** @tentative-return-type */
Is this correct @kocsismate ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at another void function, you seem to be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
...nalize changed from bool return value to void.
66aae9d to
ffb1a1f
Compare
I'd love if the signatures in question could be fixed, but I'm also worried about the unsatisfactory upgrade path which I mentioned in a similar PR. I'm afraid we cannot silently change these signatures this fundamentally :( IMO at least there should be an RFC in order to provoke a discussion and in order to settle on a solution which the majority accepts.
but I'm also worried about the unsatisfactory upgrade path
IMO at least there should be an RFC in order to provoke a discussion and in order to settle on a solution which the majority accepts.
I agree. The true return type doesn't affect normal usage of the API. Thus changing it might not be considered useful. Changing the return type from bool to true might offer similar benefits for static analysis (condition is always true error).
Those are fair points indeed.
Changing the return type from bool to true might offer similar benefits for static analysis (condition is always true error).
I agree. This path is more reasonable. It will not change the logic, but will encourage people to change the code what will allow to change the return type to void eventually.
...nalize
changed from bool return value to void.