Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Products with no price break the product selector #2578

Closed
Aljullu opened this issue May 27, 2020 · 6 comments · Fixed by #2722
Closed

Products with no price break the product selector #2578

Aljullu opened this issue May 27, 2020 · 6 comments · Fixed by #2722
Labels
focus: rest api Work impacting REST api routes. type: bug The issue/PR concerns a confirmed bug.

Comments

@Aljullu
Copy link
Contributor

Aljullu commented May 27, 2020

Similarly to #2551, I'm only able to reproduce this issue in a specific server, but not in local.

To Reproduce

  1. Create a product (or modify an existing one), so it doesn't have a price.
    imatge
  2. Create a page and add a Hand-Picked Products block or a Featured Product block. I guess all blocks which have a product selector will have this issue.
  3. Notice the block placeholder shows a 'The response is not a valid JSON response.' error message:
    imatge

The API response is:

Warning: A non-numeric value encountered in /usr/home/.../web/wp-content/plugins/woocommerce-gutenberg-products-block-2.6.0/src/StoreApi/Schemas/AbstractSchema.php on line 243

... /* the error message is repeated three times per each product with no price */

[ /* correct JSON data here */ ]

My guess is that we need to return early in:
https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/master/src/StoreApi/Schemas/AbstractSchema.php#L240
if $amount is not a number.

@Aljullu Aljullu added type: bug The issue/PR concerns a confirmed bug. status: blocker Used on issues or pulls that block work from being released. focus: rest api Work impacting REST api routes. labels May 27, 2020
@Aljullu Aljullu added this to the 2.6.0 milestone May 27, 2020
@nerrad nerrad self-assigned this May 27, 2020
@nerrad
Copy link
Contributor

nerrad commented May 27, 2020

Probably related to this.

@nerrad nerrad removed this from the 2.6.0 milestone May 27, 2020
@nerrad nerrad removed their assignment May 27, 2020
@Aljullu
Copy link
Contributor Author

Aljullu commented May 27, 2020

I couldn't find yet why this error is only occurring in my server but not in local.

Basically, the $amount value is an empty string (it comes from here). wc_format_decimal() doesn't modify it, so we end up with "" * ( 10 ** 2 ), the server shows a warning with this calculation while local, doesn't.

Server is PHP 7.4 while local is 7.3.

Doing this change fixes the issue:

-wc_format_decimal( $amount ) * ( 10 ** $decimals ),
+floatval( wc_format_decimal( $amount ) ) * ( 10 ** $decimals ),

@nerrad
Copy link
Contributor

nerrad commented May 27, 2020

wc_format_decimal removes all decimals etc from the string right? So could it just be absint?

@Aljullu
Copy link
Contributor Author

Aljullu commented May 29, 2020

@nerrad I don't think we can use absint here, wc_format_decimal might return strings representing decimal numbers (ie: "9.99").

@nerrad
Copy link
Contributor

nerrad commented May 29, 2020

Hmm, I saw this code doc for wc_format_decimal without looking at the code, seems like it's not accurate then:

Sanitize, remove decimals, and optionally round + trim off zeros.

@Aljullu
Copy link
Contributor Author

Aljullu commented May 29, 2020

Yeah, I'm a bit confused by that remove decimals from the method description, but rounding is controlled by the $dp argument. Leaving it undefined (so it defaults to false) means no rounding happens.

I tested it locally, but the tests are also quite handy to see how it works: https://github.com/woocommerce/woocommerce/blob/master/tests/legacy/unit-tests/formatting/functions.php#L297.

@Aljullu Aljullu removed the status: blocker Used on issues or pulls that block work from being released. label Jun 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: rest api Work impacting REST api routes. type: bug The issue/PR concerns a confirmed bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants