Add a NumberType that maps to the BcMath\Number value object#6686
Conversation
e8bb6fe to
cdfedeb
Compare
c75b9f2 to
7192b19
Compare
7192b19 to
f41da8e
Compare
f41da8e to
eff1362
Compare
| - name: "Install dependencies with Composer" | ||
| uses: "ramsey/composer-install@v3" | ||
| with: | ||
| composer-options: "--ignore-platform-req=php+" |
There was a problem hiding this comment.
This is necessary because of Psalm, right?
There was a problem hiding this comment.
Yes. I know, we need to kick it.
morozov
left a comment
morozov
left a comment
There was a problem hiding this comment.
Would it be possible to add some test coverage for value conversion? There is a functional BigIntTypeTest, so maybe add one for the new type as well.
| $originalValue = new Number('13.37'); | ||
| $dbValue = $this->processValue(Types::NUMBER, $originalValue); | ||
|
|
||
| self::assertSame(0, $originalValue->compare($dbValue)); |
There was a problem hiding this comment.
It took me a minute to understand why zero is expected. I'm curious if assertEquals() can be used instead.
There was a problem hiding this comment.
We cannot use assertEquals() because PHPUnit 10 would compare two Number objects by comparing all properties. That's a problem because new Number('13.37') and new Number('13.3700') should be considered equal despite all properties being different. And that's a problem for us because the database might decide to normalize decimals by stripping or padding trailing zeros.
That's an issue that could be fixed upstream however. I can look into this, but given PHPUnit's release schedule, that fix would land in PHPUnit 12 earliest.
For now, the compare() method or the overloaded operators == and <=> are the proper ways of checking if two numbers are equal. Our PHPCS rules would however fix assertTrue($a == $b) to assertEquals(), so I decided to not use that either. But I've alternated between compare() and <=>, so I've now changed all tests to consistently use the spaceship operator. The value range of that operator is defined as [-1, 0, 1], so maybe that makes things a little clearer.
There was a problem hiding this comment.
I've opened sebastianbergmann/comparator#121 to address this issue.
There was a problem hiding this comment.
Yeah, I think <=> makes it clearer since it's more visually distinct. Thanks for the explanation and addressing it upstream. I believe, ultimately, using assertEquals() will be the clearest approach as it's the standard.
eff1362 to
cf6a9fe
Compare
cf6a9fe to
8f39a40
Compare
derrabus
changed the title
NumberType that maps to the BCMath\Number value objectNumberType that maps to the BcMath\Number value object
The PR already contains two new |
|
There is a polyfill for BcMath\Number. Is it worth to use it? |
I would expect that my new type is compatible with that polyfill.
That's something you have to decide for yourself. 🙂 |
The previous inline coverage report shown quite a few places in the conversion code as uncovered, but probably it was a glitch. Now, the diff coverage is at 100%, so we're good. |
* 4.3.x: Enable platformOptions to be considered for ColumnDiff (PostgreSQL jsonb support) (doctrine#6693) Ignore new PHPStan errors Cleanup obsolete PHPStan ignore rules (doctrine#6697) Add a `NumberType` that maps to the `BcMath\Number` value object (doctrine#6686) PHPStan 2.1.1 (doctrine#6690) PHPUnit 10.5.39 (doctrine#6692) PHPUnit 9.6.22 (doctrine#6691) Bump doctrine/.github from 6.0.0 to 7.1.0 (doctrine#6649) Bump doctrine/.github from 5.3.0 to 6.0.0 (doctrine#6642)
* 4.3.x: Enable platformOptions to be considered for ColumnDiff (PostgreSQL jsonb support) (doctrine#6693) Ignore new PHPStan errors Cleanup obsolete PHPStan ignore rules (doctrine#6697) Add a `NumberType` that maps to the `BcMath\Number` value object (doctrine#6686) PHPStan 2.1.1 (doctrine#6690) PHPUnit 10.5.39 (doctrine#6692) PHPUnit 9.6.22 (doctrine#6691) Bump doctrine/.github from 6.0.0 to 7.1.0 (doctrine#6649) Bump doctrine/.github from 5.3.0 to 6.0.0 (doctrine#6642)
4 participants
Summary
PHP 8.4 introduces a new
Numberclass (RFC, php/php-src#13741) that feels like a perfect fit forDECIMALvalues. This PR introduces a newNumberTypeclass and a correspondingTypes::NUMBERconstant which maps decimal values toNumberobjects instead of plain strings.