Skip to content
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

Add an array_sum utility method to enhance PHP 8.3+ compatibility #47595

Merged
merged 6 commits into from
May 20, 2024

Conversation

coreymckrill
Copy link
Collaborator

@coreymckrill coreymckrill commented May 17, 2024

Changes proposed in this Pull Request:

In #40660 several people are reporting warnings such as the following when running on PHP 8.3:

PHP Warning: array_sum(): Addition is not supported on type string in /home/public_html/wp-content/plugins/woocommerce/includes/rest-api/Controllers/Version1/class-wc-rest-orders-v1-controller.php on line 271

This seems to happen when for some reason a non-numeric value makes it into an array that should be all integers and floats, possibly due to legacy data that hasn't been sanitized correctly or extensions that inject incorrect data types.

Rather than fixing each case of this separately, this PR introduces a new array_sum method to the NumberUtil class that takes a similar approach to the round method that is already in that class. It acts as a wrapper around the built-in array_sum, while adding a sanitization step to ensure that all the values in the array are numeric.

Note that there are over 100 uses of array_sum in the WooCommerce codebase. Rather than updating every usage to the new method, which would cause this PR to have dozens of changed files and be difficult to test, as well as troubleshoot if something goes wrong, this simply fixes the usage cases that have already been reported to be problematic, as well as a few that seem especially likely to have the same issue.

Fixes #40660

How to test the changes in this Pull Request:

Code review, and executing the test suite under PHP 8.3, is probably the best way to assess this change. Some manual steps that can be performed for added confidence:

  1. Make sure your test site is running on PHP 8.3, and that your site has WP_DEBUG_LOG set to true.
  2. Set up a tax rate, shipping zone, and shipping method. Make sure the shipping method is marked as taxable.
  3. Create an order. Add billing and shipping details that will make the order qualify for the tax rate and the shipping zone.
  4. Add items to the order, including shipping.
  5. Add this code snippet to your site. This will inject extra, invalid values into the shipping taxes when you make a REST API request:
    add_filter(
    	'woocommerce_order_item_get_taxes',
    	function ( $value ) {
    		$value['total'][] = '';
    		$value['total'][] = 'asdf';
    		$value['total'][] = '1,3';
    
    		return $value;
    	}
    );
  6. Make the following request to the REST API (note that it's using v1 of the API, not the current v3):
    curl --request GET \
    	--url 'http://localhost:8888/wp-json/wc/v1/orders/53?_fields=id%2Cshipping_lines'
    
  7. In the response, under the shipping_lines property and the taxes property within that, you should see the shipping tax you added to the order, plus the extra values injected by the code snippet. You should also see "total_tax", which should contain the correct sum of the valid tax items.
  8. Now check your log file, enabled by WP_DEBUG_LOG. This is generally found at wp-content/debug.log. Ideally the file won't be there, because no PHP warning was logged. But if the file exists, check to make sure it does not have an entry like the one listed at the top of this PR.

@coreymckrill coreymckrill added plugin: woocommerce Issues related to the WooCommerce Core plugin. type: compatibility labels May 17, 2024
@coreymckrill coreymckrill self-assigned this May 17, 2024
Copy link
Contributor

github-actions bot commented May 17, 2024

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

@coreymckrill coreymckrill requested review from a team and Konamiman and removed request for a team May 17, 2024 22:19
@coreymckrill coreymckrill marked this pull request as ready for review May 17, 2024 22:19
Copy link
Contributor

Hi @Konamiman,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

1 similar comment
Copy link
Contributor

Hi @Konamiman,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@Konamiman Konamiman merged commit ad6eea6 into trunk May 20, 2024
23 checks passed
@Konamiman Konamiman deleted the fix/40660/array-sum branch May 20, 2024 08:10
@github-actions github-actions bot added this to the 9.0.0 milestone May 20, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label May 20, 2024
@rodelgc rodelgc added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris type: compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PHP 8.3] array_sum(): Addition is not supported on type string
3 participants