laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.41k stars 10.99k forks source link

Tuples Insertion with float & double values lose the decimal portion #23850

Closed ardiefernandes closed 6 years ago

ardiefernandes commented 6 years ago

Description:

On insertions or updates, like when you use the save method from Eloquent model, float and double variables are losing their decimal part

Steps To Reproduce:

Starting by supposing that you have an accounting table in your database as it follows

Table Criation code:

CREATE TABLE IF NOT EXISTS `accounting` (
    `id` INT NOT NULL AUTO_INCREMENT,
    `description` VARCHAR(250) NOT NULL,
    `value` DECIMAL(25,2) NOT NULL,
    `bDeleted` TINYINT(1) NULL DEFAULT 0,
    PRIMARY KEY (`id`))
ENGINE = InnoDB

And that your Eloquent model and Controller have the following code:

Eloquent Model code:

namespace App;
use Illuminate\Database\Eloquent\Model as Model;
class accounting extends Model {
    protected $table = 'accounting';
    public $timestamps = false;
}

Controller code:

require_once FILE_INDEX_BASE_PATH.'/App/accounting.class.php';
class AccountingController extends Controller {
    public function __construct() {
        parent::__construct();
        $this->model = new accounting;
    }

    private function createDemoMovement() {
        $this->model->description = 'Monthly incomes';
        $this->model->value = (float) 128.75;
        $this->model->bDeleted = 0;
        $this->model->save();
    }

    public function viewAccounts() {
        $this->createDemoMovement();
        print_r($this->model->get());
    }
}

When sending a request to the viewAccounts operation from the Accounting controller, the expected output would be:

[{
    "id":1,
    "description":"Monthly incomes",
    "value":"128.75",
    "bDeleted":"0"
}]

But current output is:

[{
    "id":1,
    "description":"Monthly incomes",
    "value":"128.00",
    "bDeleted":"0"
}]

Notice that the current output shows how the decimal part is lost when the tuple is inserted into the table, and you can confirm it by checking directly into the database.

We found out that the issue might be related to the validation applied on the bindValues method, from illuminate\database\MySqlConnection.php file, where the data type bound to the value is defined on a ternary if as it follows:

 is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR

so, by changing the validation to:

is_int($value) ? PDO::PARAM_INT : PDO::PARAM_STR

we get the expected output for the given example, just by basically removing the "is_float" portion from the validation

staudenmeir commented 6 years ago

Please add the definition of the value column.

Do you have any casts, accessors or mutators in your accounting model?

ardiefernandes commented 6 years ago

Do you have any casts, accessors or mutators in your accounting model?

No, I don't have any cast, accessors nor mutations in my model. The code used to replicate the bug is as simple as the one I posted and there are no methods, functions or process in between the assingment of the value and writing in the database

About the definition of the value column, my apologize, missed it when writing the post

staudenmeir commented 6 years ago

It works for me.

Does it work if you use a float/column column in your table?

staudenmeir commented 6 years ago

And try using the decimal column with a string:

$this->model->value = '128.75';

When you use a decimal column, you should use a string in PHP. When you use a float/double column, you should use a float in PHP.

cmbf1989 commented 6 years ago

Even if you use string how does this condition make sense? is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR

Its basically saying, if its a float then pass on the type as INT when, clearly its not the same data type. Its pretty clear in the php documentation the data type of the constant.

staudenmeir commented 6 years ago

It's specific to MySQL: https://github.com/laravel/framework/pull/16069

ardiefernandes commented 6 years ago

I just tried on your recommendations and got the following results:

The problem is that our application is accounting oriented so setting a monetary destined field to float is totally discouraged due the lack of precission, and forcing all the variables to be casted as string before the insertion seems to be unpractical in my opinion taking into consideration that;

ardiefernandes commented 6 years ago

I just tested on two different database engines and found out that this issues only occurs with MariaDB engines, the one we are currently using is MariaDB 10.1.31

staudenmeir commented 6 years ago

Laravel doesn't officially support MariaDB.

For monetary values you should definitely use string in PHP. There's no point in using float with a decimal column. Use BCMath for calculations.

tillkruss commented 6 years ago

Or as integer.

ionutantohi commented 6 years ago

@ardiefernandes @tillkruss

What I think it was missed in this issue is the PDO option ATTR_EMULATE_PREPARES.

When this option is set to true on php7.2 the float values are saved without decimals.

On php 7.0 seems to work fine.

Indeed, the below "if" located in bindValues method in MySqlConnection.php is a bit odd. I think that is_float check should be removed?

is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR

tillkruss commented 6 years ago

@ionutantohi: Why should it be removed?

ionutantohi commented 6 years ago

Why would you set PDO::PARAM_INT for a float in the first place?

Until now, that check was harmful, I don't know yet what is causing this, but something changed from php7.0 to 7.2 or PDO

But now, with php7.2 it's causing issues in some conditions. An one of them is having PDO::ATTR_EMULATE_PREPARES = true

Steps to reproduce:

  1. create a fresh laravel project on a box with php 7.2
  2. set PDO::ATTR_EMULATE_PREPARES = true in config/database.php
  3. perform a insert within a table which contains a DOUBLE (12,2) column with a float value.

Output:

If is_float check will be removed, the float value will be inserted correctly, not matter of ATTR_EMULATE_PREPARES option, because in that method, it will default to PDO::PARAM_STR

staudenmeir commented 6 years ago

There's a reason for using PDO::PARAM_INT: https://github.com/laravel/framework/pull/16069

tillkruss commented 6 years ago

@ionutantohi: Cool, feel free to submit a PR with good test cases to avoid this change causing havoc.

ionutantohi commented 6 years ago

@staudenmeir I understand this, but a workaround it needed for both cases.

Here is what happened to me in real life.

I have a project started on Laravel 5.5 and php7.0. In config/databases.php I had ATTR_EMULATE_PREPARES = true

Inside the projected is a script which saves currency exchange rates multiple times a day.

We upgraded to laravel 5.6 and php7.2. All went ok until we spotted that the exchange_rates were not saved correctly.

Imagine what can happen to a billing project.

ionutantohi commented 6 years ago

Hi @themsaid @taylorotwell, I don't want to bother to much, but I would like to hear your opinion on this before continuing.

In the initial issue, the user compained that the decimals are lost when saved in database.

I experienced the same issue and it seems to be related to PDO::ATTR_EMULATES_PREPARE. When this is set to true, the above scenario happens.

On my project, I can't switch that to false because the database is on a remote server and having ATTR_EMULATES_PREPARE = true gives a performance boost.

Ie: Query from sessions table: ATTR_EMULATES_PREPARE=true: 5.9ms ATTR_EMULATES_PREPARE=false: 89.1ms database server is in the same LAN

Also, is seems to relate how a value is bound in MysqlConnection.php. A float value is bound as PDO::PARAM_INT and in correlation with ATTR_EMULATES_PREPARE=true produces the above issue.

If that check will be removed, I don't know yet the impact on this issue: https://github.com/laravel/framework/issues/16063

Here are some tests with current codebase of how a value is bound, including PARAM_INT for floats php7.0 ATTR_EMULATES_PREPARE=true : float is inserted correctly ATTR_EMULATES_PREPARE=false : float is inserted correctly

php7.2.4 ATTR_EMULATES_PREPARE=true : float looses decimals ATTR_EMULATES_PREPARE=false : float is inserted correctly

tested on both mysql 5.5/5.7

Thank you!

mdmahendri commented 6 years ago

thanks @ionutantohi, i encounter this issue too because add ATTR_EMULATES_PREPARE. it is needed for 000webhost to work properly and then i move to another hosting without deleting the options. after deleting that, float saved without losing decimal. although i do not measure performance as it is not my focus (for now)

elerocks commented 6 years ago

any news on this? Or recommendation how to proceed? @themsaid Should I cast every single float value to string when I use laravel 5.6 with PHP 7.2 and ATTR_EMULATES_PREPARE=true?

Or second option - can I modify the line in MySqlConnection.php to is_int($value) ? PDO::PARAM_INT : PDO::PARAM_STR

instead of is_int($value) || is_float($value) ? PDO::PARAM_INT : PDO::PARAM_STR

geilt commented 6 years ago

It looks like this is a 7.2 Issue. In my Custom system I always used PARAM_INT with decimal. Suddenly on Upgrade now I have to go through my whole system and find the Decimal fields and update them to PARAM_STR or it wont save. Gotta love this unanounced change.

ionutantohi commented 5 years ago

For those who monitor this.

I made a pull request (#26817) containing a failing test to demonstrate the issue, but it was closed with the reason that we shouldn't use emulate prepares anymore. In my opinion this is wrong. Emulate prepares is an option in PDO which laravel doesn't handle it correctly on php 7.2

I tried to fix this but with no luck. The pull request which introduced this (#16069) has no tests and to be honest, I was afraid to not break something else.

vitordm commented 5 years ago

Change ATTR_EMULATE_PREPARES=true to ATTR_EMULATE_PREPARES=false does not solve the problem.

If the application use some "mysql view", it enables an "MySQL Bug" like: Prepared statement needs to be re-prepared MySQL

Please. remove this "is_float" from "MySQLConnection" class

ilinato commented 4 years ago

In PHP 7.2, the behavior of emulated queries was fixed, so it makes sense to remove the is_float check for MySQL https://bugs.php.net/bug.php?id=73234 https://bugs.php.net/bug.php?id=77954 @vitordm @driesvints @taylorotwell

CaliforniaMountainSnake commented 3 years ago

I have the same issue: Eloquent saves float as int. Php 7.4.3, lumen 8.2.3