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

Relax ext-bcmath requirement in composer.json and instead add checks when attempting to use BinaryCalculator #187

Open
pointybeard opened this issue Sep 19, 2023 · 1 comment · May be fixed by #188

Comments

@pointybeard
Copy link

Currently, there is a requirement for ext-bcmath to be installed:

"require": {
"php": "^7.3 || ^8.0",
"ext-bcmath": "*",
"ext-intl": "*"
},

However, it is possible to use SimpleCalculator instead of BinaryCalculator:

* @return int|float|string
*/
public function as(UnitInterface $unit, int $precision = null, bool $binary = false)
{
return \UnitConverter\UnitConverter::createBuilder()
->{'add'.(($binary) ? 'Binary' : 'Simple').'Calculator'}() # ¯\_(ツ)_/¯
->addRegistryWith(array_unique([$this, $unit]))
->build()

To give flexibility, and support systems that do not or cannot install php-bcmath, I suggest adding some assertions in AbstractUnit::as() or in BinaryCalculator to check that the bcmath extension is installed if someone is attempting to use the BinaryCalculator.

It appears this is approach is already taken for ext-intl here.

@pointybeard
Copy link
Author

pointybeard commented Sep 19, 2023

Looks like BinaryCalculator already has an assertion:

public function __construct(int $precision = null, int $roundingMode = null)
{
if (extension_loaded('bcmath')) {
parent::__construct($precision, $roundingMode);
return;
}
$fqcn = explode('\\', static::class);
throw new RuntimeException(sprintf(
'Unable to construct a %s due to missing bcmath library. Please check your PHP extensions.',
end($fqcn),
));

So, it seems it just a matter of changing composer.json. ext-intl and ext-bcmath could be put into the suggest block of composer.json. E.g.

"suggest": {
    "ext-bcmath": "Needed to enable usage of BinaryCalculator",
    "ext-intl": "Needed to support spellout() feature of UnitConverter"
}

pointybeard added a commit to pointybeard-forks/unit-converter that referenced this issue Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant