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

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

Closed
ardiefernandes opened this issue Apr 11, 2018 · 24 comments
Closed

Comments

@ardiefernandes
Copy link

ardiefernandes commented Apr 11, 2018

  • Laravel Version: 5.6.11
  • PHP Version: 7.2.3-1
  • Database Driver & Version: mysqlnd 5.0.12

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
Copy link
Contributor

staudenmeir commented Apr 11, 2018

Please add the definition of the value column.

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

@ardiefernandes
Copy link
Author

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
Copy link
Contributor

It works for me.

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

@staudenmeir
Copy link
Contributor

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
Copy link

cmbf1989 commented Apr 11, 2018

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
Copy link
Contributor

It's specific to MySQL: #16069

@ardiefernandes
Copy link
Author

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

  • By setting value column data type to float I get the same result as when it was decimal (the decimal part is lost on the insertion)
  • By casting the value attribute/variable to string I get the desired behavior, something that I already knew when analyzing the origin of this issue

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;

  • Before upgrading from Laravel 5.4 it was working smoothly
  • If any complementary operations were needed before the insertion, a string type variable would just trigger warnings all over PHP7.2 when trying to perform mathematical operations

@ardiefernandes
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

Or as integer.

@ionutantohi
Copy link

@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
Copy link
Contributor

@ionutantohi: Why should it be removed?

@ionutantohi
Copy link

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:

  • with PDO::ATTR_EMULATE_PREPARES = true, the decimals are not inserted
  • with PDO::ATTR_EMULATE_PREPARES = false, the value is inserted correctly

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
Copy link
Contributor

There's a reason for using PDO::PARAM_INT: #16069

@tillkruss
Copy link
Contributor

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

@ionutantohi
Copy link

@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
Copy link

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: #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
Copy link

mdmahendri commented Jun 6, 2018

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
Copy link

elerocks commented Aug 16, 2018

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
Copy link

geilt commented Sep 17, 2018

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
Copy link

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.

mfn added a commit to mfn/laravel-framework that referenced this issue Jul 3, 2019
Laravel will miss-behave in multiple ways with MySQL and PgSQL as documented in various issues and even PRs, because people try to get a fix in but _the_ recommendation right now is to *not* use it.

I figured it might save everyones time if ppl fill this out upfront because it's often takes some forth and back until users mention this.

See:
- laravel#29023
- laravel#23850
- laravel#25818
- laravel#27951
- laravel#28149
@vitordm
Copy link

vitordm commented Aug 12, 2019

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
Copy link

ilinato commented Oct 29, 2019

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

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

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

No branches or pull requests