Skip to content
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

[PHP 8.4] Add bcdivmod #503

Draft
wants to merge 1 commit into
base: 1.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/Php84/Php84.php
Original file line number Diff line number Diff line change
Expand Up @@ -169,4 +169,23 @@ private static function mb_internal_trim(string $regex, string $string, ?string

return mb_convert_encoding($string, $encoding, 'UTF-8');
}

public static function bcdivmod(string $num1, string $num2, ?int $scale = null): array {
if ($num2 === '0') {
throw new \DivisionByZeroError('Division by zero');
}

if (!is_numeric($num1)) {
throw new \ValueError('Argument #1 ($num1) is not well-formed');
}

if (!is_numeric($num2)) {
throw new \ValueError('Argument #2 ($num2) is not well-formed');
}
Comment on lines +174 to +184
Copy link
Member

Choose a reason for hiding this comment

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

Those two errors are not available on PHP 7. I suggest to emit a E_USER_WARNING and return null instead if we're on PHP < 8.


return [
\bcdiv($num1, $num2, 0),
\bcmod($num1, $num2, $scale),
];
}
}
4 changes: 4 additions & 0 deletions src/Php84/bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,7 @@ function mb_ltrim(string $string, ?string $characters = null, ?string $encoding
function mb_rtrim(string $string, ?string $characters = null, ?string $encoding = null): string { return p\Php84::mb_rtrim($string, $characters, $encoding); }
}
}

if (function_exists('bcdiv')) {
function bcdivmod(string $num1, string $num2, ?int $scale = null): array {return p\Php84::bcdivmod($num1, $num2, $scale); }
Copy link
Member

Choose a reason for hiding this comment

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

such polyfill function should be defined only when the bcmath extension is loaded (similar to how we deal with mbstring new functions just above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the function_exists('bcdiv') call will check for this, and will also play nicely with any potential polyfills that may declare a bcdiv function. If you think, I can update this with a full extension_loaded call.

Copy link
Member

Choose a reason for hiding this comment

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

oh, I missed that this was checking bcdiv and not bcdivmod. But then, it misses the check for bcdivmod (all polyfilled functions need to be guarded to avoid double function definition).

and for the detection of bcmath, please use extension_loaded for consistency with our other polyfills. Thus, a extension_loaded call is fully resolved at compile-time by OPCache.

}
15 changes: 15 additions & 0 deletions tests/Php84/Php84Test.php
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,16 @@ public function testMbTrimCharactersEncoding()
mb_internal_encoding($old);
}

/**
* @covers \Symfony\Polyfill\Php84\Php84::bcdivmod
*
* @dataProvider bcDivModProvider
*/
public function testBcDivMod(string $num1, string $num2, ?int $scale = null, array $expected): void
Copy link
Member

Choose a reason for hiding this comment

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

this test needs @requires extension bcmath as the implementation would break without that extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brilliant, thank you.

{
$this->assertSame($expected, bcdivmod($num1, $num2, $scale));
}

public static function mbTrimProvider(): iterable
{
yield ['ABC', 'ABC'];
Expand Down Expand Up @@ -319,4 +329,9 @@ public static function mbRTrimProvider(): iterable

yield ["foo\n", "foo\n", 'o'];
}

public static function bcDivModProvider(): iterable
{
yield ['10', '10', null, ['1', '0']];
Copy link
Member

@derrabus derrabus Sep 25, 2024

Choose a reason for hiding this comment

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

A couple more tests would be nice. You're only testing the simplest path.

Also, beware that by setting the scale to null, your test depends on the global bcmath.scale setting. Currently, the test will fail if it's run in an environment where that setting is not 0.

}
}
Loading