Skip to content

Fix #79212: NumberFormatter::format() may detect wrong type #5141

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

Closed
wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Feb 2, 2020

We have to convert to number before detecting the type, to cater to
internal objects implementing cast_object.

We also get rid of the fallback behavior of using FORMAT_TYPE_INT32,
because that can no longer happen; after convert_to_numer() the type
is either IS_LONG or IS_DOUBLE.

We have to convert to number *before* detecting the type, to cater to
internal objects implementing `cast_object`.

We also get rid of the fallback behavior of using `FORMAT_TYPE_INT32`,
because that can no longer happen; after `convert_to_numer()` the type
is either `IS_LONG` or `IS_DOUBLE`.
Like the name implies, `convert_scalar_to_number_ex()` does not convert
array.  We enforce array to integer conversion right away, to avoid
triggering the type confusion when `::TYPE_INT64` is passed as `$type`.
if(Z_TYPE_P(number) != IS_ARRAY) {
convert_scalar_to_number_ex(number);
} else {
convert_to_long(number);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest treating this as an error condition instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can break BC here for PHP-7.3 and 7.4 (passing an array is currently supported, and the docs even implicitly support that: "Can be integer or float, other values will be converted to a numeric value."). I think changing this for master, would be prudent, though.

@cmb69
Copy link
Member Author

cmb69 commented Feb 3, 2020

Thanks! Applied as c293549.

I'll provide a PR for master ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants