Strict type checking throws error on formatting Doctrine decimal type as number

Devristo commented 3 years ago

Environment

Sonata packages

show

``` $ composer show --latest 'sonata-project/*' sonata-project/admin-bundle 3.102.0 3.102.0 The missing Symfony Admin Generator sonata-project/block-bundle 4.6.0 4.6.0 Symfony SonataBlockBundle sonata-project/cache 2.1.1 2.1.1 Cache library sonata-project/doctrine-extensions 1.13.0 1.13.0 Doctrine2 behavioral extensions sonata-project/doctrine-orm-admin-bundle 3.34.3 3.34.3 Integrate Doctrine ORM into the SonataAdminBundle sonata-project/exporter 2.7.0 2.7.0 Lightweight Exporter library sonata-project/form-extensions 1.9.0 1.9.0 Symfony form extensions sonata-project/intl-bundle 2.10.1 2.10.1 Symfony SonataIntlBundle sonata-project/translation-bundle 2.8.1 2.8.1 SonataTranslationBundle sonata-project/twig-extensions 1.6.0 1.6.0 Sonata twig extensions ```

Symfony packages

show

``` $ composer show --latest 'symfony/*' # Put the result here. ```

PHP version

$ php -v
PHP 8.0.0 (cli) (built: Dec  1 2020 03:33:03) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.0, Copyright (c), by Zend Technologies

Subject

When rendering a Doctrine DECIMAL type (PHP type is string) the NumberHelper::formatDecimal is called. Eventually a \NumberFormatter::format is called which accepts int | float only. Since declare(strict_types=1); is set PHP throws an error.

Exactly the same issue has been reported and fixed for NumberHelper::formatCurrency in the past.

https://github.com/sonata-project/SonataIntlBundle/issues/256

Minimal repository with the bug

Steps to reproduce

Try to format Doctrine DECIMAL type as number in for example the configureListFields .

Expected results

Actual results

TypeError:
NumberFormatter::format(): Argument #1 ($num) must be of type int|float, string given

  at /app/vendor/sonata-project/intl-bundle/src/Templating/Helper/NumberHelper.php:225
  at NumberFormatter->format('15.04')
     (/app/vendor/sonata-project/intl-bundle/src/Templating/Helper/NumberHelper.php:225)
  at Sonata\IntlBundle\Templating\Helper\NumberHelper->format('15.04', 1, array(), array(), null, null)
     (/app/vendor/sonata-project/intl-bundle/src/Templating/Helper/NumberHelper.php:114)
  at Sonata\IntlBundle\Templating\Helper\NumberHelper->formatDecimal('15.04', array(), array(), null, null)
     (/app/vendor/sonata-project/intl-bundle/src/Twig/Extension/NumberExtension.php:106)

nocive commented 3 years ago

In my case, this is preventing successful migration to PHP8.

VincentLanglet commented 3 years ago

In my case, this is preventing successful migration to PHP8.

Wanna provide a PR ?

nocive commented 3 years ago

@VincentLanglet it's gonna be a first for me around here, but I'm happy to.

VincentLanglet commented 3 years ago

@VincentLanglet it's gonna be a first for me around here, but I'm happy to.

This was added to fix it in formatCurrency https://github.com/sonata-project/SonataIntlBundle/blob/2.x/src/Templating/Helper/NumberHelper.php#L151-L154

I would say that all the method of the Helper with

@param string|float|int $number

should handle the similar issue.

To me, best would be to introduce an private method which

Accept string|float|int
Return float|int is it's a float|int
Return the cast to float if it's a numeric string
throw a TypeError if it's a non-numeric string And use it in every of those method.

But maybe @sonata-project/contributors thinks differently.

nocive commented 3 years ago

@VincentLanglet tks for the input.

My idea would be to patch the format() method in NumberHelper which seems to be used for all cases, and:

Continue accepting mixed for $number
TypeError if $number is not an object nor a scalar
Check if $number is an object and look for __toString method, TypeError if not found - but perhaps you don't want to allow for objects at all (?), in which case maybe we simply throw TypeError if it's not a scalar
Check if it's a string & numeric and typecast to float if it is, TypeError if it's not

On another note, should the PR target master or 2.x?

VincentLanglet commented 3 years ago

Indeed if format is called every time, it's seems better to patch directly the format method.

VincentLanglet commented 3 years ago

But supporting string|int|float might be enough

nocive commented 3 years ago

Well, it's actually not used for formatCurrency(), which has the custom patch you pointed out earlier, all other cases go through the format() method.

I guess in that case we can introduce the private method like you suggested, encapsulate the transformation logic there and invoke it from both format() and formatCurrency().

VincentLanglet commented 3 years ago

Do you know if this (or others sonata project) is using those methods with string ? Because the other solution would be to remove string from the phpdoc and ask users to cast themself.

If we decide to support string, we'll have to add tests with string values https://github.com/sonata-project/SonataIntlBundle/blob/2.x/tests/Helper/NumberHelperTest.php

nocive commented 3 years ago

I would say it's highly likely since templates tend not to be cleanest of code and people will often end up formatting stuff that is a string or null (speaking of which, how should null be handled?).

Speaking about what I know, we number format strings quite often in plenty of legacy code.

VincentLanglet commented 3 years ago

I would say it's highly likely since templates tend not to be cleanest of code and people will often end up formatting stuff that is a string or null (speaking of which, how should null be handled?).

The phpdoc say nothing about null so I would prefer to not try to add support. The user should add least add a null check before formatting the value ; the same way you can't call format on \DateTime|null.

Speaking about what I know, we number format strings quite often in plenty of legacy code.

In the same way legacy code are calling native php methods to string|null and PHP9 will throw an error when you pass null.

sonata-project / SonataIntlBundle