Closed brbrr closed 4 months ago
This issue has been marked as stale. This happened because:
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.
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()
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.
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?
@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.
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
.
@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!
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