Simple Payments: PHP Warning: number_format() expects parameter 1 to be float, string given

[brbrr]

It seems that this warning does not break anything UX wise, so I labeled it as Low priority.

Steps to reproduce the issue

I'm not really sure how to reproduce it locally, but the warning appears quite often during E2E tests in CI. I assume it's related to the fact that E2E tests insert a string into a numeric-only field

What happened instead

[28-Jan-2020 09:31:59 UTC] PHP Warning:  number_format() expects parameter 1 to be float, string given in /home/travis/build/Automattic/jetpack/modules/simple-payments/simple-payments.php on line 267
[28-Jan-2020 09:31:59 UTC] PHP Stack trace:
[28-Jan-2020 09:31:59 UTC] PHP   1. {main}() /home/travis/wordpress/index.php:0
[28-Jan-2020 09:31:59 UTC] PHP   2. require() /home/travis/wordpress/index.php:17
[28-Jan-2020 09:31:59 UTC] PHP   3. wp() /home/travis/wordpress/wp-blog-header.php:16
[28-Jan-2020 09:31:59 UTC] PHP   4. WP->main() /home/travis/wordpress/wp-includes/functions.php:1255
[28-Jan-2020 09:31:59 UTC] PHP   5. WP->parse_request() /home/travis/wordpress/wp-includes/class-wp.php:729
[28-Jan-2020 09:31:59 UTC] PHP   6. do_action_ref_array() /home/travis/wordpress/wp-includes/class-wp.php:387
[28-Jan-2020 09:31:59 UTC] PHP   7. WP_Hook->do_action() /home/travis/wordpress/wp-includes/plugin.php:544
[28-Jan-2020 09:31:59 UTC] PHP   8. WP_Hook->apply_filters() /home/travis/wordpress/wp-includes/class-wp-hook.php:312
[28-Jan-2020 09:31:59 UTC] PHP   9. rest_api_loaded() /home/travis/wordpress/wp-includes/class-wp-hook.php:288
[28-Jan-2020 09:31:59 UTC] PHP  10. WP_REST_Server->serve_request() /home/travis/wordpress/wp-includes/rest-api.php:305
[28-Jan-2020 09:31:59 UTC] PHP  11. WP_REST_Server->dispatch() /home/travis/wordpress/wp-includes/rest-api/class-wp-rest-server.php:329
[28-Jan-2020 09:31:59 UTC] PHP  12. WP_REST_Posts_Controller->update_item() /home/travis/wordpress/wp-includes/rest-api/class-wp-rest-server.php:946
[28-Jan-2020 09:31:59 UTC] PHP  13. WP_REST_Posts_Controller->prepare_item_for_response() /home/travis/wordpress/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php:758
[28-Jan-2020 09:31:59 UTC] PHP  14. apply_filters() /home/travis/wordpress/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php:1550
[28-Jan-2020 09:31:59 UTC] PHP  15. WP_Hook->apply_filters() /home/travis/wordpress/wp-includes/plugin.php:206
[28-Jan-2020 09:31:59 UTC] PHP  16. do_shortcode() /home/travis/wordpress/wp-includes/class-wp-hook.php:288
[28-Jan-2020 09:31:59 UTC] PHP  17. preg_replace_callback() /home/travis/wordpress/wp-includes/shortcodes.php:199
[28-Jan-2020 09:31:59 UTC] PHP  18. do_shortcode_tag() /home/travis/wordpress/wp-includes/shortcodes.php:199
[28-Jan-2020 09:31:59 UTC] PHP  19. Jetpack_Simple_Payments->parse_shortcode() /home/travis/wordpress/wp-includes/shortcodes.php:325
[28-Jan-2020 09:31:59 UTC] PHP  20. Jetpack_Simple_Payments->format_price() /home/travis/build/Automattic/jetpack/modules/simple-payments/simple-payments.php:149
[28-Jan-2020 09:31:59 UTC] PHP  21. number_format() /home/travis/build/Automattic/jetpack/modules/simple-payments/simple-payments.php:267

[stale[bot]]

This issue has been marked as stale. This happened because:

• It has been inactive in the past 6 months.
• It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

[kraftbj]

Now appears to fatal with an uncaught type error (PHP8+) instead of just generating a warning:

Should be reproducible via the simple-payment shortcode if the product has an invalid spay_price value (e.g 1,234.00 instead of 1234.00) - see Jetpack_Simple_Payments::parse_shortcode() and Jetpack_Simple_Payments_Widget::ajax_save_payment_button()

[kraftbj]

Over time, the issue has moved to _inc/lib/class-jetpack-currencies.php's format_price function.

While saving a float is good, it probably should also normalize the price in this function so any stored improper values are defensively handled.

I'm curious if we have a function (WP, PHP, JP) to normalize a currency value to a float, e.g. remove a currency sign, account for euro and US-style numbers "12.244,00" vs "12,244.00", etc. I don't immediately see one.

[fgiannar]

Linked PR is now ready for review.

While saving a float is good, it probably should also normalize the price in this function so any stored improper values are defensively handled.

@kraftbj I'd expect a cast to float to suffice here. Could you please elaborate on the above pls? Do you have a use-case in mind I might be missing?

[kraftbj]

@fgiannar

<?php

$price1 = "$1,249.00";
$price1 = (float) $price1;
echo $price1; // '0' as the cast will see the "$" and assume it isn't a number.

echo "\r\n";

$price2 = "1,249.00";
$price2 = (float) $price2;
echo $price2; // '1' as the cast will stop at the comma being used as the thousand separator.

[fgiannar]

Thanks Kraft for the additional case and Jeremy for the PR!

IMO given the method accepts a second argument for the currency, I wouldn't expect the $price argument to include the currency symbol.

Maybe a cleaner solution, closer to WPCOM's Store_Price::display_currency (mentioned in the method's docblock and defined in fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Snqzva%2Qcyhtvaf%2Sjcpbz%2Qovyyvat%2Sfgber%2Qcevpr.cuc%3Se%3Q6n341309%2335-og) would be to change the method's signature and accept a float instead of a string as $price.

[kraftbj]

@fgiannar That's a good point. Since the consuming code isn't expecting it to require a float, it seems like a bigger piece of work to change the method. I think you're right, though, that since it accepts a currency param, I wouldn't expect to even try to pass it in the price string. Unless this becomes a thing that people start reporting or we see in logs, I'm okay letting this lay as it is and pick it back up later if the need arises.

Thank you both!