PHPLIB-1147, PHPLIB-1148: Int64 improvements #1112

alcaeus · 2023-06-20T09:31:20Z

This PR removes the previous workarounds for creating Int64 instances through unserialise and adds a comparator for Int64 values. Using this comparator renders other workarounds for comparing Int64 instances obsolete.

I've built the comparator to only become active if at least one value is an Int64 instance and the other value is an int or numeric string. On 64-bit platforms we're comparing int values directly, whereas we compare string representations on 32-bit platforms to ensure correct behaviour for 64-bit values.

A note to contributors: if you have your own phpunit.xml file, make sure to update the bootstrap configuration as seen in phpunit.xml.dist. This is necessary to ensure that the new comparator is loaded.

GromNaN

Great idea to use this comparator. It's not even intrusive. I think it should be made available to the user of the library.

tests/Comparator/Int64Comparator.php

tests/bootstrap.php

GromNaN · 2023-06-20T11:20:49Z

tests/Comparator/Int64Comparator.php

@@ -0,0 +1,61 @@
+<?php
+
+namespace MongoDB\Tests\Comparator;


This class could be in the src directory, with namespace MongoDB\Test\Comparator. The need to compare Int64 also exist for the developers using this lib.

Per our discussion during the team meeting (after this comment was made), we can look into this down the line. A separate ticket could be used to track that.

@alcaeus and I previously discussed publishing various utility functions from our test suite for the benefit of downstream libraries. Various functions for detecting server functionality or comparing BSON documents might prove useful for things like Doctrine and the Laravel integration. I don't believe we have a ticket for that, so I'd suggest grouping all of these suggestions under a single epic to create a collection of shared code downstream test suites. Then the ticket for the Int64 comparator can be grouped under there, along with anything else we consider.

Understood, exposing such PHPUnit extension requires that we think about the supported PHPUnit versions compatibility promise. If a user feels the need for this comparator, please raise an issue or add a comment to the new task PHPLIB-1165.

GromNaN

Can be merged as this, once updated, to unlock the CI of other PRs.

jmikola · 2023-06-20T19:50:25Z

tests/Comparator/Int64Comparator.php

@@ -0,0 +1,61 @@
+<?php
+
+namespace MongoDB\Tests\Comparator;


Per our discussion during the team meeting (after this comment was made), we can look into this down the line. A separate ticket could be used to track that.

@alcaeus and I previously discussed publishing various utility functions from our test suite for the benefit of downstream libraries. Various functions for detecting server functionality or comparing BSON documents might prove useful for things like Doctrine and the Laravel integration. I don't believe we have a ticket for that, so I'd suggest grouping all of these suggestions under a single epic to create a collection of shared code downstream test suites. Then the ticket for the Int64 comparator can be grouped under there, along with anything else we consider.

tests/Comparator/Int64Comparator.php

jmikola · 2023-06-20T20:01:34Z

tests/Comparator/Int64Comparator.php

+        if (PHP_INT_SIZE == 8) {
+            // On 64-bit systems, compare integers directly
+            $expectedValue = (int) $expected;
+            $actualValue = (int) $actual;
+        } else {
+            // On 32-bit systems, compare integers as strings
+            $expectedValue = (string) $expected;
+            $actualValue = (string) $actual;
+        }
+
+        if ($expectedValue === $actualValue) {
+            return;
+        }


Is this necessary given how comparisons for Int64 work? Would it be preferable to use == here, or might that cause problems on 32-bit platforms?

bson-int64-compare-001.phpt doesn't test comparisons against other types, but I think this is handled via the cast handlers (also added in mongodb/mongo-php-driver/commit/b1ff48a00c831cd1f18d6ab9b94ec2c61002162d for PHPC-2214). Consider:

> 5 == new MongoDB\BSON\Int64(5) = true > 5.2 == new MongoDB\BSON\Int64(5) = false > '5.0' == new MongoDB\BSON\Int64(5) = true > 5.0 == new MongoDB\BSON\Int64(5) = true > '5.2' == new MongoDB\BSON\Int64(5) = false

My concern for 32-bit platforms is rooted in the string casting. Consider:

> (string) new MongoDB\BSON\Int64(5) === (string) '5.0' = false > (int) new MongoDB\BSON\Int64(5) === (int) '5.0' = true

assertEquals() should behave consistently on any platform.

This should also provide you with a bunch of new regression tests with fractional floats and float-like string. And if there's some way to test the 32-bit branch on 64-bit platforms, that might also help. As-is, I assume we have no actual test coverage of that code path.

alcaeus · 2023-06-21T07:05:12Z

Turns out I've completely overthought this. I've added more comparison tests in PHPC (see mongodb/mongo-php-driver#1439), and the only reason we're seeing these test failures is the order of comparators in PHPUnit:

when comparing a string with an object that supports __toString (which the Int64 class does), ScalarComparator is used,
when comparing an Int64 instance with any other value, a TypeComparator is used which obviously fails.

In this case, our comparator will become active as long as we're comparing an Int64 instance with another Int64 instance or numeric value. The rest is handled correctly by PHP itself using casts and type comparisons.

I've deliberately used 2**33 as test values here, as it's a 64-bit integer that also fits into a double without loss of precision, so we can test comparison with floats.

GromNaN · 2023-06-21T08:54:22Z

tests/Comparator/Int64Comparator.php

@@ -0,0 +1,61 @@
+<?php
+
+namespace MongoDB\Tests\Comparator;


Understood, exposing such PHPUnit extension requires that we think about the supported PHPUnit versions compatibility promise. If a user feels the need for this comparator, please raise an issue or add a comment to the new task PHPLIB-1165.

jmikola

One suggestion and a question, but LGTM.

jmikola · 2023-06-21T16:36:27Z

tests/Comparator/Int64ComparatorTest.php

+            'actualValue' => new Int64(123),
+        ];
+
+        yield 'Expects float, Actual Float' => [


Suggested change

yield 'Expects float, Actual Float' => [

yield 'Expects float, Actual float' => [

jmikola · 2023-06-21T16:37:43Z

tests/Comparator/Int64ComparatorTest.php

+
+        yield 'Expected Int64, Actual int' => [
+            'expected' => new Int64(8589934592),
+            'actual' => 8589934592,


I realize we don't test PHPLIB on 32-bit platforms, but is this integer literal problematic or would PHP interpret it as a float?

jmikola

Two more suggested changes:

tests/SpecTests/ClientSideEncryptionSpecTest.php defines a private createInt64() method, which is used in a few places.

tests/SpecTests/ClientSideEncryption/FunctionalTestCase.php also defines a protected createInt64() method, which is used in Prose22_RangeExplicitEncryptionTest.

Both of those can be factored out in this PR.

alcaeus requested review from jmikola and GromNaN June 20, 2023 09:31

alcaeus self-assigned this Jun 20, 2023

GromNaN reviewed Jun 20, 2023

View reviewed changes

GromNaN approved these changes Jun 20, 2023

View reviewed changes

jmikola reviewed Jun 20, 2023

View reviewed changes

alcaeus force-pushed the int64-improvements branch 2 times, most recently from 1fb1159 to be5d6da Compare June 21, 2023 07:01

alcaeus requested a review from jmikola June 21, 2023 07:05

GromNaN approved these changes Jun 21, 2023

View reviewed changes

jmikola approved these changes Jun 21, 2023

View reviewed changes

jmikola mentioned this pull request Jun 22, 2023

PHPLIB-947: Require 7.0+ for explain apiVersion bug fix #1111

Merged

jmikola reviewed Jun 22, 2023

View reviewed changes

alcaeus added 3 commits June 22, 2023 08:21

PHPLIB-1147: Create Int64 instances directly

5b5983a

PHPLIB-1148: Add comparator for Int64 objects

e61a5bd

Simplify comparison of Int64

20c1664

alcaeus force-pushed the int64-improvements branch from be5d6da to 20c1664 Compare June 22, 2023 06:21

alcaeus merged commit fe47d48 into mongodb:master Jun 22, 2023

alcaeus deleted the int64-improvements branch June 22, 2023 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1147, PHPLIB-1148: Int64 improvements #1112

PHPLIB-1147, PHPLIB-1148: Int64 improvements #1112

alcaeus commented Jun 20, 2023

GromNaN left a comment

GromNaN Jun 20, 2023

jmikola Jun 20, 2023

GromNaN Jun 21, 2023 •

edited

Loading

GromNaN left a comment

jmikola Jun 20, 2023

jmikola Jun 20, 2023

alcaeus commented Jun 21, 2023

GromNaN Jun 21, 2023 •

edited

Loading

jmikola left a comment

jmikola Jun 21, 2023

jmikola Jun 21, 2023

jmikola left a comment

	yield 'Expects float, Actual Float' => [
	yield 'Expects float, Actual float' => [

PHPLIB-1147, PHPLIB-1148: Int64 improvements #1112

PHPLIB-1147, PHPLIB-1148: Int64 improvements #1112

Conversation

alcaeus commented Jun 20, 2023

GromNaN left a comment

Choose a reason for hiding this comment

GromNaN Jun 20, 2023

Choose a reason for hiding this comment

jmikola Jun 20, 2023

Choose a reason for hiding this comment

GromNaN Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

GromNaN left a comment

Choose a reason for hiding this comment

jmikola Jun 20, 2023

Choose a reason for hiding this comment

jmikola Jun 20, 2023

Choose a reason for hiding this comment

alcaeus commented Jun 21, 2023

GromNaN Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

jmikola Jun 21, 2023

Choose a reason for hiding this comment

jmikola Jun 21, 2023

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

GromNaN Jun 21, 2023 •

edited

Loading

GromNaN Jun 21, 2023 •

edited

Loading