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

[9.x] Fix decimal cast, again #45602

Merged
merged 3 commits into from
Jan 12, 2023
Merged

[9.x] Fix decimal cast, again #45602

merged 3 commits into from
Jan 12, 2023

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Jan 11, 2023

For 9.x, alternative to: #45580
For 10.x, Supersedes: #45457

This implementation handles both rounding and arbitrary sized floats, restoring the rounding functionality while addressing the large float issue.

It supports systems with bcmath or gmp, and delegates to pure PHP implementations when these are not found, meaning that it does not introduce any hard extension requirements to the framework.

This PR introduces brick/math in order to handle the rounding.

Some points to consider on introducing a new framework dependency...

Adding to the dependency tree

brick/math is already a sub-dependency of laravel/framework via ramsey/uuid. brick/math only requires the JSON extension and has no sub-dependencies.

Package is pre-1.0

From the readme:

While this library is still under development, it is well tested and considered stable enough to use in production environments.

This package is already used in brick/money, which is an extremely popular money handing library, second to moneyphp/money.

Screen Shot 2023-01-11 at 12 20 31 pm

Used for only one method

Introducing an entire dependency for a single method feels like a lot, however this library could be used to improve other parts of the framework.

  • The multiple_of rule could potentially drop it's dependency on the bcmath extension.
  • Addition improvements I've mentioned in our internal Slack.

Nice to have

I believe it is nice to have a strong math library in the framework anyway. Requiring it directly may lead others to the package and learn to improve their own handling of large numbers within their own applications.

@timacdonald timacdonald changed the title Fix decimal cast, again [9.x] Fix decimal cast, again Jan 11, 2023
@barryvdh
Copy link
Contributor

Yeah I think this is a lot cleaner, while not breaking BC :)

I understand the dependency concern, but if it's already loaded anyways.. Only thing I might consider is handling the exception differently, eg catch the bigmath exception and throw a Laravel exception. In that case you keep the implementation of bigmath internal, in case you need to swap it out later (or bigmath changes it).

@timacdonald
Copy link
Member Author

Good thinking. I have wrapped brick/math exceptions in a first party exception.

This does not meant it is not breaking if the underlying exception changes, as the "previous" exception will have changed and code may rely on its state, but at least it can continue to be caught via try / catch.

I've placed the exception in Illuminate\Support as the framework will need the exception in other parts of the framework, not just Illuminate\Database. Illuminate\Database required Illuminate\Support already.

@timacdonald
Copy link
Member Author

Thanks for the feedback and help on this @barryvdh

joshhanley added a commit to livewire/livewire that referenced this pull request Jan 15, 2023
* Update signature for UploadedFile::storeAs to match new Laravel signature

* Update composer.json

* Update test.yml

* Update session test to use version compare for anything over Laravel version 9

* Add version 10 to laravel/framework requirement

* Update version comparisons to not include any newer Laravel 9 versions or greater as this issue has been fixed/update again in the next version of Laravel 9, see laravel/framework#45602

Co-authored-by: Chris Lloyd <chrislloyd403@gmail.com>
Co-authored-by: Josh Hanley <josh@hitsystems.com.au>
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.

3 participants