-
Notifications
You must be signed in to change notification settings - Fork 5
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
PHP 8.1 compatibility #2
Comments
Hi! I was getting acquainted with your package and found what I believe to be the root cause (incredible job, by the way! I'm loving it!). What I found is that some PHP classes extend from WordPress classes 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. In my case running integration tests was returning errors because of that. All the problems I found was in the WP SQLite DB package. Manually editing the db.php file and adding some #[\ReturnTypeWillChange] annotation solved the problem (for now). I’ll send a pull request asking for solving that there in the afternoon. So the solution, for now, is to solve those signature mismatches. For example: /**
* This class extends PDO class and does the real work.
*
* It accepts a request from wpdb class, initialize PDO instance,
* execute SQL statement, and returns the results to WordPress.
*/
class PDOEngine extends PDO {
…
/**
* Method to call PDO::rollBack().
*
* @see PDO::rollBack()
*/
public function rollBack() {
$this->pdo->rollBack();
$this->has_active_transaction = false;
} Since the overwritten method PDO::rollBack() returns a boolean, the two possible solutions are: #[\ReturnTypeWillChange]
public function rollBack() {
$this->pdo->rollBack();
$this->has_active_transaction = false;
} or public function rollBack() {
$value = $this->pdo->rollBack();
$this->has_active_transaction = false;
return $value
} Both ways make it compliant but only returning the right value will make it ready for the next PHP 9. I hope I’ve helped in any way! Best regards and thank you for sharing this plugin! |
Thanks for investigating this issue! I think the best course of action would be to create an issue in the https://github.com/aaemnnosttv/wp-sqlite-db repo which is responsible for the in-memory DB integration (I can do this part). If it's fixed upstream it will help this package automatically. I still haven't tested the integration tests on PHP 8.1, as I did get some deprecation notices in the core when testing it locally. Nothing that is critical, and it should be working, but still, better to fully test it 😄 I do plan on releasing v2 next year, which will require PHPUnit 10 and pest v2, and will be a BC break because both packages will require PHP 8.1 IIRC. But that will happen next year 🙂 I do need to think about long-term support for v1 of my package (which supports now the EOL PHP 7.4 version). |
Hi, Denis. I just had my PR merged. As soon as WP SQLite DB publishes a new release update this PHP 8.1 compatibility issue should be solved. Merry Christmas! |
Awesome! Thanks for helping out 🙂 Merry Christmas to you too 🎄 |
Alpha version of v2 has been released: https://github.com/dingo-d/wp-pest/releases/tag/2.0.0-alpha |
Describe your feature request
Check if the package works with PHP 8.1, currently the tests are failing with some weird patchwork issue.
Describe the solution you'd like
Make the test pass, and ensure the package works with PHP 8.1
Please confirm that you have searched existing issues and discussions about this feature.
Yes
The text was updated successfully, but these errors were encountered: