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

[6.x] Fix can't insert float into MySQL with PHP 7.3 #31100

Merged
merged 2 commits into from Jan 13, 2020

Conversation

@Chekote
Copy link
Contributor

Chekote commented Jan 10, 2020

Problem

When inserting a float value into MySQL via the Illuminate\Database package when using at least PHP 7.3, the value is converted to an int.

The bug has been reported a few times before:

Cause

PR #16069 introduced logic to the MySQLConnection that cast floats to ints to address a comparison problem with JSON columns, which was reported in issue #16063. This does not seem to cause a problem with PHP 7.1 and below but causes the float to lose it's decimal places when using PHP 7.3. I have not tested with PHP 7.2.

Bug Reproduction

Given we have the following MySQL table:

CREATE TABLE `zip_codes` (
  `zip_code` varchar(255) NOT NULL,
  `latitude` decimal(15,10) NOT NULL,
  `longitude` decimal(15,10) NOT NULL
)

When we execute the following using Artisan Tinker:

DB::table('zip_codes')->delete();

// Using the same values
$lat = -0.2;
$lon = 0;
$sql = 'INSERT INTO zip_codes (latitude, longitude, zip_code) VALUES (:lat, :long, :zip)';

// We insert the first by using PDO directly
$values = [':lat' => $lat, ':long' => $lon, ':zip' => 1];
DB::connection()->getPdo()->prepare($sql)->execute($values);

// Then the second by using Eloquent
DB::table('zip_codes')->insert(['latitude' => $lat, 'longitude' => $lon, 'zip_code' => 2]);

// Then pull them both.
DB::table('zip_codes')->get();

Then we should see both records with -0.2 as the latitude. But only the first has the value, and the second row is zero.

@Chekote Chekote force-pushed the Chekote:patch-1 branch from 95a8141 to 92737be Jan 10, 2020
When inserting a float value into MySQL via the Illuminate\Database package when using at least PHP 7.3, the value is converted to an int.

The bug has been reported a few times before:

* #30435
* illuminate/database#246

PR #16069 introduced logic to the MySQLConnection that cast floats to ints to address a comparison problem with JSON columns, which was reported in issue #16063. This does not seem to cause a problem with PHP 7.1 and below but causes the float to lose it's decimal places when using PHP 7.3. I have not tested with PHP 7.2.

Given we have the following MySQL table:

```
CREATE TABLE `zip_codes` (
  `zip_code` varchar(255) NOT NULL,
  `latitude` decimal(15,10) NOT NULL,
  `longitude` decimal(15,10) NOT NULL
)
```

When we execute the following using Artisan Tinker:

```php
DB::table('zip_codes')->delete();

// Using the same values
$lat = -0.2;
$lon = 0;
$sql = 'INSERT INTO zip_codes (latitude, longitude, zip_code) VALUES (:lat, :long, :zip)';

// We insert the first by using PDO directly
$values = [':lat' => $lat, ':long' => $lon, ':zip' => 1];
DB::connection()->getPdo()->prepare($sql)->execute($values);

// Then the second by using Eloquent
DB::table('zip_codes')->insert(['latitude' => $lat, 'longitude' => $lon, 'zip_code' => 2]);

// Then pull them both.
DB::table('zip_codes')->get();
```

Then we should see both records with -0.2 as the latitude. But only the first has the value, and the second row is zero.
@Chekote Chekote force-pushed the Chekote:patch-1 branch from 92737be to f6530aa Jan 10, 2020
@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 10, 2020

This comment is relevant to this PR: #23850 (comment)

@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 10, 2020

It seems as if the emulated prepared statements were broken prior to PHP 7.2, and that's why the is_float() check was required. After PHP 7.2, the check is not only no longer required but is now breaking the expected behavior.

What is the policy regarding logic that is required to work around a bug with a specific PHP version? Do we do a PHP version check in the code and apply the check conditionally?

@GrahamCampbell GrahamCampbell changed the title Fix can't insert float into MySQL with PHP 7.3 [6.x] Fix can't insert float into MySQL with PHP 7.3 Jan 11, 2020
@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 11, 2020

Laravel 6.x already requires PHP 7.2 or higher.

@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 11, 2020

OK, so it's an easy choice for Laravel 6.x. But what about the LTS: 5.5?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 11, 2020

Laravel 5 stopped receiving bug fixes when Laravel 6 was released. This included 5.5 LTS.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 11, 2020

Laravel 5.5 LTS will continue to get security fixes until the end of August this year. Laravel 6 is the new LTS release.

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 12, 2020

So we are expecting just deleting code to cause absolutely no problems? 🤔

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 12, 2020

We will need tests verifying this does break what it was intended to fix regarding JSON comparison. Do we have those?

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 12, 2020

Are you using emulate prepares?

@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 12, 2020

Yes, I am expecting that. The code was added to address a problem with PDO emulated prepared statements prior to PHP 7.2, I do not believe it is required any longer.

The code being deleted was added by #16069. That was merged without requiring tests to confirm there was a problem and that the PR resolved it. I would normally write tests for my PR, but I took that as a precedence to indicate that this situation does not require a test.

I will have to look into writing tests.

Yes, I am using emulated prepares.

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 12, 2020

We will definitely want tests on this to confirm the problem fixed by #16069 is fixed before merging this.

@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 12, 2020

Do any of your existing test environments have access to a real MySQL database?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 12, 2020

Yes, the unit tests on travis run against a real mysql database.

@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 12, 2020

The only reference I can find to a MySQL connection in the test code is \Illuminate\Tests\Integration\Auth\ApiAuthenticationWithEloquentTest\ApiAuthenticationWithEloquentTest::getEnvironmentSetUp(). But it's using invalid credentials. Can you please tell me where to find the valid credentials?

@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 12, 2020

@Chekote Chekote force-pushed the Chekote:patch-1 branch 4 times, most recently from 2b48ee7 to 9bff3da Jan 12, 2020
@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 12, 2020

I'm getting an error in my test that I don't know how to resolve: Target class [config] does not exist.

I based my tests off of \Illuminate\Tests\Integration\Database\EloquentDeleteTest, and I can't see why that test would have the config service but my test does not. I'm guessing there's some config somewhere that I have to update to make the service available to my test class?

Thanks in advance for any help.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 12, 2020

It's because you are trying to resolve config from the container before the app has booted or after it's been cleared out.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 12, 2020

Replace tearDown with:

    protected function tearDown(): void
    {
        DB::table(self::TABLE)->truncate();

        parent::tearDown();
    }
@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 12, 2020

It's important you call the parent method after, since it clears the container out.

@Chekote Chekote force-pushed the Chekote:patch-1 branch from 9bff3da to 11b51fa Jan 12, 2020
@Chekote Chekote force-pushed the Chekote:patch-1 branch from 11b51fa to 850425d Jan 12, 2020
@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 12, 2020

Thanks so much! 🙏 I think it's working now.

@Chekote

This comment has been minimized.

Copy link
Contributor Author

Chekote commented Jan 12, 2020

@taylorotwell I've added tests. Please let me know if you need anything else.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Jan 13, 2020

Does this test fail without your changes?

@taylorotwell taylorotwell merged commit 850425d into laravel:6.x Jan 13, 2020
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Chekote Chekote deleted the Chekote:patch-1 branch Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.