Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Re: Make bcmath functions more strict »

Re: Make bcmath functions more strict

php.internals

Legale Legage

7 years ago
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.

Vladyslav Startsev

7 years ago
Any other ideas on that one? I'll wait for one more week and make RFC then

Benjamin Morel

7 years ago
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:

Vladyslav Startsev

7 years ago
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 > > 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 >> >> -- >> PHP Internals - PHP Runtime Development Mailing List >> To unsubscribe, visit: http://www.php.net/unsub.php >>
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

Benjamin Morel

7 years ago
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

Christoph Becker

7 years ago
On 26.04.2019 at 09:45, Benjamin Morel wrote:
>> 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

Benjamin Morel

7 years ago
> 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

Vladyslav Startsev

7 years ago
> 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

Benjamin Morel

7 years ago
> 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

Nikita Popov

7 years ago
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