[PHP 8.1] Fix code that throws deprecation notices in tests in PHP 8.1 #31333

Konamiman · 2021-11-30T15:25:07Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Do the necessary code fixes so that the WooCommerce unit tests don't throw deprecation notices in PHP 8.1. The deprecation notices fixed are:

Various "passing null to parameter (...) is deprecated".
Usage of strftime.
Using false as if it was an empty array.
Mismatching return type of implemented interfaces, that's fixed by adding #[\ReturnTypeWillChange] attributes.

Note that this pull request alone is not enough to fix all the tests:

Blocks and Action Scheduler will need fixes too (pull request links in the testing instructions).
WordPress core itself isn't currently compatible with PHP 8.1. Some tests are skipped temporarily (see skip_on_php_8_1 method introduced in class-wc-unit-test-case.php), only in PHP 8.1, until that's no longer the case.

Once Blocks and Action Scheduler have got the required fixes we could extend the GitHub action that run the tests to include PHP 8.1, keeping the WordPress-offending ones disabled for the time being.

Closes: #31270
Closes: #31386

NOTE: A lot of code formatting changes were needed for the code sniffer to be happy. These are in a separate commit for easier review.

How to test the changes in this Pull Request:

Ensure that the fixes required for Blocks and Action Scheduler are applied (PR for blocks - PR for Action Scheduler).
Run the unit tests in PHP 7.
Perform the changes needed to run tests in PHP 8.
Run the unit tests in both PHP 8.0 and PHP 8.1.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?

Changelog entry

Fix - Code that produces deprecation notices when running unit tests in PHP 8.1.

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

peterfabian

Apologies for the monstrous delay! Thanks, overall it's looking pretty good! Great work! I have a couple of questions/suggestions in the comments below.

peterfabian · 2022-04-07T11:41:43Z

plugins/woocommerce/includes/data-stores/class-wc-product-data-store-cpt.php

@@ -368,7 +368,7 @@ protected function read_product_data( &$product ) {
 		$set_props['category_ids']      = $this->get_term_ids( $product, 'product_cat' );
 		$set_props['tag_ids']           = $this->get_term_ids( $product, 'product_tag' );
 		$set_props['shipping_class_id'] = current( $this->get_term_ids( $product, 'product_shipping_class' ) );
-		$set_props['gallery_image_ids'] = array_filter( explode( ',', $set_props['gallery_image_ids'] ) );
+		$set_props['gallery_image_ids'] = array_filter( explode( ',', $set_props['gallery_image_ids'] ?? '' ) );


Since we're fixing this, I think there are probably more places in code where the second argument may be null, e.g. maybe over here: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/includes/class-wc-tax.php#L51
or here
https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/includes/wc-core-functions.php#L984

Do you think we should fix them proactively or ad-hoc?

I suppose the situation is similar with a bunch of other functions like str_replace, class_exists or urldecode...

At the current point, with WordPress itself not being yet compatible with PHP 8.1, my bet is to keep a "reactive" approach and fix things as they appear. This also gives more time to the static analysis tools to become better.

peterfabian · 2022-04-07T11:42:55Z

plugins/woocommerce/includes/data-stores/class-wc-product-variable-data-store-cpt.php

@@ -119,6 +119,9 @@ protected function read_product_data( &$product ) {
 	public function read_children( &$product, $force_read = false ) {
 		$children_transient_name = 'wc_product_children_' . $product->get_id();
 		$children                = get_transient( $children_transient_name );
+		if(false === $children) {


Suggested change

if(false === $children) {

if ( false === $children ) {

plugins/woocommerce/tests/legacy/bootstrap.php

plugins/woocommerce/tests/php/includes/class-wc-ajax-test.php

peterfabian · 2022-04-08T12:38:37Z

Side note: it might be worth rebasing this to make testing a bit easier, because only god knows how to build this now, I used composer install, not the new pnpm stuff, as it was giving me npm/node errors, plus it should fix the Blocks error without needing for manual patching.

The deprecation notices are about: - Various "passing null to parameter (...) is deprecated" - Usage of strftime - Using "false" as if it was an empty array - Mismatching return type of implemented interfaces, that's fixed by adding #[\ReturnTypeWillChange]

masteradhoc · 2022-05-10T13:07:13Z

any update on this @Konamiman. Would solve a lot php notices we got ;)

botwoo · 2022-06-10T08:15:35Z

📊 Test reports for this pull request have been published and are accessible through the following links:

REST API Test Report
E2E Test Report (TBC)

Latest commit referenced in the reports: Formatting. 0bc38d4
This comment will automatically be updated with the latest referenced commit when you push new changes to this pull request.

Visit the WooCommerce Test Reports homepage to view all published reports. See the FAQs page if you're having problems accessing them.

peterfabian

This has been a bit more involved than I hoped.

Testing this in PHP 8.1 now with WP 6.0, I got a bunch of errors. After cleaning up some WC Admin code and some code of ours and applying a bunch of fixes to WP core while testing (adding several #[\ReturnTypeWillChange] statements to wp-includes/Requests/Cookie/Jar.php (related issue) and wp-includes/Requests/Utility/CaseInsensitiveDictionary.php), I'm getting just 2 errors and 1 failure:

There were 2 errors:

1) WC_Admin_Tests_API_Reports_Customers::test_user_creation
PHPUnit\Framework\Exception: PHP Deprecated:  trim(): Passing null to parameter #1 ($string) of type string is deprecated in /private/var/folders/rh/s6k2nr3s6qnc7bt1zz0glxwr0000gn/T/wordpress/wp-includes/pluggable.php on line 2481

2) WC_Admin_Tests_API_Reports_Customers::test_customer_user_profile_name
PHPUnit\Framework\Exception: PHP Deprecated:  trim(): Passing null to parameter #1 ($string) of type string is deprecated in /private/var/folders/rh/s6k2nr3s6qnc7bt1zz0glxwr0000gn/T/wordpress/wp-includes/pluggable.php on line 2481

--

There was 1 failure:

1) WC_Tests_Product_CSV_Importer::test_server_file
WP_Error Object (...) does not match expected type "string".

/Users/peter/Documents/GitHub/woocommerce/plugins/woocommerce/tests/legacy/unit-tests/importer/product.php:180

peterfabian · 2022-06-20T08:22:27Z

Just retested with PHP 8.0 and got just one failure:

There was 1 failure:

1) WC_Tests_Product_CSV_Importer::test_server_file
WP_Error Object (...) does not match expected type "string".

/Users/peter/Documents/GitHub/woocommerce/plugins/woocommerce/tests/legacy/unit-tests/importer/product.php:180

--

So I think after this one failure gets fixed and the 2 PRs above are merged, this should be good to go.

Konamiman · 2022-06-20T10:34:02Z

WC_Tests_Product_CSV_Importer::test_server_file
WP_Error Object (...) does not match expected type "string".

I'm not getting this error, neither in trunk nor in the branch of this pull request, when testing in PHP 8.0. Maybe the error is caused by one of the other two pull requests?

peterfabian · 2022-06-21T08:12:28Z

So I retested this on a clean repo clone again to make sure my attempt to rebase won't interfere. I see no deprecation notices from PHP 8 or 8.1 anymore, so we can ignore those 2 PRs I opened, but the 1 failure is still there:

PHP 8.0

PHPUnit 7.5.20.PHP-8-dev by Sebastian Bergmann and contributors.

Runtime:       PHP 8.0.20
Configuration: /Users/peter/Documents/GitHub/woocommerce/plugins/woocommerce/phpunit.xml
...
Time: 12.5 minutes, Memory: 201.50 MB

There was 1 failure:

1) WC_Tests_Product_CSV_Importer::test_server_file
WP_Error Object (...) does not match expected type "string".

/Users/peter/Documents/GitHub/woocommerce/plugins/woocommerce/tests/legacy/unit-tests/importer/product.php:180

--

PHP 8.1

PHPUnit 7.5.20.PHP-8-dev by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.7
Configuration: /Users/peter/Documents/GitHub/woocommerce/plugins/woocommerce/phpunit.xml
...

Time: 11.06 minutes, Memory: 199.00 MB

There was 1 failure:

1) WC_Tests_Product_CSV_Importer::test_server_file
WP_Error Object (...) does not match expected type "string".

/Users/peter/Documents/GitHub/woocommerce/plugins/woocommerce/tests/legacy/unit-tests/importer/product.php:180

--

Will try to reinstall the testing suite to make sure my changes to WP aren't causing this, but I only added a couple of #[\ReturnTypeWillChange] flags, so that shouldn't cause a test error I think...

peterfabian · 2022-06-21T09:01:24Z

So the test that fails here will most likely fail also in PHP 7. It's not really related to the PHP 8 change, I think it was introduced as part of the changes in the downloadable files code (over here).

Either way, maybe your setup returns different paths for this test. For me, test_server_file results in failure because realpath beginning is different from ABSPATH:

file exist: 1
realpath: /private/var/folders/rh/s6k2nr3s6qnc7bt1zz0glxwr0000gn/T/wordpress/sample.csv
stripos:
abspath: /var/folders/rh/s6k2nr3s6qnc7bt1zz0glxwr0000gn/T/wordpress//

thus the check over here fails.

peterfabian

After a re-test, this works fine, so approving. Thanks!

peterfabian

👍

github-actions · 2022-06-21T10:17:06Z

Hi @peterfabian, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add testing instructions label

Konamiman self-assigned this Nov 30, 2021

Konamiman changed the title ~~[PHP 8.1] Fix code that thtows deprecation notices in tests in php 8 1~~ [PHP 8.1] Fix code that throws deprecation notices in tests in php 8 1 Nov 30, 2021

Konamiman changed the title ~~[PHP 8.1] Fix code that throws deprecation notices in tests in php 8 1~~ [PHP 8.1] Fix code that throws deprecation notices in tests in PHP 8.1 Dec 1, 2021

This was referenced Dec 1, 2021

[PHP 8.1] Fix code that causes a deprecation notice in BlockTypesController woocommerce/woocommerce-blocks#5287

Merged

[PHP 8.1] Fix code that causes deprecation notices woocommerce/action-scheduler#791

Merged

Konamiman marked this pull request as ready for review December 2, 2021 08:22

Konamiman requested review from a team and peterfabian and removed request for a team December 10, 2021 15:33

om4james mentioned this pull request Jan 19, 2022

WooCommerce core and tests not compatible with PHP 8.1 #31270

Closed

5 tasks

ObliviousHarmony added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 21, 2022

peterfabian reviewed Apr 8, 2022

View reviewed changes

Konamiman added 4 commits April 13, 2022 09:33

Fix phpcs errors and warnings

0b3e699

Fix some more code that throw deprecation notices in PHP 8.1

0aa0073

Small commenting/formatting fixes.

de5f2e3

Konamiman force-pushed the fix/deprecation_notices_in_tests_in_php_8_1 branch from 0bc7bd4 to de5f2e3 Compare April 13, 2022 07:40

Konamiman requested a review from peterfabian April 21, 2022 09:59

Konamiman added 2 commits June 10, 2022 09:41

Merge branch 'trunk' into fix/deprecation_notices_in_tests_in_php_8_1

48b6c8e

Add changelog file

f3661b6

peterfabian reviewed Jun 17, 2022

View reviewed changes

This was referenced Jun 20, 2022

Remove deprecated warnings in unit tests in PHP 8.1 for core #33501

Closed

Remove deprecated warnings in unit tests in PHP 8.1 #33500

Closed

peterfabian previously approved these changes Jun 21, 2022

View reviewed changes

Formatting.

0bc38d4

peterfabian dismissed their stale review via 0bc38d4 June 21, 2022 10:04

peterfabian approved these changes Jun 21, 2022

View reviewed changes

peterfabian merged commit 3b089f9 into trunk Jun 21, 2022

peterfabian deleted the fix/deprecation_notices_in_tests_in_php_8_1 branch June 21, 2022 10:16

github-actions bot added this to the 6.8.0 milestone Jun 21, 2022

jorgeatorres mentioned this pull request Feb 28, 2023

PHP 8.1 Compatibility Deprecated code #33808

Closed

5 tasks

rrennick mentioned this pull request Jun 6, 2023

Deprecated use of strlen with null parameter #32361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PHP 8.1] Fix code that throws deprecation notices in tests in PHP 8.1 #31333

[PHP 8.1] Fix code that throws deprecation notices in tests in PHP 8.1 #31333

Konamiman commented Nov 30, 2021 •

edited

peterfabian left a comment •

edited

peterfabian Apr 7, 2022

Konamiman Apr 13, 2022 •

edited

peterfabian Apr 7, 2022

peterfabian commented Apr 8, 2022 •

edited

masteradhoc commented May 10, 2022

botwoo commented Jun 10, 2022 •

edited

peterfabian left a comment •

edited

peterfabian commented Jun 20, 2022

Konamiman commented Jun 20, 2022

peterfabian commented Jun 21, 2022 •

edited

peterfabian commented Jun 21, 2022 •

edited

peterfabian left a comment

peterfabian left a comment

github-actions bot commented Jun 21, 2022

[PHP 8.1] Fix code that throws deprecation notices in tests in PHP 8.1 #31333

[PHP 8.1] Fix code that throws deprecation notices in tests in PHP 8.1 #31333

Conversation

Konamiman commented Nov 30, 2021 • edited

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Changelog entry

FOR PR REVIEWER ONLY:

peterfabian left a comment • edited

Choose a reason for hiding this comment

peterfabian Apr 7, 2022

Choose a reason for hiding this comment

Konamiman Apr 13, 2022 • edited

Choose a reason for hiding this comment

peterfabian Apr 7, 2022

Choose a reason for hiding this comment

peterfabian commented Apr 8, 2022 • edited

masteradhoc commented May 10, 2022

botwoo commented Jun 10, 2022 • edited

peterfabian left a comment • edited

Choose a reason for hiding this comment

peterfabian commented Jun 20, 2022

Konamiman commented Jun 20, 2022

peterfabian commented Jun 21, 2022 • edited

PHP 8.0

PHP 8.1

peterfabian commented Jun 21, 2022 • edited

peterfabian left a comment

Choose a reason for hiding this comment

peterfabian left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 21, 2022

Konamiman commented Nov 30, 2021 •

edited

peterfabian left a comment •

edited

Konamiman Apr 13, 2022 •

edited

peterfabian commented Apr 8, 2022 •

edited

botwoo commented Jun 10, 2022 •

edited

peterfabian left a comment •

edited

peterfabian commented Jun 21, 2022 •

edited

peterfabian commented Jun 21, 2022 •

edited