Skip to content

PHPC-2242, PHPC-2243, PHPC-2244: BSON class improvements #1435

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

Merged
merged 3 commits into from
Jun 20, 2023

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Jun 16, 2023

PHPC-2242
PHPC-2243
PHPC-2244

This PR cleans up the BSON class handling a bit. While working with the changes committed in #1412, I noticed that wrapping BSON values in MongoDB\BSON\Value instances was cumbersome and no longer necessary after changing the way int64 values are handled in raw BSON. To summarise, we decided that MongoDB\BSON\Value::getValue() would always return an Int64 instance, even when the value would fit within a platform-sized integer. That removed the main incentive for the MongoDB\BSON\Value class, as we originally planned to use it to differentiate between 32-bit and 64-bit integers when the value was within the 32-bit range.

With that in mind, this PR undoes some of the changes introduced in the pull request. The main change is that Document::get, PackedArray::get, and Iterator::current now return Int64 instances whenever a BSON long is encountered. For one, the raw BSON classes are designed to interact with BSON structures, so returning BSON values that can't be represented as PHP scalars as BSON class instances makes sense. Furthermore, with the recent changes to the Int64 class (see #1417), instances of that class can be used like numbers in arithmetic operations and cast to integer if a user wishes to do so.
This change also applies to Document::toPHP and PackedArray::toPHP, which return BSON long values as Int64 instances. The already existing MongoDB\BSON\toPHP function retains its current behaviour, as do any other instances of where we convert BSON data to PHP. The new behaviour only applies when interacting with the new BSON Document and PackedArray classes.

Last but not least, the MongoDB\BSON\Value class was dropped without a replacement. The improvements and refactoring made during the creation of the class remain in place.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion but LGTM.

var_dump($document->get('foo'));
var_dump($document->get('bar'));
var_dump($document->get('int64'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted that you've added tests for always returning an Int64.

@@ -0,0 +1,23 @@
--TEST--
MongoDB\BSON\toPHP(): int64 values are returned as int scalars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll note that 64-bit values on a 32-bit platform are an edge case where Int64 objects would still be returned, but I don't think we need to demonstrate that in a test. If you want you can add a note above 'int64' => new MongoDB\BSON\Int64(123), explaining that you're using a 32-bit value to simplify testing.

@alcaeus alcaeus force-pushed the bson-class-improvements branch from ee27b14 to b0cea97 Compare June 20, 2023 06:49
@alcaeus alcaeus merged commit b9fe58f into mongodb:master Jun 20, 2023
@alcaeus alcaeus deleted the bson-class-improvements branch June 20, 2023 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants