fix: handle deprecation notices in PHP 8.1 #50
Conversation
There was a problem hiding this comment.
Thanks for the PR! Unfortunately, the addition of return types won't work here without breaking compatibility with PHP 5.6. WP has solved this in other places by having alternate classes that are loaded based on the PHP version, but that won't work for our single file drop-in here because the whole file needs to be syntactically valid for the runtime.
src/db.php
Outdated
| */ | ||
| public function beginTransaction() | ||
| public function beginTransaction(): bool |
There was a problem hiding this comment.
Unfortunately, we can't use return types due to compatibility with PHP 5.6.
There was a problem hiding this comment.
Absolutely right. The solution will be to use the ReturnTypeWillChange annotation on all those methods. Jut sent a new update. Thanks for your reply!
| } | ||
|
|
||
| $this->has_active_transaction = $this->pdo->beginTransaction(); | ||
|
|
||
| return $this->has_active_transaction; |
There was a problem hiding this comment.
This part seems a bit unrelated but is an improvement 😄
…ard compatibility up to php5.6
There was a problem hiding this comment.
Thanks @mwguerra – tests are all passing now, just one last thing.
| @@ -1319,9 +1319,9 @@ private function prepare_directory() | |||
| * @param int $mode | |||
| * @param array $fetch_mode_args | |||
| * | |||
| * @return mixed according to the query type | |||
There was a problem hiding this comment.
Let's keep the @return doc block annotations since we aren't able to replace them with return types 👍
| @@ -2425,39 +2425,45 @@ private function get_sqlite_version() | |||
| * Method to call PDO::beginTransaction(). | |||
| * | |||
| * @see PDO::beginTransaction() | |||
| * @return boolean | |||
Hi, everyone! This pull request solves deprecation notices and untreated errors, new after upgrading to PHP 8.1.
The PDOEngine class extends from the PHP PDO class, but with no indication of the return code or a signature mismatch in some methods. Starting in PHP 8.1 not being explicit on the return types (or returning anything different from the parent method) leads to a deprecation notice and untreated errors.
There were four methods to adjust. Three of them were easy fixes/enhancements.
The fourth method is query. The way query was implemented, changed a lot the original PDO query response behavior. It gets the job done but also introduces new challenges to make the code compliant with the original signature/return type, leading to a deprecation warning.
The original PDO query always returns a PDOStatement object, or false on any failure. The current PDOEngine, as implemented, could return (from my initial analysis) undefined, true, false, PDOStatement objects and even integers.
For this reason, I believe it’s a fair decision to use the #[\ReturnTypeWillChange] annotation in this case. In the near future, I suggest someone should review and refactor the codebase. There will be some breaking changes, but I believe it's an important step towards PHP 8.3 (that I believe should be launched by the end of next year) and even a future PHP 9, where I also believe the #[\ReturnTypeWillChange]
will be retired.
After those changes, all tests passed.
Solves #27
Solves #49
I hope my analysis and this PR help the project in any way.
Best regards and Merry Christmas!