-
Notifications
You must be signed in to change notification settings - Fork 7.9k
PDO - Added PDO::disconnect and PDO::isConnected and refactored free object #19214
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
base: PHP-8.3
Are you sure you want to change the base?
Conversation
Clearly missed something in testing and will investigate. |
Please, fix the typos in the PR title (PHP -> PDO). Also, I think this requires RFC. |
Understood. I will begin exploring the RFC process. |
@@ -1027,8 +1044,6 @@ PHP_METHOD(PDO, errorCode) | |||
|
|||
ZEND_PARSE_PARAMETERS_NONE(); | |||
|
|||
PDO_CONSTRUCT_CHECK; |
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.
question: why did you remove/not add the construct checks?
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.
Here's the relevant section from my initial comment on this PR.
Both
PDO::errorInfo()
andPDO::errorCode()
may now be called on an uninitialized PDO object, i.e. one without a defineddriver
member. This change is being done to permit use of both functions afterPDO::disconnect()
is called, and as a result of modifying thePDO_CONSTRUCT_CHECK
macro to impose check onis_closed
.
Both method implementations look to tolerate the absence of a defined driver
member, with the exception of methods->fetch_err()
invocation from within errorInfo()
, where I've added the null check on methods
member as safeguard.
ext/pdo/pdo_dbh.stub.php
Outdated
@@ -394,6 +394,9 @@ public function beginTransaction(): bool {} | |||
/** @tentative-return-type */ | |||
public function commit(): bool {} | |||
|
|||
/** @tentative-return-type */ |
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.
you don't need to add tentative return type for new methods. It's purely for BC of existing methods.
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.
Thank you, I've removed it.
…object. Added disconnect for __destruct alternative to disconnect from db, related to bug #40681 and requests #62065 and #67473. Added PHP::isConnected to yield current connection status. Fixed bug #63343 in rework of PDO pdo_dbh_free_storage logic to avoid transaction rollback against persistent db connection. Added pdo_is_persisted and pdo_list_entry_from_key helpers to determine when persistent db handler is actively registered as resource, avoiding opaque use of _pdo_dbh_t::refcount to this end. Updated php_pdo_internal_construct_driver to consume added list entry helper and to check _pdo_dbh_t::is_closed in addition to liveness when validating a discovered persistent db connection for re-use. Removed extraneous call to zend_list_close given that only a persistent destructor is defined for the _pdo_dbh_t resource that zend_list_close does not invoke. Fixed potential for bad memory access in persistent connections from former PDO objects, once the connection has been replaced due to liveness check failure. The resource was freed without invalidating other PDO object references, but now the resource will not be freed until the last referring PDO object is freed. Made _pdo_dbh_t::refcount accurate and consistent, regardless of persistent connection, to represent count of PDO object's referencing the handle. In the case of persistent connections, the count was +1, and in the case of non-persistent connections, never decremented.
After working through several test failures, I am left with 1 failure in another area that I am not sure is on my end, or if it is, could use some guidance as to what I've broken here, or where to look.
In the build and test output, I notice no Oracle client is found and the pdo extension is not built, though perhaps that is normal for Windows and PHP 8.3, considering the extension was removed.
|
First of all, this definitely requires RFC and it can only go into PHP 8.6, not PHP 8.3. I am categorically against adding disconnect method to PDO. The way PDO was designed was to never allow the object to be in a disconnected state. Yes, it is possible for the constructor to be never called and PDO has a check for that but this is a rather exceptional situation that could not have been avoided otherwise. You could only ever arrive at this error if you override the constructor or use reflection API. By introducing the method that closes a connection you are introducing another potential foot gun. Closing the connection puts the PDO object in an "inconsistent" broken state. This is a problem well known from the MYSQLI API. It's one of the major design problems that MYSQLI is suffering from. PDO avoided it from the start by not allowing the user to ever put the object in such a state. I don't want to sound presumptuous but people who are petitioning in the bug reports for PDO to have this method mistakenly think that the disconnect method is the solution to their problems. There are plenty of applications which use PDO without requiring such a method and they work without any problems. We have gone on for so many years without this method, and I see no reason why we should cave in and add it now. The topic of persistent connection is a topic of its own. Users who decide to use the infamous persistent connection feature agree to live with the problems it causes. Having said that, I was not aware of the issue of reusing the same persistent connection in the same request without releasing the first one. This seems like a bug to me.
That's wrong in my opinion. The transaction must be rolled back when a PDO object is a freed to prevent transaction inconsistencies. Now, having just learned about the issue above, I am not sure if what you are proposing is better or worse, but I would be leaning towards going the other way and patching the possibility of reusing the same persistent connection twice. With regards to closing a persistent connection, just as the name suggests the connection should persist after the request ends. While it obviously applies to automatic closing, it also implies that users should not be allowed to manually close it. I don't know if there are legitimate situations when a user would like to explicitly close a persistent connection, but in my opinion it points at an XY problem. |
The principal target of this PR is to introduce
PDO::disconnect()
to disconnect from the database as an alternative to PDO's destructor, which feature is related to bug #40681 and requests #62065 and #67473. Further and related efforts have been made herein to modify the destruct behavior of persistent PDO db connections. The rollback invocation questioned with bug #63343 is postponed until time of disconnect as opposed to PDO object destruct, given the potential to affect other PDO instances. Additionally, PDO object integrity is maintained w.r.t. it's inner db handle in the case that a liveness check fails upon other PDO construct and a new persistent resource is registered.Current recommendations in PDO application integration avoid the need for an explicit disconnect routine and workaround the above described issues through prescribed use of a single PDO object instance for any given database connection, and which database connection is terminated upon either PDO object destruct or, in the case of persistent connections, at the end of script execution. While the use of a singleton object for database connection handling within applications is a common implementation pattern, the tight integration of database disconnect behavior with PHP's object free hook has imposed a need to further manage PDO object references within an application in order to reliably replace and/or shutdown a db connection instance, an effort which scales with application complexity. This reality is further complicated by the PDO statement object, which also holds a reference to a PDO object and thus must also be managed from a reference standpoint, as well as by PHP itself, which governs the time at which an object free hook is invoked, subject to influence by bug or design. It is also worth noting that in the current form, it is not possible to use the PDO object to determine the state of database connection closure, whether normal or errant.
The proposed changes add application integration options without majorly modifying current behavior, but the following behavior changes are present.
PDO::disconnect()
, subsequent invocations against the same db handle by any surviving PDO objects will throw a novelPDOException
with the message "connection is closed"._pdo_object_t::is_closed
struct member is put to use, presently not referenced within the source. Other driver implementations which have used the struct member in unintended ways may suffer._pdo_object_t::refcount
struct member accurately reflects a count of PDO objects referring to it, regardless of persistent connection state.PDO::errorInfo()
andPDO::errorCode()
may now be called on an uninitialized PDO object, i.e. one without a defineddriver
member. This change is being done to permit use of both functions afterPDO::disconnect()
is called, and as a result of modifying thePDO_CONSTRUCT_CHECK
macro to impose check onis_closed
.I trust you will inform me as this submission has departed from the contribution guidelines or coding standards in any way that I may have missed, and welcome your feedback.