Make bcmath functions more strict

Hello Vlad, internals.

If you'll make an RFC someone will certainly vote :-) 
Meanwhile till php bureaucracy hasn't put quorum rule in the voting process you have a chance to push it through.

Good luck.

Any other ideas on that one?

I'll wait for one more week and make RFC then

Hi Vladyslav,

I just checked your code snippet, here are my 2 cents:

• bcmath should definitely trigger an error when encountering a malformed
  string like '17 .123', silently converting it to zero is indeed the worth
  thing to do;
• I don't think that there's much value in having it handle exponential
  notation, you can use userland libraries such as brick/math
  https://github.com/brick/math for this;
• I really don't think that it should depend on the current locale, like
  PHP sometimes does with numbers; there has been some discussion just a few
  months ago to actually stop making float-to-string conversion depend on
  locale:
  https://externals.io/message/103638

Ben

On Thu, 25 Apr 2019 at 11:02, Vladyslav Startsev <
vladyslavstartsev@gmail.com> wrote:

Any other ideas on that one?

I'll wait for one more week and make RFC then

On Thu, Apr 25, 2019 at 12:35 PM Benjamin Morel
benjamin.morel@gmail.com wrote:

Hi Vladyslav,

I just checked your code snippet, here are my 2 cents:

• bcmath should definitely trigger an error when encountering a malformed string like '17 .123', silently converting it to zero is indeed the worth thing to do;
• I don't think that there's much value in having it handle exponential notation, you can use userland libraries such as brick/math for this;
• I really don't think that it should depend on the current locale, like PHP sometimes does with numbers; there has been some discussion just a few months ago to actually stop making float-to-string conversion depend on locale:
  https://externals.io/message/103638

Ben

Any other ideas on that one?

I'll wait for one more week and make RFC then

--

Hello Benjamin!
Thank you for the reply.

Can you please explain your point of view a little bit?
Things that I don't understand

• bcmath should definitely trigger an error when encountering a malformed string like '17 .123', silently converting it to zero is indeed the worth thing to do;
  I don't really understand it, I must be strict, converting to zero
  will lead to wrong calculations.

• I don't think that there's much value in having it handle exponential notation, you can use userland libraries such as brick/math for this;
  Definitely yes, that was my point

• I really don't think that it should depend on the current locale, like PHP sometimes does with numbers; there has been some discussion just a few months ago to actually stop making float-to-string conversion depend on locale:
  Sure thing, I was just trying to describe all the cases that we have

Hi Vladyslav,

I don't really understand it, I must be strict, converting to zero
will lead to wrong calculations.

That's what I said :) Fully agree here, any non-numeric string should throw
an exception.

In summary, I would check that the string matches /^[\+\-]?[0-9]+(\.[0-9]+)?$/or else throw an exception.

• Ben

I don't really understand it, I must be strict, converting to zero
will lead to wrong calculations.

That's what I said :) Fully agree here, any non-numeric string should throw
an exception.

In summary, I would check that the string matches /^[\+\-]?[0-9]+(\.[0-9]+)?$/or else throw an exception.

Throwing an exception would be rather uncommon for functions, and would
definitely have to wait for PHP 8 for BC reasons. Another option might
be to introduce an object-oriented layer on top of libbcmath. A basic
problem of the BC extension is that all functions accept strings,
convert these to bc_nums[1], do the actual calculation, convert the
bc_nums back to strings[2], and return these. This looks like quite
some overhead, especially for more complex calculations where the
intermediary results are not even needed as strings. If there is an OOP
API, the conversion from string would only be needed in the constructor,
and conversion to string only in the __toString() method. All
intermediary calculations wouldn't need the from/to string conversions.
And of course, users could switch to the OOP API if they desire, but
could stick with the procedural API otherwise. An additional benefit
might be that we sometime could switch away from the decimal storage[3]
which appears to be mostly useful for the string conversions, but is
otherwise wasteful (1 byte per decimal digit).

[1]
https://github.com/php/php-src/blob/php-7.3.4/ext/bcmath/libbcmath/src/str2num.c#L44
[2]
https://github.com/php/php-src/blob/php-7.3.4/ext/bcmath/libbcmath/src/num2str.c
[3]
https://github.com/php/php-src/blob/php-7.3.4/ext/bcmath/libbcmath/src/bcmath.h#L61-L64

--
Christoph M. Becker

Throwing an exception would be rather uncommon for functions, and would
definitely have to wait for PHP 8 for BC reasons.

Sure. This is actually a discussion I'd like to start for all PHP functions
in PHP 8.

Another option might
be to introduce an object-oriented layer on top of libbcmath.

That would be nice, but I the meantime though (PHP 7.4?) we could just add
a warning when the number is malformed and zero is returned for this reason.
At least this warning can be logged, or caught by an error handler to
convert it to an exception in userland.

• Ben

Another option might be to introduce an object-oriented layer on top of libbcmath

I think it should be a separate discussion for this. Create new
discussion so we can talk there.

Throwing an exception would be rather uncommon for functions, and would
definitely have to wait for PHP 8 for BC reasons.

Sure. This is actually a discussion I'd like to start for all PHP functions in PHP 8.

Yes, PHP 8 for BCs and PHP 7.4 for warnings that's what I was trying to said

Yes, PHP 8 for BCs and PHP 7.4 for warnings that's what I was trying to
said

FWIW, I don't think anyone will complain about adding a warning to
something that's obviously a bug when it fails.
I'd say go ahead a write an RFC. If you want to have more chances for it to
pass though, I would not change the semantics of the current conversions
from string to decimal (what's considered a valid decimal number and what's
not). I would just propose to add a warning when an argument is converted
to zero because it is currently considered malformed.

• Ben

On Fri, Apr 26, 2019 at 3:10 PM Benjamin Morel benjamin.morel@gmail.com
wrote:

Yes, PHP 8 for BCs and PHP 7.4 for warnings that's what I was trying to
said

FWIW, I don't think anyone will complain about adding a warning to
something that's obviously a bug when it fails.
I'd say go ahead a write an RFC. If you want to have more chances for it to
pass though, I would not change the semantics of the current conversions
from string to decimal (what's considered a valid decimal number and what's
not). I would just propose to add a warning when an argument is converted
to zero because it is currently considered malformed.

• Ben

Feel free to just send a PR that implements the warning. We don't really
need an RFC for such a minor and straightforward change.

Nikita