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

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

Closed
wants to merge 1 commit into from
Closed
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
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ before_install:

before_script:
- if [[ "$DB" == "mysql" || "$DB" == "mysqli" || "$DB" == *"mariadb"* ]]; then mysql < tests/travis/create-mysql-schema.sql; fi;
- tests/travis/install-language-de.sh

install:
- rm composer.lock
Expand Down
1 change: 1 addition & 0 deletions lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class MysqliStatement implements IteratorAggregate, Statement
ParameterType::NULL => 's',
ParameterType::INTEGER => 'i',
ParameterType::LARGE_OBJECT => 'b',
ParameterType::DOUBLE => 'd',
];

/** @var mysqli */
Expand Down
1 change: 1 addition & 0 deletions lib/Doctrine/DBAL/Driver/PDOStatement.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class PDOStatement extends \PDOStatement implements Statement
ParameterType::BINARY => PDO::PARAM_LOB,
ParameterType::LARGE_OBJECT => PDO::PARAM_LOB,
ParameterType::BOOLEAN => PDO::PARAM_BOOL,
ParameterType::DOUBLE => PDO::PARAM_STR,
];

private const FETCH_MODE_MAP = [
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/DBAL/ParameterType.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ final class ParameterType
*/
public const BINARY = 16;

/**
* Represents a double data type.
*/
public const DOUBLE = 17;

/**
* This class cannot be instantiated.
*/
Expand Down
9 changes: 9 additions & 0 deletions lib/Doctrine/DBAL/Types/FloatType.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Doctrine\DBAL\Types;

use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Platforms\AbstractPlatform;

class FloatType extends Type
Expand Down Expand Up @@ -29,4 +30,12 @@ public function convertToPHPValue($value, AbstractPlatform $platform)
{
return $value === null ? null : (float) $value;
}

/**
* {@inheritdoc}
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morozov

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

*/
public function getBindingType()
{
return ParameterType::DOUBLE;
}
}
123 changes: 123 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Types/DoubleTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\DBAL\Functional\Types;

use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Schema\Table;
use Doctrine\Tests\DbalFunctionalTestCase;
use function abs;
use function floatval;
use function is_int;
use function is_string;
use function microtime;
use function sprintf;

class DoubleTest extends DbalFunctionalTestCase
{
protected function setUp() : void
{
parent::setUp();

$table = new Table('double_table');
$table->addColumn('id', 'integer');
$table->addColumn('val', 'float');
$table->setPrimaryKey(['id']);

$sm = $this->connection->getSchemaManager();
$sm->dropAndCreateTable($table);
}

public function testInsertAndSelect() : void
{
$value1 = 1.1;
Copy link
Member

Choose a reason for hiding this comment

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

If these values are independent, they should be injected using a data provider.

$value2 = 77.99999999999;
$value3 = microtime(true);
Copy link
Member

Choose a reason for hiding this comment

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

How are these three values different? Do they test different code paths?


$this->insert(1, $value1);
$this->insert(2, $value2);
$this->insert(3, $value3);

$result1 = $this->select(1);
$result2 = $this->select(2);
$result3 = $this->select(3);

if (is_string($result1)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

$result1 = floatval($result1);
$result2 = floatval($result2);
$result3 = floatval($result3);
}

if ($result1 === false) {
mmucklo marked this conversation as resolved.
Show resolved Hide resolved
$this->fail('Expected $result1 to not be false');

return;
}
if ($result2 === false) {
$this->fail('Expected $result2 to not be false');

return;
}
if ($result3 === false) {
$this->fail('Expected $result3 to not be false');

return;
}

$diff1 = abs($result1 - $value1);
$diff2 = abs($result2 - $value2);
$diff3 = abs($result3 - $value3);

$this->assertLessThanOrEqual(0.0001, $diff1, sprintf('%f, %f, %f', $diff1, $result1, $value1));
Copy link
Member

Choose a reason for hiding this comment

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

Where does 0.001 come from?

$this->assertLessThanOrEqual(0.0001, $diff2, sprintf('%f, %f, %f', $diff2, $result2, $value2));
$this->assertLessThanOrEqual(0.0001, $diff3, sprintf('%f, %f, %f', $diff3, $result3, $value3));

$result1 = $this->selectDouble($value1);
$result2 = $this->selectDouble($value2);
$result3 = $this->selectDouble($value3);

$this->assertSame(is_int($result1) ? 1 : '1', $result1);
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be any conditionals in a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

$this->assertSame(is_int($result2) ? 2 : '2', $result2);
$this->assertSame(is_int($result3) ? 3 : '3', $result3);
}

private function insert(int $id, float $value) : void
{
$result = $this->connection->insert('double_table', [
'id' => $id,
'val' => $value,
], [
ParameterType::INTEGER,
ParameterType::DOUBLE,
]);

self::assertSame(1, $result);
}

/**
* @return mixed
*/
private function select(int $id)
{
return $this->connection->fetchColumn(
'SELECT val FROM double_table WHERE id = ?',
[$id],
0,
[ParameterType::INTEGER]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

);
}

/**
* @return mixed
*/
private function selectDouble(float $value)
{
return $this->connection->fetchColumn(
'SELECT id FROM double_table WHERE val = ?',
[$value],
0,
[ParameterType::DOUBLE]
);
}
}
17 changes: 17 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Types/LanguageDoubleTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\DBAL\Functional\Types;

use const LC_ALL;
use function setlocale;

class LanguageDoubleTest extends DoubleTest
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morozov

Let me look into it.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

parent::setUp();
}
}
8 changes: 8 additions & 0 deletions tests/travis/install-language-de.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#!/usr/bin/env bash

set -ex

echo "Installing language pack de..."

sudo apt -y -q update
sudo apt install language-pack-de