PHPLIB-1001, PHPLIB-1010, PHPLIB-1011: Improvements to $$type operator #991
Conversation
| @@ -167,6 +168,9 @@ private function doMatches($other): bool | |||
| case 'maxKey': | |||
| return $other instanceof MaxKeyInterface; | |||
|
|
|||
| case 'number': | |||
| return is_int($other) || (PHP_INT_SIZE == 4 && $other instanceof Int64) || is_float($other) || $other instanceof Decimal128Interface; | |||
There was a problem hiding this comment.
Out of curiosity, is there a reason for limiting the Int64 check to 32-bit platforms? Technically we won't have an Int64 instance on 64-bit platforms, but is there a practical reason for the check?
There was a problem hiding this comment.
This likely goes back to the legacy test format's DocumentMatchConstraint, from which we derived IsBsonType. The logic was as follows:
- On a 64-bit platform where PHP's
intis used for both BSON types, we can't differentiate. And it's incorrect to consider the numeric value, since a 32-bit value could certainly occupy a 64-bit integer in BSON and satisfy the server's evaluation of{ $type: "long" }. - On a 32-bit platform, PHPC only uses Int64 when necessary. Therefore, an Int64 will never be instantiated from a 64-bit BSON type containing a value within the range of 32-bit signed integers.
Given that last point, I don't think it ever made sense to consult PHP_INT_SIZE here; however, that never came up in practice because we use { $$type: [ "int", "long" ] } in all of our spec tests.
Some related side notes:
- The corresponding Matches constraint prepares 64-bit integers such that we should never have an Int64 type on a 64-bit platform
- PHPLIB-1001 only applies the the unified test runner, so DocumentMatchConstraint will continue to not support "number" in its
$$typeoperator implementation.
Having said all that, I agree that IsBsonType should not be strict about this. I'm also going to go back and relax the logic in DocumentMatchConstraint as well. In the interest of visibility, I'll address this in a separate ticket and add it to this PR.
| $undefined = toPHP(fromJSON('{ "x": {"$undefined": true} }'))->x; | ||
| $symbol = toPHP(fromJSON('{ "x": {"$symbol": "test"} }'))->x; | ||
| $dbPointer = toPHP(fromJSON('{ "x": {"$dbPointer": {"$ref": "db.coll", "$id" : { "$oid" : "5a2e78accd485d55b405ac12" } }} }'))->x; | ||
| $long = PHP_INT_SIZE == 4 ? unserialize('C:18:"MongoDB\BSON\Int64":38:{a:1:{s:7:"integer";s:10:"4294967296";}}') : 4294967296; |
There was a problem hiding this comment.
Our tests make the best case for providing a constructor for Int64. Can't wait to eventually get rid of it.
There was a problem hiding this comment.
Remember that a constructor would provide users a foot-gun allowing them to force 32-bit values to serialize as 64-bit, only to have BSON decoding convert them back to basic integers. Previous discussions on this:
- Howto insert with document validation of type "long". #589
- Integer types missing NumberLong mongo-php-driver#525
Unless we implement some BSON codec API that allows users to coerce integer serialization (e.g. always use 64-bit BSON types), I think the current behavior is less likely to confuse folks while noting we can't please everyone.
There was a problem hiding this comment.
I wasn't suggesting we add a constructor - apologies if this was misunderstood. I was alluding to getting rid of Int64 if and when we drop support for 32-bit platforms. It's wishful thinking at this point, but I hope that x86 goes out of style sometime this century ^^
| @@ -89,10 +94,20 @@ public function testAnyOf(): void | |||
| $c = IsBsonType::anyOf('double', 'int'); | |||
|
|
|||
| $this->assertResult(true, $c, 1, 'int is double or int'); | |||
| $this->assertResult(true, $c, 1.4, 'int is double or int'); | |||
| $this->assertResult(true, $c, 1.4, 'double is double or int'); | |||
There was a problem hiding this comment.
After I implemented PHPLIB-1010 to relax the Int64 logic I realized that the legacy $$type operator (a) did not have a default case for unsupported values and (b) never added support for an array of multiple types. There was also a separate bug where we used "boolean" instead of "bool" (actual type from the server operator).
The assertion method also had a string type hint (possibly added recently), so I imagine we'd have encountered an error attempting to cast an array to a string if we ever encountered a test using multiple types.
Anyway, this seemed like a good time to just refactor the entire legacy constraint to use IsBsonType, which added the necessary functionality and deleted a fair bit of code in the process (including some legacy code paths for PHPUnit < 6.5).
Also revises instantiation of deprecated BSON types and fixes an assertion message.
Also add a default case to raise an exception for unsupported types, which were previously ignored.
This refactors DocumentsMatchConstraint to use the IsBsonType constraint from the unified test runner.
jmikola
changed the title
https://jira.mongodb.org/browse/PHPLIB-1001
https://jira.mongodb.org/browse/PHPLIB-1010
https://jira.mongodb.org/browse/PHPLIB-1011
Also revises instantiation of deprecated BSON types and fixes an assertion message.