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

Conversation

CraigHarley
Copy link
Contributor

According to the PHP Docs:
https://www.php.net/manual/en/language.types.float.php

You shouldn't compare two floats directly for equality.

This only manifests when you cast to a float, otherwise you'll fall into the:

return is_numeric($current) && is_numeric($original)
                && strcmp((string) $current, (string) $original) === 0;

Which works for floats fine.

@CraigHarley CraigHarley changed the base branch from 7.x to 6.x June 18, 2020 09:03
@driesvints driesvints changed the title originalIsEquivalent not correct for floats [6.x] originalIsEquivalent not correct for floats Jun 18, 2020
@GrahamCampbell GrahamCampbell changed the title [6.x] originalIsEquivalent not correct for floats [6.x] Fix originalIsEquivalent with for floats Jun 18, 2020
@CraigHarley CraigHarley changed the title [6.x] Fix originalIsEquivalent with for floats [6.x] Fix originalIsEquivalent with floats Jun 18, 2020
@taylorotwell taylorotwell merged commit 0a49b23 into laravel:6.x Jun 18, 2020
@@ -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 😄

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

Successfully merging this pull request may close these issues.

5 participants