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

[10.x] Improve decimal cast precision #45457

Closed

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Dec 30, 2022

Remove manual manipulation and require bcmath to use the decimal cast.

This should be noted in the 10.x upgrade guide.

@timacdonald
Copy link
Member Author

On hold until #45492 is merged.

@driesvints driesvints changed the base branch from master to 10.x January 5, 2023 13:43
@driesvints
Copy link
Member

@timacdonald I guess this can be continued to work on now that PR is merged?

@timacdonald timacdonald marked this pull request as ready for review January 10, 2023 01:36
@driesvints
Copy link
Member

@timacdonald I moved the bc-math requirement to "suggest" which is what you wanted to do I think.

@barryvdh
Copy link
Contributor

If it's in the suggest, then it crashes?
This is different then #45457 which checks if the extension is loaded. In this version for 10.x it always used bcmath

@driesvints
Copy link
Member

@barryvdh it's in "suggest" because you don't have to use the decimal cast per se. It's suppose to crash if you don't have it installed. That's why it's a "suggest" and not a hard dependency.

Also, I think you wanted to link to a different PR?

@barryvdh
Copy link
Contributor

If you don't want to require it, maybe better to keep the fallback? Maybe add an exception if the float precision is to low? Something like f66db46
(Removed the exception in #45580 for 9.x now)

@timacdonald timacdonald marked this pull request as draft January 10, 2023 22:10
@timacdonald timacdonald deleted the decimal-precision-bcmath branch January 11, 2023 22:23
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