Skip to content

BcMath\Number methods are not 1-to-1 with bcmath functions due to int argument signature #18674

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
zimzat opened this issue May 27, 2025 · 6 comments

Comments

@zimzat
Copy link

zimzat commented May 27, 2025

Description

The following code:

<?php
var_dump(
    (string)new BcMath\Number(5)->sub(4.2, 1),
    bcsub(5, 4.2, 1),
);

Resulted in this output:

string(3) "1.0"
string(3) "0.8"

But I expected this output instead:

string(3) "0.8"
string(3) "0.8"

The signature of bcsub only accepts string which means integers and floats will both be cast to string, while BcMath\Number::sub allows integers, causing it to truncate the float and emit a deprecation notice instead of preserving the decimal places.

 bcsub(string $num1, string $num2, ?int $scale = null): string
 public BcMath\Number::sub(BcMath\Number|string|int $num, ?int $scale = null): BcMath\Number

Ultimately this means usages of bcmath functions cannot be automatically converted to objects and developers manually typing static math operations must enclose them in strings to avoid getting erroneous results.

PHP Version

PHP 8.4.7 (cli) (built: May  9 2025 06:54:08) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.4.7, Copyright (c) Zend Technologies
    with Zend OPcache v8.4.7, Copyright (c), by Zend Technologies

Operating System

No response

@ramsey
Copy link
Member

ramsey commented May 27, 2025

What I find interesting here is the difference in behavior. bcsub() produces different results from Number::sub(), but the user expectation is that they should produce the same results.

@ramsey
Copy link
Member

ramsey commented May 27, 2025

Here's the result on 3v4l: https://3v4l.org/Bi5Kl#v8.4.7

@nielsdos
Copy link
Member

It makes sense why this happens: bcsub has string typed arguments, while BcMath\Number uses BcMath\Number|string|int. In the engine, int coercion takes precedence over string when dealing with floats, so 4.2 becomes 4 for BcMath\Number while it is converted to the string "4.2" for bcsub.
A solution could be to drop the int argument type on BcMath\Number and then they should behave the same. Unless someone relies on the loss of precision this probably doesn't cause a BC break.
The behaviour would've been the same if https://wiki.php.net/rfc/allow_int_args_to_bcmath_function was added to 8.4 (i.e. both produces "1.0" instead of "0.8", probably also not really desirable)

@SakiTakamachi
Copy link
Member

SakiTakamachi commented May 28, 2025

The RFC clearly states that the constructor accepts an int.

Since there is no need to consider errors for integers in the range that can be represented by ints, it is reasonable to accept ints in the constructor.

https://wiki.php.net/rfc/support_object_type_in_bcmath

So this is not a bug.
I believe that a separate RFC is required to change the specifications outlined in the current RFC. In any case, this will not be changed in version 8.4.

If you wish to discuss this issue in more detail, I recommend gathering opinions via the internal mailing list.

Edit:
If there is strong demand for creating the object from a float, it would be better to add support for float rather than removing int from the signature.
As with the bc functions, converting a float to a string can be easily handled within php-src.

In any case, discussion on the mailing list is necessary.

@nielsdos
Copy link
Member

Closing per previous comment

@nielsdos nielsdos closed this as not planned Won't fix, can't repro, duplicate, stale May 28, 2025
@zimzat
Copy link
Author

zimzat commented May 28, 2025

If there is strong demand for creating the object from a float, it would be better to add support for float rather than removing int from the signature.

I think that would be acceptable. I guess I'll have to sign up for the mailing list instead of just passively watching externals.io 😶‍🌫️


(mostly written before seeing the edit above) I agree this isn't technically a bug in the strictest sense, however it massively limits the utility or fit for purpose Number has given it will do the exact opposite of half the reason why someone would want to use it: preserve decimal places in arithmetic. Knowing that $number * 5.25 will silently produce an invalid result (instead of throwing an exception) has me extremely hesitant to encourage team members to use it since this edge case is practically guaranteed to be hit (the code base I'm currently working in is massive and mixes-and-matches integer, float, and string representations of numbers all over the place; I was hoping Number could be a drop-in replacement to ease the transition to always using string representations).

Thanks!

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

No branches or pull requests

4 participants