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
Add Decimal128 type #2128
Add Decimal128 type #2128
Conversation
f8960d8
to
137ca5d
Compare
Please ignore CS issues for now, there's something fishy going on with my local instance of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, mapping should ensure that whenever a field is mapped with the Decimal128
type, it requires the ext-bcmath
extension to be present. Otherwise, the code will terminate with a fatal
error later down the line.
f011128
to
88a57bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor things, rest LGTM 👍
Can you create an issue to track the interface for version types? (see #2128 (comment))
private function typeRequirementsAreMet(string $type) : void | ||
{ | ||
if ($type === Type::DECIMAL128 && ! extension_loaded('bcmath')) { | ||
throw MappingException::typeRequirementsNotFulfilled(Type::DECIMAL128, 'ext-bcmath is missing'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the field and class names to the exception message will make it easier for users to spot which piece of their code is causing these errors.
Thanks Maciej! |
Summary
As per title, again ;)