filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
19.19k stars 2.95k forks source link

Money field and TextColunm->money() bug #3098

Closed HomaEEE closed 2 years ago

HomaEEE commented 2 years ago

Package

filament/filament

Package Version

v2.13

Laravel Version

v9

Livewire Version

v2

PHP Version

PHP 8.1

Bug description

Mysql field - $table->decimal('price', 10, 2)->nullable(); From field - TextInput::make('price')->mask(fn (TextInput\Mask $mask) => $mask->money('$', ',', 2)) Table field - TextColumn::make('price')->money('uah')->label('Цена')

In database I store value - 7.89 (its right) In Table - $ 0.08 (its error for display)

Снимок экрана 2022-07-13 в 00 29 57

But in form - $ 7.89 (its right)

Снимок экрана 2022-07-13 в 00 30 21

Steps to reproduce

No response

Relevant log output

No response

simonbuehler commented 2 years ago

you would need TextColumn::make('price')->money('uah', true)->label('Цена') to convert from your decimal 7.9 value ( = ~8 cents ) as it by default gets constructed from minor (https://github.com/akaunting/laravel-money#usage)

in the form no actual Money instance is created, its just a mask

can be closed imo

blite commented 2 years ago

This is actually not correct in the docs. The TextColumn encourages you to store as integer (which is correct). See https://filamentphp.com/docs/2.x/tables/columns#text-column

The examples on https://filamentphp.com/docs/2.x/forms/fields#input-masking

TextInput::make('cost')->mask(fn (TextInput\Mask $mask) => $mask->money('$', ',', 2))

Seem to expect you have stored your monetary type as decimal.

I will link to laravel cashier which is probably the most standard application we are going to get here: https://laravel.com/docs/9.x/billing#simple-charge

The charge method accepts the payment amount in the lowest denominator of the currency used by your application. For example, if customers are paying in United States Dollars, amounts should be specified in pennies.

Hence you should store money as integers. There likely are situations where decimals make more sense, but for the majority of people storing as integers is far superior, especially due to the likelihood they will let them end up as floats if they get passed to javascript. Decimal money values

IMO the examples should be congruent and work together or at least link to instructions for https://github.com/akaunting/laravel-money#usage that would make them so.

simonbuehler commented 2 years ago

i agree that for many standard usecases a integer value of the minor unit ( e.g. cents) is ok, but when handling stocks and exchange rate calculations or when handling multiple different currencies a higher decimal scale is needed than the scale the main currency provides. just for reference: https://github.com/brick/money is also a great library for handling currencies

blite commented 2 years ago

The issue I am having and that I believe op is reporting is that the

TextInput::make('price')->mask(fn (TextInput\Mask $mask) => $mask->money('$', ',', 2))

only works with decimals

while

TextColumn::make('price')->money('uah')->label('Цена')

works with integers.

As far as I can see the imask.js library doesn't support integer money. Which honestly means it either needs to get it added or we should seek a different library. The fact that I can't even find an issue about this on https://github.com/uNmAnNeR/imaskjs is somewhat worrying.

the demo app uses decimals https://github.com/filamentphp/demo/blob/main/database/migrations/2021_12_13_164519_create_shop_products_table.php#L27 which needs to be fixed.

Op likely copied the decimals from the demo app and doesn't understand that https://github.com/akaunting/laravel-money#usage (that column->money() uses is correctly expecting integers). The shortcut is probably to use https://github.com/akaunting/laravel-money#usage 's convert functionality, but it would still be not the best example to be giving everyone.

simonbuehler commented 2 years ago

imask.js has multiple issues and there are plans to replace it soon

blite commented 2 years ago

anyway I added the documentation pull request https://github.com/filamentphp/filament/pull/3190 which for now at least makes them in sync even if not ideal.