[9.x] Improve decimal cast fix

[timacdonald]

This is option 1 of 2 for potential further fixes to the decimal model cast.

If you want to read up on what PHP considers a numeric string, check the docs.

Solution 1 (this PR)

A more comprehensive fix than the initial fix, taking the same approach.

This PR is important for two reasons:

1. The first PR I submitted uses padLeft instead of padRight for the precision (🤦‍♂️).
2. I believe that we should also handle string values when it comes to floats, as floats are often passed around and stored as strings, to not exceed overflows / lose precision of very large values.

Regarding 2 (handling strings), the additional improvements included in this PR are:

• If an exponential is passed through as a string, we will throw (this matches the behaviour of bcmath).

Note The original number_format approach was able to handle these as values would be cast to a float internally. We are unable to cast to a float as doing so may lose precision, which was the issue in the first place that we are trying to address here.

So the decimal cast no longer supports string based numbers using exponents, whether bcmath is installed or not.

If we do not throw an exception, we will end up with weird values, as "1e3" is considered a numeric string by PHP:

$model = new class extends Model
{
    protected $casts = [
        'amount' => 'decimal:20',
    ];
};
$model->amount = '1e3';

$model->amount;
// 1e3.00000000000000000000

• Correctly handle missing integer values, e.g. ".1"

Pros

• We get the full fix without requiring bcmath in a patch release.
• The return values are the same for both branches.

Cons

• I think the code wrangling we are doing speaks for itself in that regards 🤢
• Different exception types are thrown in the different code branches.

Solution 2 (#45494)

This solution introduces all the fixes and changes that this PR introduces, but only when bcmath is installed.

Otherwise it falls back to number_format.

Pros

• A much cleaner solution.

Cons

• The return value of the code branches is not the same in many cases. number_format will round numbers and cannot handle large numbers (see the original issue: Decimal loses precision because of internal cast to float #45454).
• Different exception types are thrown in the different code branches.

Laravel 10.x

As mentioned in the original PR, in Laravel 10.x I would like to reduce this to just use bcmath and no other implementations.

[joshhanley]

@timacdonald not sure yet if this is an issue for us or an issue with this PR, but we're now experiencing failing tests in Livewire's core, which were passing in Laravel 9.45.1, but failing when running against 9.46.0.

Have a look at the failing test results here https://github.com/livewire/livewire/actions/runs/3852570505/jobs/6564814421

And these are the two tests that are failing https://github.com/livewire/livewire/blob/4cc5dedaab1e9512efb4d528fde67df98e9b465a/tests/Unit/ModelAttributesCanBeCastTest.php#L157-L195

I believe our tests are correct with the rounding, but would appreciate if you can have a look if you get the chance.

Thanks! 🙂

[timacdonald]

Hey Josh,

Unfortunately this was a "fix" that was is also potentially breaking.

You can see the initial PR here: #45456 which outlines that the new solution, and the solution improved on here, no longer rounds values.

I'll re-flag this with Taylor so he is aware, but i'm not sure there is much we can do when dealing with unknown sized floats as we are just delegating to BCMath, which does not provide rounding.

[joshhanley]

@timacdonald ah right, ok that makes sense why it breaks our tests as they assume rounding!

Thanks I will see if I can wrap my head around the change, I think it will just mean updating our tests to not assume rounding.

We have tests matrix that tests multiple Laravel versions though, so we will probably need to add Laravel version checks to make sure our assertions assert for rounding/non-rounding depending on which version we're testing. Unless you've got any better ideas for handling it? 😅

Thanks for the quick response 🙂

[timacdonald]

No worries. A version check makes sense here I think.

[barryvdh]

I see that my initial implementation in #26173 might have been a bit simplistic, but I'm not sure if rounding is a problem? I guess I would expect rounding when losing precision?

The use-case was mainly to prevent inconsistencies when inserting/loading data from a decimal field in the database vs data that changed in that request. If I save 8.55 in a decimal 6,1 field (1 digit precision), it is store as 9, not as 8. It would be weird to changed that, right?

[timacdonald]

@barryvdh your initial implementation was fine. It is just floats being floats.

The issue is more complicated due to large floats, where need to rely on bcmath, which does not support rounding.

I personally feel supporting large numbers is more important than rounding.

[barryvdh]

Yeah maybe, but changing the rounding after 5 year could potentially have more effect for existing code then rounding handling large numbers which were broken already? Maybe we can handle both?

[barryvdh]

@timacdonald something like this? #45580
That restores rounding and throws an error when precision is to high. Or would it be better to just fail silently in 9.x