Skip to content

FormatterNumberTest::testIntlAsScientific fails on GitLab #17708

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

Closed
schmunk42 opened this issue Dec 4, 2019 · 28 comments
Closed

FormatterNumberTest::testIntlAsScientific fails on GitLab #17708

schmunk42 opened this issue Dec 4, 2019 · 28 comments

Comments

@schmunk42
Copy link
Contributor

schmunk42 commented Dec 4, 2019

What steps will reproduce the problem?

See tests here: https://gitlab.com/yiisoft/yii2/-/jobs/368388365

What is the expected result?

No failure or error.

What do you get instead?

There was 1 failure:
573 1) yiiunit\framework\i18n\FormatterNumberTest::testIntlAsScientific
574 Failed asserting that two strings are identical.
575 --- Expected
576 +++ Actual
577 @@ @@
578 -8.76543210987654E16
579 +8.765432109876544E16
580 /project/tests/framework/i18n/FormatterNumberTest.php:531
581 /project/vendor/phpunit/phpunit/phpunit:52

Additional info

Q A
Yii version master
PHP version 7.1.33
Operating system Debian (Docker)

Looks like there's one more digit now?!
Test fails since https://gitlab.com/yiisoft/yii2/commit/c1e9739ed232e207ff6920e90b7351b2b19f69d0

Pipelines: https://gitlab.com/yiisoft/yii2/pipelines

@schmunk42
Copy link
Contributor Author

Just noticed: Test worked on PHP 7.1.26

@samdark
Copy link
Member

samdark commented Dec 4, 2019

So is there a change between that and PHP 7.1.33 that causes one extra digit to appear?

@bizley
Copy link
Member

bizley commented Dec 4, 2019

Has precision setting been changed in php.ini?

@schmunk42
Copy link
Contributor Author

I triggered tests for all versions in question at https://gitlab.com/yiisoft/yii2-docker/pipelines

I have not changed anything in our files.

@schmunk42
Copy link
Contributor Author

Tests fail for 7.1.30

PHP Changelog says:

Version 7.1.30
30 May 2019
EXIF:
Fixed bug #77988 (heap-buffer-overflow on php_jpg_get16) (CVE-2019-11040).
GD:
Fixed bug #77973 (Uninitialized read in gdImageCreateFromXbm) (CVE-2019-11038).
Iconv:
Fixed bug #78069 (Out-of-bounds read in iconv.c:_php_iconv_mime_decode() due to integer overflow) (CVE-2019-11039).
SQLite:
Fixed bug #77967 (Bypassing open_basedir restrictions via file uris).

@schmunk42
Copy link
Contributor Author

Debian version has been changed:

> docker run --rm php:7.1.30-apache cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux 10 (buster)"
NAME="Debian GNU/Linux"
VERSION_ID="10"
VERSION="10 (buster)"
VERSION_CODENAME=buster
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"



> docker run --rm php:7.1.29-apache cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux 9 (stretch)"
NAME="Debian GNU/Linux"
VERSION_ID="9"
VERSION="9 (stretch)"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

and likely some system-lib. Which one would be responsible for that?

@samdark
Copy link
Member

samdark commented Dec 4, 2019

No idea :(

@bizley
Copy link
Member

bizley commented Dec 4, 2019

Most likely precision setting has been changed. Maybe we should adjust tests to not rely on OS default settings.

@schmunk42
Copy link
Contributor Author

It's the same...

> docker run --rm php:7.1.29-apache php -i | grep precision
precision => 14 => 14
serialize_precision => -1 => -1

> docker run --rm php:7.1.30-apache php -i | grep precision
precision => 14 => 14
serialize_precision => -1 => -1

@alex-code
Copy link
Contributor

I don't think the precision setting of PHP is used by NumberFormatter.

@diego-mathis
Copy link
Contributor

I would try:

'components' => [
 'formatter' => [
   'numberFormatterOptions'=>[
	\NumberFormatter::MIN_FRACTION_DIGITS => 0,
	\NumberFormatter::MAX_FRACTION_DIGITS => 14
   ]

Its rather strange

@bizley
Copy link
Member

bizley commented Dec 4, 2019

So... ICU version changed?

@schmunk42
Copy link
Contributor Author

Diff between ini .29 and .30

> diff ini-29 ini-30 --color

2c2
< PHP Version => 7.1.29
---
> PHP Version => 7.1.30


4,5c4,5
< System => Linux 3b72a58a22bc 5.4.1-arch1-1 #1 SMP PREEMPT Fri, 29 Nov 2019 13:37:24 +0000 x86_64
< Build Date => May  8 2019 03:29:15
---
> System => Linux 1c8166c72bc8 5.4.1-arch1-1 #1 SMP PREEMPT Fri, 29 Nov 2019 13:37:24 +0000 x86_64
> Build Date => Jul 13 2019 00:26:16


< cURL Information => 7.52.1
< Age => 3
---
> cURL Information => 7.64.0
> Age => 4


164,166c164,166
< SSL Version => OpenSSL/1.0.2r
< ZLib Version => 1.2.8
< libSSH Version => libssh2/1.7.0
---
> SSL Version => OpenSSL/1.1.1c
> ZLib Version => 1.2.11
> libSSH Version => libssh2/1.8.0


226c226
< iconv library version => 2.24
---
> iconv library version => 2.28


455,456c455,456
< OpenSSL Library Version => OpenSSL 1.1.0j  20 Nov 2018
< OpenSSL Header Version => OpenSSL 1.1.0j  20 Nov 2018
---
> OpenSSL Library Version => OpenSSL 1.1.1c  28 May 2019
> OpenSSL Header Version => OpenSSL 1.1.1c  28 May 2019


576c576
< SQLite3 module version => 7.1.29
---
> SQLite3 module version => 7.1.30


626,627c626,627
< Compiled Version => 1.2.8
< Linked Version => 1.2.8
---
> Compiled Version => 1.2.11
> Linked Version => 1.2.11

How do I get ICU versions?

@diego-mathis
Copy link
Contributor

Seems to me like a bug in Yii.
Php default is precision 14, but if intl is used - there is no default set. At least I cannot see any here:
https://www.php.net/manual/en/class.numberformatter.php

Maybe would be good to set defaults.

The info at the function asScientific - its not prominent enough.

* If not given, the number of digits depends in the input value and is determined based on
     * `NumberFormatter::MIN_FRACTION_DIGITS` and `NumberFormatter::MAX_FRACTION_DIGITS`, which can be configured
     * using [[$numberFormatterOptions]].
     * If the [PHP intl extension](https://secure.php.net/manual/en/book.intl.php) is not available, the default value depends on your PHP configuration.
     * If you want consistent behavior between environments where intl is available and not, you should explicitly
     * specify a value here.

@bizley
Copy link
Member

bizley commented Dec 4, 2019

How do I get ICU versions?

php -i | grep ICU

@alex-code
Copy link
Contributor

Changelog here for ICU 60 (http://site.icu-project.org/download/60) mentions the NumberFormatter code has been redone and the old api is now a wrapper to the new.

@schmunk42
Copy link
Contributor Author

Good find!

7.1.29
ii  libicu57:amd64                57.1-6+deb9u2                  amd64        International Components for Unicode

7.1.30
ii  libicu63:amd64                63.1-6                      amd64        International Components for Unicode

@schmunk42
Copy link
Contributor Author

@samdark How should this be fixed?
It's blocking all Docker image builds, since framework tests are also run there.

@diego-mathis
Copy link
Contributor

IMHO; either there should be a global default for precision, or the test should specify it.
I believe the test should specify it, as it depends on the underlying system.

https://github.com/yiisoft/yii2/blob/master/tests/framework/i18n/FormatterNumberTest.php#L531

@samdark
Copy link
Member

samdark commented Dec 5, 2019

Test specifying it sounds fine to me.

@samdark samdark added the status:ready for adoption Feel free to implement this issue. label Dec 5, 2019
@samdark
Copy link
Member

samdark commented Dec 5, 2019

@My6UoT9 want to do a pull request?

@diego-mathis
Copy link
Contributor

@My6UoT9 want to do a pull request?

Yes will do a pr tomorrow morning, but I do not have the time to locally test it, I have the yii2 project not setup in that way.
Might do that when I have more time available.

@diego-mathis
Copy link
Contributor

diego-mathis commented Dec 6, 2019

Interesting, phpunit did not need any setup, so I did run it too.. and got same issue:
Under Windows 10, with php 7.2 (from laragon)

yiiunit\framework\i18n\FormatterNumberTest::testIntlAsScientific
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-8.76543210987654E16
+8.765432109876544E16

E:\x\yii2\tests\framework\i18n\FormatterNumberTest.php:531
E:\x\yii2\vendor\phpunit\phpunit\phpunit:52

@samdark
Copy link
Member

samdark commented Jan 28, 2020

  1. It's not an issue in PHP. What it does based on intl source code is passing arguments to icu4c. That's it.
  2. The following may help to reveal differences (I suspect different scientific formats in different ICU versions):
$formatter = new NumberFormatter('en-US', NumberFormatter::SCIENTIFIC);
$formatter->setAttribute(NumberFormatter::MIN_FRACTION_DIGITS, 10);
$formatter->setAttribute(NumberFormatter::MAX_FRACTION_DIGITS, 10);

echo $formatter->format('87654321098765436');
echo "\n";
echo $formatter->getPattern();

@samdark
Copy link
Member

samdark commented Jan 28, 2020

Not only precision varies with ICU version. The interpretation of fraction digits does as well. 3 means 8.76E16 in one instance and 8.765E16 in another.

@samdark
Copy link
Member

samdark commented Jan 28, 2020

Overall conclusion is that the test is doing more harm than good. Going to mark it as always skipped.

@samdark samdark closed this as completed Jan 29, 2020
@samdark samdark removed the status:ready for adoption Feel free to implement this issue. label Jan 29, 2020
@samdark samdark added this to the infrastructure milestone Jan 29, 2020
@samdark
Copy link
Member

samdark commented Jan 29, 2020

@schmunk42 it's merged. Would you please handle Docker part?

schmunk42 added a commit to yiisoft/yii2-docker that referenced this issue Jan 29, 2020
@schmunk42
Copy link
Contributor Author

For reference, tested via trigger

curl -X POST \
     -F token=${GITLAB_YII2_DOCKER_TOKEN} \
     -F ref=master \
     -F "variables[DOCKERFILE_FLAVOUR]=debian" \
     -F "variables[PHP_BASE_IMAGE_VERSION]=7.4-apache" \
     -F "variables[TEST_YII_VERSION]=a777c2e8f69dc753f8a945b1dd54bbaaa1e9e66c" \
     https://gitlab.com/api/v4/projects/2858803/trigger/pipeline  

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants