Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059
PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059
Changes from all commits
163f05a
684ca2c
dff423c
28bea34
cf85d8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does this imply the class is something we want documented?
I know we use
@api
elsewhere, but I've forgotten the motivation behind that. When we're thinking about preserving BC, I know everything in the library is implicitly part of the API unless we've annotated it with@internal
, so that might lead one to think@api
is redundant.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.
It may be superfluous, in which case I'd go through and remove instances of
@api
across the board. 👍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.
Opened https://jira.mongodb.org/browse/PHPLIB-1117.
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.
Any reason to use this over
foreach
? You're not actually modifying the array elements, so the callback doesn't seem necessary.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.
We may want to replace this with an
InvalidArgumentException
- for now this is fine.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 does
null
get special treatment? Likewise forcanEncode()
.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.
The idea was for each codec to decide how to handle a
null
value. For example, an ODM may decide that for a list of embedded documents it may not want to encodenull
as an empty array. If a single codec can't deal with anull
value, it should returnfalse
in itscanEncode
method.On second thought, the special handling for
null
seems misguided. Ifnull
deserves special treatment, other scalar values should probably enjoy the same. By that logic, it would make more sense to include a separateScalarValueCodec
that handles all scalar values that PHP offers, includingnull
and returns them as-is. This would also do away with theencodeOrReturn
topic in other places. Same goes for array, where a separate codec should support any array and traverse the array to encode all items within the array. I think that would make it more explicit.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.
Would ScalarValueCodec require additional changes in PHPC? If so, this may be a good opportunity to try and provide a way to differentiate 32-bit and 64-bit integers.
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.
IMO, the guard statement makes sense if we were doing something else here that would need a
break
afterwards, but that's not the case withreturn
. This just seems more concise unless you expect the body of this loop to get more complicated down the line.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.
Is this intentionally after the
foreach
since individual Decoders might handlenull
?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 is correct - it was thought as a fallback. I might end up refactoring this though, see my comment above.
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.
Same comments as I left on
decode()
apply here as well.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 assume "T" refers to a PHP value and "B" is for BSON. Can we use more descriptive names (e.g. TBson) like you did in AsListIterator?
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.
Are you adamant about using a RuntimeException here? If not, I think
InvalidArgumentException::invalidType()
would work well here.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.
Reading the documentation,
InvalidArgumentException
might indeed work better. I'll update this 👍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.
Currently codecs are designed to throw an exception if
encode
is called with an unsupported object. However, we may want to consider aencodeOrReturn
kind of method that contains this logic to avoid repeating this all over the place.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.
encodeOrReturn()
seems sensible.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.
Rather than call
array_values()
here, would it make sense to disregard offsets during iteration of the LazyBSONArray instance and just use$return[] = ...
to collect processed values?As I'm writing this, I've yet to read through
LazyBSONArray::getIterator()
but a quick glance there tells me that you're relying on AsListIterator to maintain the key sequence of a packed array.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.
🤦♂️
Yes, that seems like an easier way to accomplish the same thing. And yes,
AsListIterator
ensures the correct key sequence of a packed array. I'll update this and also give the whole stack of encoding the lazy array another look to ensure we're not needlessly iterating this multiple times.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.
Is this written as such to avoid constructing a CodecLibrary that might get discarded if
attachLibrary()
is called beforedecode()
orencode()
?Code style nit: I'm in favor of rewriting this with an explicit if statement to initialize the null property before an unconditional return.
Full disclosure: Avoiding one-liners in PHP from @localheinz was fresh on my mind.
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.
Originally I wanted to use a null-coalescing assignment, but those aren't available in 7.2. Your assumption however is correct: in the absence of a codec library (which we assume will be attached to the codec earlier) we create our own codec library consisting of the two codecs necessary to handle the lazy objects. I can refactor this if you'd like.
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.
The assignment within a
return
statement is really the thing that rubbed me the wrong way. That feels a bit like doing assignments within conditionals, which we also try to avoid.I wouldn't object to:
Once we require PHP 7.4+, I expect the CS tools would suggest a refactoring to use
??=
.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 recall that
Document::fromPHP()
accepts an array or object. If you were to pass the LazyBSONDocument instance directly, how wouldLazyBSONDocument::bsonSerialize()
differ from the explicit iteration/encoding you're doing here?Also, I noted that
PackedArray::fromPHP()
requires a list array (notarray|object
), so passing a LazyBSONArray directly isn't even a possibility; however, I suppose you could explicitly pass the result ofLazyBSONArray::bsonSerialize()
.I imagine there's a clear reason you're not doing this in both
encode()
implementations, but knowing why would help me better understand the codec design.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.
LazyBSONDocument::bsonSerialize
is implemented for compatibility toBSONDocument
, but it lacks the ability to support additional codecs (apart fromLazyBSONArrayCodec
). I'll give this another look though to see if I can break up the hard coupling from the document to the codec itself somehow. Either way, I'd rather not open up the ability to inject aCodecLibrary
into theLazyBSONDocument
instance and instead keep it as a lazy value holder while doing the heavy lifting in the codec.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'd appreciate the opportunity to discuss this over video, so please make a note to bring it up in our next 1:1.