PHPLIB-1122: Support Document and PackedArray objects in public APIs #1077
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alcaeus
reviewed
May 24, 2023
| /* Nested documents and arrays are intentionally left as BSON. We avoid | ||
| * iterator_to_array() since Document and PackedArray iteration returns | ||
| * all values as MongoDB\BSON\Value instances. */ | ||
| return $document->toPHP([ |
There was a problem hiding this comment.
I remember playing around with psalm return types based on the $typeMap parameter, but IIRC there were some issues comparing against the array keys. I've created PHPC-2227 to track adding stubs to psalm to ensure Psalm understands these classes.
In the meantime, consider adding these two errors to the baseline or storing the result of the toPHP call and using @var to let Psalm know that this will be an array.
There was a problem hiding this comment.
Done. I found a few more errors as well and fixed all but one (see the commit for the regenerated baseline).
jmikola
commented
May 25, 2023
GromNaN
approved these changes
May 25, 2023
GromNaN
approved these changes
May 25, 2023
jmikola
force-pushed
the
phplib-1122
branch
2 times, most recently
from
51ba28d
to
80d88d5
Compare
Excluding tests, most changes were concentrated in functions.php. A document_to_array() function was introduced to make it easier to access document values as arrays. This is mainly used by other utility functions (e.g. checking for dollar prefixed keys) but also ended up being used in the Find operation for handling the "modifiers" option. Adds tests for using Document and PackedArray objects in operations. Beyond handling of document/array types, this also filled gaps in test coverage for: * Validation for IndexInput key/name options * Passing a MongoDB\Driver\Command directly to DatabaseCommand * is_last_pipeline_operator_write()
Processing a Document and PackedArray will yield an array, so we can avoid a redundant Serializable check by using elseif.
…operator() This introduces a dependency on symfony/polyfill-php73 and bumps both polyfill packages to the latest minor version for consistency. The doc block for is_first_key_operator() was revised to note its additional purpose for validating pipeline stages.
This suppresses a new error in BulkWrite pertaining to a variable previously checked by is_first_key_operator() and is_pipeline(). ``` ERROR: MixedArgument - src/Operation/BulkWrite.php:341:45 - Argument 2 of MongoDB\Driver\BulkWrite::update cannot be mixed, expecting array<array-key, mixed>|object (see https://psalm.dev/030) $bulk->update($args[0], $args[1], $args[2]); The type of $args[1] is sourced from here - vendor/vimeo/psalm/stubs/CoreGenericFunctions.phpstub:154:12 * @return (TArray is array<empty,empty> ? false : (TArray is non-empty-array ? TValue : TValue|false)) ``` Other changes to the baseline are removals for obsolete errors.
alcaeus
approved these changes
May 26, 2023
|
CI failures are related to CSFLE QEv2 protocol changes, and two timeouts in CSFLE tests on Debian 9.2 (unrelated to this PR). I'll proceed with merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://jira.mongodb.org/browse/PHPLIB-1122
Excluding tests, most changes were concentrated in functions.php. A document_to_array() function was introduced to make it easier to access document values as arrays. This is mainly used by other utility functions (e.g. checking for dollar prefixed keys) but also ended up being used in the Find operation for handling the "modifiers" option.
Adds tests for using Document and PackedArray objects in operations.
Beyond handling of document/array types, this also filled gaps in test coverage for:
Addressed
Most changes were made to internal functions, which are used by many operations/classes. The following is a summary of test coverage changes in FunctionsTest:
document_to_array(): various types for$documentargument. This is used by many other internal functions and the Find operation.is_first_key_operator(): various types for$documentargument. This is used by operations that peform updates (e.g. BulkWrite, FindOneAndUpdate, Update).is_last_pipeline_operator_write(): testing more inputs and using a variety of types for stage documents. This is used by Aggregate (operation and helpers) and indirectly by the Watch operation.is_mapreduce_output_inline(): various types for$outargument. This is used by MapReduce.is_pipeline(): various types for$pipelineargument and stage documents within. This is used for validating update values forupdateandfindAndModifycommands.The following is a summary of added test coverage for various operations/classes. When an argument/option is listed without additional details, that means we've added test coverage for expressing the document in various forms (i.e. array, stdClass, Serializable, Document):
$filter$filterkeyoption. Additionally filled gaps in test coverage for validatingkeyandnameoptions (unrelated to documents)keyoptionnameoption fromkeyoption$command. Additionally added test coverage for passing aMongoDB\Driver\Commandobject constructed from various document types.$filter$filter$filter$filter,modifiersoption$filter,modifiersoptionfilterandqueryoptions$filter$filter,$replacement$filter,$update$replacementvalidation$updatevalidation$documentselements and_idgeneration/access$documentand_idgeneration/access$replacementvalidation$updatevalidation based onmultioption$filter,$update$filter,$replacement$filter,$update$filter,$update$updatevalidation$updatevalidationOmitted
The following are notable parts of PHPLIB that were not touched by this PR:
apply_type_map_to_document(): only used in GridFS on stdClass objects