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.

Sign up for GitHub

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 originalIsEquivalent with floats #33259

Merged
merged 2 commits into from
Jun 18, 2020
Merged

[6.x] Fix originalIsEquivalent with floats #33259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
972d393
with test
CraigHarley Jun 18, 2020
3b0ec9b
styleci
CraigHarley Jun 18, 2020
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
5 changes: 5 additions & 0 deletions src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,6 +1178,11 @@ public function originalIsEquivalent($key, $current)
} elseif ($this->hasCast($key, ['object', 'collection'])) {
return $this->castAttribute($key, $current) ==
$this->castAttribute($key, $original);
} elseif ($this->hasCast($key, 'float')) {
return bccomp(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think by using bccomp function, we need to require ext-bcmath into the composer.json file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This project has a depency on: moontoast/math.

So therefore the existence of ext-bcmath is already checked: https://github.com/moontoast/math/blob/master/composer.json#L14

Copy link
Contributor

Choose a reason for hiding this comment

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

@BassemN is correct though; that another dependency depends on something which is now ultimately a shared dependency is not enough.

This project has a depency on: moontoast/math.

It's only a suggested dependency of illuminate/support.

However you can use illuminate/database independent of Laravel (which I'm doing).

Therefore I believe that the dependency belongs into:

  • src/Illuminate/Database/composer.json
    as well as into
  • composer.json

Because it's now a hard requirement for Laravel 6.

Copy link
Member

Choose a reason for hiding this comment

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

We cannot add that dependency. I'd suggest we change this code to avoid using that function.

Copy link
Contributor

Choose a reason for hiding this comment

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

lol 😄

$this->castAttribute($key, $current),
$this->castAttribute($key, $original)
) === 0;
} elseif ($this->hasCast($key)) {
return $this->castAttribute($key, $current) ===
$this->castAttribute($key, $original);
Expand Down
18 changes: 18 additions & 0 deletions tests/Database/DatabaseEloquentModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,23 @@ public function testCleanAttributes()
$this->assertFalse($model->isClean(['foo', 'bar']));
}

public function testCleanWhenFloatUpdateAttribute()
{
$original = -16.666347;
$new = 20.1 - 36.766347;

// PHP isn't able to compare two floats using === reliably.
// See warning here:
// https://www.php.net/manual/en/language.types.float.php

$this->assertTrue($original !== $new);
$this->assertTrue(bccomp($original, $new) === 0);

$model = new EloquentModelStub(['castedFloat' => $original]);
$model->syncOriginal();
$this->assertTrue($model->originalIsEquivalent('castedFloat', $new));
}

public function testCalculatedAttributes()
{
$model = new EloquentModelStub;
Expand Down Expand Up @@ -1992,6 +2009,7 @@ class EloquentModelStub extends Model
protected $table = 'stub';
protected $guarded = [];
protected $morph_to_stub_type = EloquentModelSaveStub::class;
protected $casts = ['castedFloat' => 'float'];

public function getListItemsAttribute($value)
{
Expand Down