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
Upgrade for PHP 7.2 #21
Conversation
Upgraded packages & added strict typing Changes might cause a BC due to strict typing
* @param int $id | ||
*/ | ||
public function __construct($id) | ||
public function __construct(int $id) |
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.
This change will generate a lot of work to get compatible within the monolith
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.
I do not intend to ever make this compatible within the monolith, but only for use in other services.
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.
That would mean we'd need to get very creative with major versions if we want to introduce something with BC to the monolith and forever maintain a separate package version with backports, even when it gets upgraded to new PHP versions
$trait = $this->getMockForTrait(StringCollectionTrait::class); | ||
$trait->expects(self::once()) | ||
->method('toArray') | ||
->willReturn([0.00, $exception, 1337]); | ||
|
||
self::assertEquals(['0.00', (string) $exception, '1337'], $trait->asStrings()); | ||
self::assertEquals(['0', (string) $exception, '1337'], $trait->asStrings()); |
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.
This seems wrong, why has the key been altered?
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.
All these alterations are made because the upgraded package seems to handle the rounding different from before (as you can see via the passing tests, even though this seems wrong).
I haven't been able to pinpoint the exact cause, because the litipk/php-bignumbers
package is >= 7.0
, so the same version is used as before.
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.
But this "rounding" has nothing to do with php-bignumbers, does this comment refer to NumberTest
?
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.
But this "rounding" has nothing to do with php-bignumbers, does this comment refer to NumberTest?
Ah yes, didn't see this single comment wasn't about that class. This particular case should have never worked: https://3v4l.org/HVKaS , no idea how the test succeeded before.
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.
Odd, I would expect the decimals to be included in the conversion. I guess the new PHPUnit version is more strict in array comparison
{ | ||
self::assertEquals(new Number(666), (new Number(123))->add(new Number(543))); | ||
self::assertTrue((new Number(666.0))->equals((new Number(123))->add(new Number(543)))); | ||
self::assertEquals(new Number(666.666), (new Number(123.123))->add(new Number(543.543))); | ||
self::assertTrue((new Number(666.666))->equals((new Number(123.123, 14))->add(new Number(543.543)))); |
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.
Why add an explicit scale to this test-case? That should not be required for this case
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.
All these alterations are made because the upgraded package seems to handle the rounding different from before (as you can see via the passing tests, even though this seems wrong).
I haven't been able to pinpoint the exact cause, because the litipk/php-bignumbers package is >= 7.0, so the same version is used as before.
This will need to be looked at in detail then, because we do rely on this implicit natural scale behaviour. What is the result of the original test, without an explicit scale?
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.
This will need to be looked at in detail then, because we do rely on this implicit natural scale behaviour. What is the result of the original test, without an explicit scale?
I think your earlier remark on the PHPUnit version is the missing piece in the puzzle.
There is an optional $delta
parameter in the assertEquals
method, which defaults to 0.0
, thereby implicitly ignoring the decimals. Looks like that behaviour is altered. (perhaps this issues has something to do with it: sebastianbergmann/phpunit#3340).
What is the result of the original test, without an explicit scale?
The result was that it expected 666.666
but received 666.66600000000000
. The explicit scale can be dropped here, because the issue is solved via assertTrue
in combination with equals
.
If you dig deeper into the bignumbers code, you'll see that the 14 decimals behaviour is applied when calculations are done, but not directly after construction, which explains the above result.
{ | ||
self::assertEquals(new Number(111), (new Number(666))->divideBy(new Number(6))); | ||
self::assertEquals(new Number(111.1, 14), (new Number(666.6))->divideBy(new Number(6))); | ||
self::assertTrue((new Number(111.1))->equals((new Number(666.6))->divideBy(new Number(6)))); |
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.
Why replace only one case with assertTrue
?
Upgraded packages & added strict typing
Changes might cause a BC due to strict typing, therefore, a new major release would be necessary.