Skip to content

Conversation

@jensdenies
Copy link
Contributor

@jensdenies jensdenies commented Dec 27, 2020

Hi,

This my first attempt to solve one of the bugs from the PHP bug tracker (#80545). I targeted this to the PHP 8.0 branch because the warning promotion didn't take place yet in PHP 7.4 (please correct me if I'm wrong).

Bccomp had the same issue, so I changed it there too.

If there's anything I did wrong or needs to change, please let me know! :)

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2020

That is analogous to what GMP does, but in my opinion, it would be beneficial to have a dedicated function to verify the validity of a number given as string. Also, I think a ValueError would be more appropriate than a TypeError.

@jensdenies
Copy link
Contributor Author

@cmb69 I will change it to a value error, that indeed looks more appropriate.

There's a function called php_str2num in the bcmath.c file, or would you like to have a globally accessible function?

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2020

I will change it to a value error, that indeed looks more appropriate.

I'd rather be consistent with GMP. @Girgias, since you promoted to TypeError, what do you think? /cc @nikic

There's a function called php_str2num in the bcmath.c file, or would you like to have a globally accessible function?

Yes, this was about a function which could be called from userland. And that function should just return true or false, similar to is_numeric(). Anyhow, a new function should likely have its own PR.

@Girgias
Copy link
Member

Girgias commented Dec 28, 2020

I think a ValueError would be better, not exactly sure why I did a TypeError for GMP, but I need to possibly tweak some things with GMP anyway.

Would it be possible to use the version with the argument number (IIRC zend_argument_value_error()) ? As this is usually better IMHO.

@cmb69
Copy link
Member

cmb69 commented Dec 28, 2020

Would it be possible to use the version with the argument number (IIRC zend_argument_value_error()) ?

That would require to pass the arg num to php_str2num(), but that shouldn't be an issue.

Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
@jensdenies
Copy link
Contributor Author

@cmb69 @Girgias I changed the TypeError to a ValueError and I've used the zend_argument_value_error variant.

@jensdenies
Copy link
Contributor Author

Not sure why the CI failed.

@cmb69
Copy link
Member

cmb69 commented Dec 29, 2020

I think that CI failure is unrelated to this PR.

Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
@jensdenies
Copy link
Contributor Author

@nikic Thanks, I changed the return type from void to zend_result and moved the value error to the functions itself. I also added the early returns for the other functions in this file. Also noticed some empty returns at the end of some functions, I removed those as they seem unnecessary. The tests still pass locally.

Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

This looks good to me, I'd only suggest to adjust the message to fix the "argument_value_error" format better.

Signed-off-by: Jens de Nies <j.de.nies@protonmail.com>
@jensdenies
Copy link
Contributor Author

@nikic I've pointed this PR to the PHP-8.0 branch, is that correct?

@nikic
Copy link
Member

nikic commented Jan 19, 2021

I merged this as 94a151a a while ago, but messed up the auto-close tag.

@nikic nikic closed this Jan 19, 2021
@jensdenies jensdenies deleted the bugfix/80545 branch January 19, 2021 12:27
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.

4 participants