FIX number of parameters at ArgumentCountError exception

[tkuschel]

tl;dr The exception ArgumentCountError gives wrong given parameter length.

The func_num_args() used - only returns the number of parameters of the called function and is always 2 in this case. You are probably wondering why I came up with this? My editor shows me an error that the parameters for the __callStatic() magic function should always be the amount of 2.

I also adapted the test file with phpunit version 11.1.3, expanded it and checked the output with phpunit --testdox-test testdox.txt --display-skipped:

tom@silver ~/github/bcmath_compat (git)-[master] % phpunit --testdox-text testdox.txt --display-skipped
PHPUnit 11.1.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.7
Configuration: /home/tom/github/bcmath_compat/phpunit.xml.dist

...............................................................  63 / 152 ( 41%)
............................................................... 126 / 152 ( 82%)
..................SSSSS..S                                      152 / 152 (100%)

Time: 00:00.154, Memory: 26.86 MB

There were 6 skipped tests:

1) BCMathTest::test_argumentsScaleCallstatic with data set #1 (4, 2)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:372 : bcscale() expects at most 1 parameters, 2 given

2) BCMathTest::test_argumentsScaleCallstatic with data set #2 (4, 2, 3)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:372 : bcscale() expects at most 1 parameters, 2 given

3) BCMathTest::test_argumentsScaleCallstatic with data set #3 (4, 2, 3, 5)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:372 : bcscale() expects at most 1 parameters, 2 given

4) BCMathTest::test_argumentsPowModCallstatic with data set #0 ('9')
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:368 : bcpowmod() expects at least 3 parameters, 2 given

5) BCMathTest::test_argumentsPowModCallstatic with data set #1 ('9', '17')
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:368 : bcpowmod() expects at least 3 parameters, 2 given

6) BCMathTest::test_argumentsPowModCallstatic with data set #4 ('9', '17', '-111', 5, 8)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:372 : bcpowmod() expects at most 4 parameters, 2 given

OK, but some tests were skipped!
Tests: 152, Assertions: 155, Skipped: 6.

Now with the patch, which is also attached, we get: bcmath_patch.txt

tom@silver ~/github/bcmath_compat (git)-[master] % phpunit --testdox-text testdox.txt --display-skipped
PHPUnit 11.1.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.7
Configuration: /home/tom/github/bcmath_compat/phpunit.xml.dist

...............................................................  63 / 152 ( 41%)
............................................................... 126 / 152 ( 82%)
..................SSSSS..S                                      152 / 152 (100%)

Time: 00:00.155, Memory: 26.86 MB

There were 6 skipped tests:

1) BCMathTest::test_argumentsScaleCallstatic with data set #1 (4, 2)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:373 : bcscale() expects at most 1 parameters, 2 given

2) BCMathTest::test_argumentsScaleCallstatic with data set #2 (4, 2, 3)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:373 : bcscale() expects at most 1 parameters, 3 given

3) BCMathTest::test_argumentsScaleCallstatic with data set #3 (4, 2, 3, 5)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:373 : bcscale() expects at most 1 parameters, 4 given

4) BCMathTest::test_argumentsPowModCallstatic with data set #0 ('9')
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:369 : bcpowmod() expects at least 3 parameters, 1 given

5) BCMathTest::test_argumentsPowModCallstatic with data set #1 ('9', '17')
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:369 : bcpowmod() expects at least 3 parameters, 2 given

6) BCMathTest::test_argumentsPowModCallstatic with data set #4 ('9', '17', '-111', 5, 8)
ArgumentCountError in /home/tom/github/bcmath_compat/vendor/phpseclib/bcmath_compat/src/BCMath.php:373 : bcpowmod() expects at most 4 parameters, 5 given

OK, but some tests were skipped!
Tests: 152, Assertions: 155, Skipped: 6.

Best regards, tkuschel

[terrafrost]

Looks like the unit tests are failing.

I can take a look this evening but if you wanna take a look sooner that'd be cool too!

[terrafrost]

So it looks like it's your changes to the unit tests that are causing the failures on GitHub Actions. It's like you made the unit tests compliant with a newer version of PHPUnit without updating the version of PHPUnit required in composer.json.

Your changes, otherwise, do look good, and it'd be good for the 1.0 / 2.0 branches to have them as well - just not the unit test changes. I also have a feeling that the unit test changes require a newer version of PHPUnit than the 1.0 branch could even support due to the 1.0 branch working on versions of PHP as low as 5.6.

I need to get ready to go into work but if you want to update your PR request to include an update to composer.json that'd be great. If not I'll cherry-pick your commit into the 1.0 branch, undo the unit test changes in a second commit, and then merge those two commits into the 2.0 branch and then merge 2.0 into master.

[tkuschel]

I will have a look at the composer.json, but I need some time to do so. Automatic testing on github is new territory for me. I don't know the necessary settings on github. How do you trigger the phpunit in github.... Please give me this time, then I'll try it myself. BR Tom

[terrafrost]

I think all you need to do is go to the Settings tab of your repo and enable GitHub Actions from there. The repo, itself, already contains all the necessary config info for that (in the .github subdirectory).

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository talks about this more. In truth it's been a while since I've setup GitHub Actions on a fork.

I guess I'll give you until the weekend to try to figure it out? Not trying to rush you or anything but you made a good find and I'm eager to have it in the codebase 😁

Thanks!

[tkuschel]

The updated tests run smoothly in my forked repo and I've done it.

[terrafrost]

And it's been merged!:

https://github.com/phpseclib/bcmath_compat/commit/50affd7fad37fb8338334b988ca6a4e9a1d528b9

Thanks!