dingo-d / wp-pest

A package that will add WordPress integration test suite with Pest
MIT License
119 stars 5 forks source link

PHP 8.1 compatibility #2

Open dingo-d opened 2 years ago

dingo-d commented 2 years ago

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

mwguerra commented 1 year ago

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!

dingo-d commented 1 year ago

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).

mwguerra commented 1 year ago

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!

dingo-d commented 1 year ago

Awesome! Thanks for helping out 🙂

Merry Christmas to you too 🎄

dingo-d commented 11 months ago

Alpha version of v2 has been released: https://github.com/dingo-d/wp-pest/releases/tag/2.0.0-alpha