Updates PHP unit tests matrix and dev dependencies for PHP8.2 compatibility.

[krutidugade]

Changes proposed in this Pull Request:

As part of PHP 8.1+ compatibility testing, we are updating unit test tools to the newer version of PHP. This PR updates the php-unit-tests.yml to run unit tests for PHP 7.4, 8.0, 8.1, and 8.2. We have bumped the PHP requirement to 7.4 and phpunit to 8.5. Also, we have updated yoast/phpunit-polyfills dependency to 2.0.

You get a bunch of deprecated warnings one running unit tests with PHP8.2 which can be viewed here under Run PHP Unit Tests. This PR eliminates those warnings by introducing the usage of complex syntax for variable $rate_limit_id.

PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /home/runner/work/facebook-for-woocommerce/facebook-for-woocommerce/includes/API/Traits/Rate_Limited_API.php on line 38
Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /home/runner/work/facebook-for-woocommerce/facebook-for-woocommerce/includes/API/Traits/Rate_Limited_API.php on line 36
PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /home/runner/work/facebook-for-woocommerce/facebook-for-woocommerce/includes/API/Traits/Rate_Limited_API.php on line 52

• Do the changed files pass phpcs checks? Please remove phpcs:ignore comments in changed files and fix any issues, or delete if not practical.

Detailed test instructions:

• Verify all the git workflow unit test checks pass.
• Expand PHP unit tests - PHP 8.2, WP latest check
• Verify you don't see any deprecation warnings indicating use of complex syntax {$var}

Changelog entry

Dev - Updates PHP unit tests matrix in git workflow and versions of dev dependencies in composer.

[ecgan]

The changes here look okay. I have one comment about the WP version being set to 5.9, which I have marked with ❓ .

I did some testing and code search on this PR. Here are my findings. Important ones are marked with ❓ . I think we need to make a few more changes in this PR.

npm run lint:php and npm run lint:php:summary

With PHP 7.4, I see 0 errors and 3 warnings.

With PHP 8.2, I see there are 98 errors and 0 warnings, and they are mostly about "The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated".

A reference on how to fix the error: Migration to PHP 8.1 - how to fix Deprecated Passing null to parameter error - rename build in functions

📜 I think we can fix this is a separate PR, and also put this linting into GitHub Actions, which we do not have currently.

composer test-unit

I also tried to run the unit test locally, but apparently it is not that straightforward as it requires some installation step with ./bin/install-wp-tests.sh with database access. It makes me wonder why unit test requires database access. 🤔 I think it would be good to have more documentation on this, and also add an npm script so that we can have all the scripts listed in package.json file.

npm run i18n

PHP 7.4: OK.
PHP 8.2: OK.

npm run generate:category_attribute_json

PHP 7.4: OK.
PHP 8.2: OK.

npm run archive

PHP 7.4: OK.
PHP 8.2: OK.

PHP version find

I did some PHP version search in the code base.

"7.2"

I think this line can be removed, because the WooCommerce core plugin already has PHP version specified and our extension should just follow it (related discussion: pb0Spc-4fD-p2#comment-8238):

https://github.com/woocommerce/facebook-for-woocommerce/blob/0f6298cbc6a38fdeaa4a5eb5252166dc4cbd2ce1/facebook-for-woocommerce.php#L20

The following variable and its usage should be removed too, this would probably require a bit more testing, so we can do it in a separate PR:

https://github.com/woocommerce/facebook-for-woocommerce/blob/0f6298cbc6a38fdeaa4a5eb5252166dc4cbd2ce1/facebook-for-woocommerce.php#L50-L51

❓ The following should be updated to "7.4" (reference: https://github.com/PHPCompatibility/PHPCompatibility#sniffing-your-code-for-compatibility-with-specific-php-versions):

https://github.com/woocommerce/facebook-for-woocommerce/blob/0f6298cbc6a38fdeaa4a5eb5252166dc4cbd2ce1/phpcs.xml.dist#L7

❓ In the following PHP coding standards configuration, we are using PHP 7.2, and this can be seen in the GitHub Action run too. It should be updated to "7.4" (see https://github.com/StephaneBour/actions-php-lint); I wonder if we can put 7.4 and 8.2 into a matrix configuration, perhaps in a separate PR 🤔 :

https://github.com/woocommerce/facebook-for-woocommerce/blob/0f6298cbc6a38fdeaa4a5eb5252166dc4cbd2ce1/.github/workflows/php-coding-standards.yml#L40

[ecgan]

Adding one more comment about PHPUnit 8.5.

[krutidugade]

I did some PHP version search in the code base.
"7.2"
Requires PHP: 7.2
const MINIMUM_PHP_VERSION = '7.2.0';

Thank you for the suggestion. I have made the changes in this commit. I'll be working through the rest of your feedback shortly.

[krutidugade]

The following should be updated to "7.4" (reference: https://github.com/PHPCompatibility/PHPCompatibility#sniffing-your-code-for-compatibility-with-specific-php-versions):

✅ I have updated this to 7.4-.

With PHP 8.2, I see there are 98 errors and 0 warnings, and they are mostly about "The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated".
A reference on how to fix the error: Migration to PHP 8.1 - how to fix Deprecated Passing null to parameter error - rename build in functions
📜 I think we can fix this is a separate PR, and also put this linting into GitHub Actions, which we do not have currently.

I did some digging on this. We get a bunch of errors but they are just repetitions of the following two errors.

An error occurred during processing; checking has been aborted. The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated in /abc/wp-content/plugins/facebook-for-woocommerce/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php on line 280 (Internal.Exception)

An error occurred during processing; checking has been aborted. The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated in /abc/wp-content/plugins/facebook-for-woocommerce/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/WP/I18nSniff.php on line 194 (Internal.Exception)

These errors come from wp-coding-standards/wpcs. I tried updating wp-coding-standards/wpcs dev dependency to 3.0.0 hoping it will fix these errors given that the code is refactored in the latest release. However, we can't update it as "wp-coding-standards/wpcs": "^2.3" is a requirement for "woocommerce/woocommerce-sniffs": "^0.1.3". It's been more than a year since woocommerce-sniffs was updated. I'm wondering what should we do next. Should we figure out a way to display a notice to add ?? '' at specific locations for PHP versions >=8.1 to be able to run unit tests locally? @ecgan

In the following PHP coding standards configuration, we are using PHP 7.2, and this can be seen in the GitHub Action run too. It should be updated to "7.4" (see https://github.com/StephaneBour/actions-php-lint); I wonder if we can put 7.4 and 8.2 into a matrix configuration, perhaps in a separate PR 🤔 :

Because of the reason mentioned above, I have updated it to 8.0 StephaneBour/actions-php-lint@8.0.

[krutidugade]

I also tried to run the unit test locally, but apparently it is not that straightforward as it requires some installation step with ./bin/install-wp-tests.sh with database access. It makes me wonder why unit test requires database access. 🤔 I think it would be good to have more documentation on this, and also add an npm script so that we can have all the scripts listed in package.json file.

That's a good suggestion. I have added the script in package.json and updated README.md file in 64d88cf commit.

It makes me wonder why unit test requires database access.

To answer this, we set up a test environment to run these tests. So we install WP, WC, test suite, and test database separately when we run the install-wp-tests.sh script file.

[ecgan]

It makes me wonder why unit test requires database access.

To answer this, we set up a test environment to run these tests. So we install WP, WC, test suite, and test database separately when we run the install-wp-tests.sh script file.

Thanks for the link, TIL! 😄

What I meant to say is that if it requires database access, then it is more of an integration test than a unit test. Even the title of the page that you linked to is called "Plugin Integration Tests". 😂 But anyway, I'll just ignore the naming of it now, don't want to get too caught up by it.

[krutidugade]

What I meant to say is that if it requires database access, then it is more of an integration test than a unit test. Even the title of the page that you linked to is called "Plugin Integration Tests". 😂 But anyway, I'll just ignore the naming of it now, don't want to get too caught up by it.

That makes sense. I'm still learning this whole testing thing so it is interesting to read about this bit. Thanks for highlighting it.

[ecgan]

These errors come from wp-coding-standards/wpcs. I tried updating wp-coding-standards/wpcs dev dependency to 3.0.0 hoping it will fix these errors given that the code is refactored in the latest release. However, we can't update it as "wp-coding-standards/wpcs": "^2.3" is a requirement for "woocommerce/woocommerce-sniffs": "^0.1.3". It's been more than a year since woocommerce-sniffs was updated. I'm wondering what should we do next. Should we figure out a way to display a notice to add ?? '' at specific locations for PHP versions >=8.1 to be able to run unit tests locally? @ecgan

I did a some more testings on this. At first, I thought it is a problem with woocommerce/woocommerce-sniffs, so I uninstalled the package, remove the related PHPCS configuration and rerun PHPCS in PHP 8.2, but I still see the same problem. Then, I updated wp-coding-standards/wpcs to version 3.0, and it works with no errors.

I did some googling on the error message and I found this issue WordPress/WordPress-Coding-Standards#2035. In short, "wp-coding-standards/wpcs": "^2.3" only works with PHP 7.4 and does not work with PHP 8.0+. To make it work with PHP 8.0+, we need to use "wp-coding-standards/wpcs": "^3.0".

There is also an alternative, which is to put in the following into phpcs.xml configuration file, to suppress reporting on deprecated error:

<!--
Prevent errors caused by WordPress Coding Standards not supporting PHP 8.0+.
See https://github.com/WordPress/WordPress-Coding-Standards/issues/2035
-->
<ini name="error_reporting" value="E_ALL &#38; ~E_DEPRECATED" />

I tested it and it seems to work as expected, but I'm not sure if it will give us trouble down the road, e.g. suppressing deprecated error in our own plugin code, not just in WordPress Coding Standards.

So, I think what we should do is that when we run phpcs / npm run lint:php, we should only use PHP 7.4 (we should mention this in the documentation). When WooCommerce core moves to using minimum PHP version 8.0+, hopefully by then woocommerce-sniffs will be updated to work with WPCS 3.0, and then we shall update the packages in our plugins to use the newer woocommerce-sniffs and WPCS.

Side note: I also look into the PHP Coding Standards GitHub Action that runs PHPCS. The PHP version used in the GitHub Action depends on the woocommerce/grow/prepare-php@actions-v1:

https://github.com/woocommerce/facebook-for-woocommerce/blob/0f6298cbc6a38fdeaa4a5eb5252166dc4cbd2ce1/.github/workflows/php-coding-standards.yml#L28-L29

... which currently has a default value of "7.4" (see https://github.com/woocommerce/grow/blob/9ea43833593986daeb1edbaa54404acacb62527b/packages/github-actions/actions/prepare-php/action.yml#L8). That's why it runs okay with no errors.

[ecgan]

In the following PHP coding standards configuration, we are using PHP 7.2, and this can be seen in the GitHub Action run too. It should be updated to "7.4" (see StephaneBour/actions-php-lint); I wonder if we can put 7.4 and 8.2 into a matrix configuration, perhaps in a separate PR 🤔 :

Because of the reason mentioned above, I have updated it to 8.0 StephaneBour/actions-php-lint@8.0.

Sorry, I don't quite get it, but what do you mean by "the reason mentioned above"? Why do we use 8.0? 🤔

[krutidugade]

Then, I updated wp-coding-standards/wpcs to version 3.0, and it works with no errors.

Amazing troubleshooting, well done @ecgan . You confirmed my assumption that the errors won't appear with wp-coding-standards/wpcs 3.0.

In short, "wp-coding-standards/wpcs": "^2.3" only works with PHP 7.4 and does not work with PHP 8.0+
Sorry, I don't quite get it, but what do you mean by "the reason mentioned above"? Why do we use 8.0? 🤔

I beg to differ. I have tested it with PHP version 8.0 and I don't get the errors. This can be confirmed in this PHPCS job where PHP8.0 is installed and the check has passed. This is why I have used StephaneBour/actions-php-lint@8.0 and explicitly mentioned PHP version 8.0. We get the errors for PHP8.1+ versions.

There is also an alternative, which is to put in the following into phpcs.xml configuration file, to suppress reporting on deprecated error:
I tested it and it seems to work as expected, but I'm not sure if it will give us trouble down the road, e.g. suppressing deprecated error in our own plugin code, not just in WordPress Coding Standards.

I'm not in favor of suppressing warnings. We may end up not fixing these warnings for a long time so I would rather have these visible until they are fixed.

So, I think what we should do is that when we run phpcs / npm run lint:php, we should only use PHP 7.4 (we should mention this in the documentation). When WooCommerce core moves to using minimum PHP version 8.0+, hopefully by then woocommerce-sniffs will be updated to work with WPCS 3.0, and then we shall update the packages in our plugins to use the newer woocommerce-sniffs and WPCS.

Yes, I'm on board with this idea. Since I have tested with 8.0, I'm happy to add a note suggesting the use of PHP8.0 for PHPCS linting.

Side note: I also look into the PHP Coding Standards GitHub Action that runs PHPCS. The PHP version used in the GitHub Action depends on the woocommerce/grow/prepare-php@actions-v1:

It says description: Specify PHP version. "7.4" by default.. So if we don't specify the PHP version in our YAML file then it will pick PHP7.4. Since we have mentioned 8.0, our job runs with PHP8.0.

[krutidugade]

So, I think what we should do is that when we run phpcs / npm run lint:php, we should only use PHP 7.4 (we should mention this in the documentation). When WooCommerce core moves to using minimum PHP version 8.0+, hopefully by then woocommerce-sniffs will be updated to work with WPCS 3.0, and then we shall update the packages in our plugins to use the newer woocommerce-sniffs and WPCS.

I have made these changes in 0800642 commit.

[ecgan]

In short, "wp-coding-standards/wpcs": "^2.3" only works with PHP 7.4 and does not work with PHP 8.0+
Sorry, I don't quite get it, but what do you mean by "the reason mentioned above"? Why do we use 8.0? 🤔

I beg to differ. I have tested it with PHP version 8.0 and I don't get the errors. This can be confirmed in this PHPCS job where PHP8.0 is installed and the check has passed. This is why I have used StephaneBour/actions-php-lint@8.0 and explicitly mentioned PHP version 8.0. We get the errors for PHP8.1+ versions.

Sorry, I should have been clearer. What I meant is that in the issue comment WordPress/WordPress-Coding-Standards#2035 (comment), it says:

The last WPCS release does not support PHP 8.0 or 8.1. Either run it on PHP < 8.0 or use develop for the time being until the next release.

"The last WPCS release" refers to the "^2.3" version. It does not support PHP 8.0, at least officially.

But I guess if it works for us, it is okay for us to use it with PHP 8.0.

I'm not in favor of suppressing warnings. We may end up not fixing these warnings for a long time so I would rather have these visible until they are fixed.

I agree. 🙂 👍

Side note: I also look into the PHP Coding Standards GitHub Action that runs PHPCS. The PHP version used in the GitHub Action depends on the woocommerce/grow/prepare-php@actions-v1:

It says description: Specify PHP version. "7.4" by default.. So if we don't specify the PHP version in our YAML file then it will pick PHP7.4. Since we have mentioned 8.0, our job runs with PHP8.0.

Ahh, got it. I think when I was testing and reviewing your PR with that side note, I haven't got your latest commit at that time. I see them now. Thank you. 🙂

[ecgan]

The changes here look good. Tested and they run okay. I'm approving this PR. 👍

I am leaving a few minor comments below. I'll trust your judgement on them. 🙂

Thanks so much @krutidugade !

[krutidugade]

Sorry, I should have been clearer. What I meant is that in the issue comment WordPress/WordPress-Coding-Standards#2035 (comment), it says:

The last WPCS release does not support PHP 8.0 or 8.1. Either run it on PHP < 8.0 or use develop for the time being until the next release.

"The last WPCS release" refers to the "^2.3" version. It does not support PHP 8.0, at least officially.
But I guess if it works for us, it is okay for us to use it with PHP 8.0.

Thank you for sharing additional context. I understand you better now. And yes, let's still go ahead with 8.0 given it works for us.