WIP - Issue #3631 - fix for Double data type when using MySQLi with languages that do not use '.' for the decimal point #3660

mmucklo · 2019-08-18T07:10:29Z

Q	A
Type	bug
BC Break	no
Fixed issues	#3631

Summary

Add a special ParameterType for Double
Detect floats in MysqliStatement and pass the 'd' as the parameter type (see: mysqli_stmt::bind-param)
Create two tests cases, one for the standard '.' version, and one that extends the same test case, but switches the language to de_DE and tests the ',' case.

tests/travis/install-language-de.sh

morozov · 2019-08-18T17:51:25Z

tests/Doctrine/Tests/DBAL/Functional/Types/DoubleTest.php

+        $result2 = $this->select(2);
+        $result3 = $this->select(3);
+
+        if (is_string($result1)) {


Why is if needed here? Are there different results expected from the same SELECT?

@morozov

Not all SQL drivers operate the same as I discovered while testing this.

PDO in most if not all cases seems to return floats as strings, while mysqli appears to return them as actual floats.

I discovered this while testing.

As I wanted the test to work on all drivers, I need to convert the ones that are strings into floats so that my comparisons properly work.

This could probably be done differently, I'm open to suggestions.

tests/Doctrine/Tests/DBAL/Functional/Types/DoubleTest.php

morozov · 2019-08-20T02:49:07Z

lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php

        foreach ($values as &$v) {
            $params[] =& $v;
+            // fix for issue #3631 - detect parameter types as they have to be bound differently


Why would we want to autodetect the type of untyped parameters? They should be bound as strings by design.

@morozov,

Removed this for now (was missing the case for double / d anyway, as I was still testing things out) - this is for a separate "part II" of the bug where another test fails when under de_DE when dealing with floats.

I may add it back later, or do a follow on PR once this is accepted.

morozov · 2019-08-20T02:49:30Z

lib/Doctrine/DBAL/Types/FloatType.php

@@ -29,4 +30,12 @@ public function convertToPHPValue($value, AbstractPlatform $platform)
    {
        return $value === null ? null : (float) $value;
    }
+
+    /**
+     * {@inheritdoc}


Is this necessary?

@morozov

Just following convention set in IntegerType - do you want me to remove the inheritdoc?:

dbal/lib/Doctrine/DBAL/Types/IntegerType.php

Line 38 in 9ff47e7

* {@inheritdoc}

morozov · 2019-08-20T02:51:28Z

tests/Doctrine/Tests/DBAL/Functional/Types/DoubleTest.php

+        $diff2 = abs($result2 - $value2);
+        $diff3 = abs($result3 - $value3);
+
+        $this->assertLessThanOrEqual(0.0001, $diff1, sprintf('%f, %f, %f', $diff1, $result1, $value1));


Where does 0.001 come from?

morozov · 2019-08-20T02:51:44Z

tests/Doctrine/Tests/DBAL/Functional/Types/DoubleTest.php

+        $result2 = $this->selectDouble($value2);
+        $result3 = $this->selectDouble($value3);
+
+        $this->assertSame(is_int($result1) ? 1 : '1', $result1);


There shouldn't be any conditionals in a test.

@morozov

Similar to my other comment, some drivers seem to return integers as strings in which case I have to either cast everything, or just detect.

I'm open to alternatives?

morozov · 2019-08-20T02:53:54Z

tests/Doctrine/Tests/DBAL/Functional/Types/DoubleTest.php

+            'SELECT val FROM double_table WHERE id = ?',
+            [$id],
+            0,
+            [ParameterType::INTEGER]


How is this relevant to the test? We're testing binding double values, not integers.

@morozov

As this is a test with Doubles, it's trying to test both insertion and result sets of the doubles to make sure what was inserted matches what's queried.

I can just test insertions / updates if you want things more concise. Although without querying the database, I may not know for sure if my insertion actually worked.

Please advise?

morozov · 2019-08-20T02:54:39Z

tests/Doctrine/Tests/DBAL/Functional/Types/LanguageDoubleTest.php

+use const LC_ALL;
+use function setlocale;
+
+class LanguageDoubleTest extends DoubleTest


Please avoid extending tests if possible. Specifically in this case, if you want to test the same code paths using different locales, use a data provider.

@morozov

Let me look into it.

morozov · 2019-08-20T02:55:11Z

tests/Doctrine/Tests/DBAL/Functional/Types/LanguageDoubleTest.php

+{
+    protected function setUp() : void
+    {
+        setlocale(LC_ALL, 'de_DE.UTF-8', 'de_DE', 'de', 'ge');


What if neither of the locales is available at the runtime? Should there be any teardown logic in the test?

@morozov

I can make a tear down.

For travis, every run has the de_DE language installed, but I suppose I could try to add detection logic here if it works.

… languages that do not use '.' for the decimal point

morozov · 2021-07-26T06:59:39Z

Closing as stale.

mmucklo force-pushed the issue/3631_part1 branch 3 times, most recently from 0208bae to dc71600 Compare August 18, 2019 08:21

mmucklo changed the title ~~Issue #3631 - fix for Double data type when using MySQLi with languages that do not use '.' for the decimal point~~ WIP - Issue #3631 - fix for Double data type when using MySQLi with languages that do not use '.' for the decimal point Aug 18, 2019

mmucklo force-pushed the issue/3631_part1 branch 6 times, most recently from becbb8a to 8ccef4d Compare August 19, 2019 07:00

morozov requested changes Aug 20, 2019

View reviewed changes

mmucklo force-pushed the issue/3631_part1 branch from 8ccef4d to 27f9174 Compare August 20, 2019 04:47

Issue doctrine#3631 - fix for Double data type when using MySQLi with…

3593ecb

… languages that do not use '.' for the decimal point

mmucklo force-pushed the issue/3631_part1 branch from 27f9174 to 3593ecb Compare August 20, 2019 04:54

Base automatically changed from master to 4.0.x January 22, 2021 07:44

morozov closed this Jul 26, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Issue #3631 - fix for Double data type when using MySQLi with languages that do not use '.' for the decimal point #3660

WIP - Issue #3631 - fix for Double data type when using MySQLi with languages that do not use '.' for the decimal point #3660

mmucklo commented Aug 18, 2019

morozov Aug 18, 2019

mmucklo Aug 20, 2019

morozov Aug 20, 2019

mmucklo Aug 20, 2019

morozov Aug 20, 2019

mmucklo Aug 20, 2019

morozov Aug 20, 2019

morozov Aug 20, 2019

mmucklo Aug 20, 2019

morozov Aug 20, 2019

mmucklo Aug 20, 2019

morozov Aug 20, 2019

mmucklo Aug 20, 2019

morozov Aug 20, 2019

mmucklo Aug 20, 2019

morozov commented Jul 26, 2021

WIP - Issue #3631 - fix for Double data type when using MySQLi with languages that do not use '.' for the decimal point #3660

WIP - Issue #3631 - fix for Double data type when using MySQLi with languages that do not use '.' for the decimal point #3660

Conversation

mmucklo commented Aug 18, 2019

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Jul 26, 2021