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

[5.7] Add decimal:<num> cast to Model #26173

Merged
merged 3 commits into from
Oct 23, 2018
Merged

[5.7] Add decimal:<num> cast to Model #26173

merged 3 commits into from
Oct 23, 2018

Conversation

barryvdh
Copy link
Contributor

Adds a decimal cast, to cast to a string with a set number of decimals. This makes it easier to preserve the type when updating the value and makes the isDirty() check work.
A decimal field is already returned as string when getting it from the database. Simplified example:

Old behavior:

$item = Item::first();
var_dump($item->amount); // (string) "2.00";
$item->amount = 2; 
var_dump($item->amount); // (int) 2;
var_dump($item->isDirty()); // true
$item->save();
$item->refresh();
var_dump($item->amount); // (string) "2.00"

This will allow to add this cast to the model:

    protected $casts = [
        'amount' => 'decimal:2',
    ];

In the example it will now always output (string) "2.00" and is no longer dirty.

Fixes #13742

Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

Lgtm. Can you fix the StyleCI failures though?

@@ -718,7 +718,7 @@ public function fromFloat($value)
}

/**
* Return a decimal as string
span class="pl-s1"> * Return a decimal as string.
Copy link
Member

Choose a reason for hiding this comment

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

Hah, I just experienced this bug as well with the new suggested changes feature from Github. It's still very beta :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:( Will do it manually :)

@staudenmeir
Copy link
Contributor

Can you please add a test?

@barryvdh
Copy link
Contributor Author

@staudenmeir Added some tests!

@taylorotwell taylorotwell merged commit da4a287 into laravel:5.7 Oct 23, 2018
@barryvdh barryvdh deleted the patch-4 branch October 24, 2018 09:21
@fridzema
Copy link
Contributor

I think this should be marked as a breaking change, i had to change al models using decimal cast.

barryvdh added a commit to barryvdh/docs that referenced this pull request Oct 25, 2018
@barryvdh
Copy link
Contributor Author

There was no decimal cast before? Or did you extend the method yourself?

@fridzema
Copy link
Contributor

Hmm strange i used it in my application for quite some time. Don't know how i get it in there in if it did anything tough.

@m-lotze
Copy link

m-lotze commented Oct 25, 2018

Great addition. I think it should allow to set $dec_point and $thousands_sep for number_format. In languages like german a comma is usually used for $dec_point. Or maybe these two variables could be set as a config value somehow.

@LucaRed
Copy link
Contributor

LucaRed commented Oct 26, 2018

Italian has the same problem: it uses dots as thousands separators and commas as decimal separators. However, I think it's ok to hardcode those defaults, because both PHP and SQL follow the US notation anyway.

@barryvdh
Copy link
Contributor Author

The current implementation follows the output from MySQL, so dots for decimals and no thousand seperators. If you want to format it for invoices etc, you should probably do that last. Otherwise you can't calculate etc with it.

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.

None yet

7 participants