Skip to content

PHPLIB-749: Support comment option on command helpers #925

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Jun 7, 2022
Merged

PHPLIB-749: Support comment option on command helpers #925

merged 17 commits into from
Jun 7, 2022

Conversation

aleksandr-rudo
Copy link
Contributor

@aleksandr-rudo aleksandr-rudo requested a review from jmikola May 10, 2022 13:46
@aleksandr-rudo aleksandr-rudo mentioned this pull request May 10, 2022
Merged
@aleksandr-rudo
Copy link
Contributor Author

According to the ticket, spec tests should be synced with mongodb/specifications@0fc77cd. All actual tests in master branch are already newer.

@jmikola
Copy link
Member

jmikola commented May 23, 2022

According to the ticket, spec tests should be synced with mongodb/specifications@0fc77cd. All actual tests in master branch are already newer.

Note that PHPLIB-749 has a "split from" relationship with many DRIVERS tickets. mongodb/specifications@0fc77cd is only the commit for DRIVERS-2189.

The latest commit related to the comment option looks to be for DRIVERS-2290. There may still be no new tests to sync, but if you'd like to double check you can ensure that the change-streams and crud tests are up-to-date for that commit. Those appear to be the only two specs across all related DRIVERS tickets that had unified tests to be synced.

jmikola
jmikola reviewed May 24, 2022
View reviewed changes
jmikola
jmikola reviewed May 24, 2022
View reviewed changes
jmikola
jmikola reviewed May 24, 2022
View reviewed changes
src/Operation/InsertOne.php Outdated
* Server versions between 3.6 and 4.2 only support string as comment,
* and providing a non-string type will result in a server-side error.
* Older server versions do not support comment for aggregate command at all,
* and providing one will result in a server-side error.
Copy link
Member

Choose a reason for hiding this comment

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

This appears to have been copied from the Aggregate operation class. Per the insert command docs, comment was never supported prior to 4.4.

Feel free to leave this as-is and I'll take care of it (and related docs) when merging the PR.

jmikola
jmikola reviewed May 24, 2022
View reviewed changes
docs/includes/apiargs-common-option.yaml Outdated
The comment can be any valid BSON type for server versions 4.4 and above.
Server versions between 3.6 and 4.2 only support string as comment,
and providing a non-string type will result in a server-side error.
Older server versions do not support comment for aggregate command at all,
Copy link
Member

Choose a reason for hiding this comment

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

Note that apiargs-common-option.yaml is a common include shared by various methods, but this is specific to the aggregate command.

@jmikola
Copy link
Member

jmikola commented May 24, 2022

I just noticed that the previous PR for estimatedDocumentCount (#926) introduced test failures for pre-4.4 servers. See: https://evergreen.mongodb.com/version/mongo_php_library_d620cc0bdbce3e82a697f485495f70614acd106b

I expect that is because some tests (e.g. countDocuments) were synced but not skipped, which otherwise should have been. This PR should resolve those failures.

@aleksandr-rudo
Copy link
Contributor Author

Current branch state (inherited from master branch):

@aleksandr-rudo
Copy link
Contributor Author

aleksandr-rudo commented May 24, 2022 •
edited
Loading

Need new crud spec tests for distinct helper. Therefore:
Synced with mongodb/specifications@TBD

@aleksandr-rudo aleksandr-rudo requested a review from jmikola May 25, 2022 21:28
@jmikola
Copy link
Member

jmikola commented May 26, 2022

Need new crud spec tests for distinct helper. Therefore:
Synced with mongodb/specifications@TBD

@aleksandr-rudo: this seems premature as the work for DRIVERS-2334 hasn't been published yet, and no distinct spec tests are present in this PR. I used that commit message format in #933 because I was actually committing tests from an un-merged specifications PR.

@jmikola
Copy link
Member

jmikola commented Jun 6, 2022

@aleksandr-rudo: Please merge aleksandr-rudo#1 when you get a chance.

jmikola
jmikola reviewed Jun 7, 2022
View reviewed changes
@jmikola jmikola changed the title PHPLIB-749: Add support for the comment field to selected helpers PHPLIB-749: Support comment option on command helpers Jun 7, 2022
@jmikola jmikola merged commit 32a1cf8 into mongodb:master Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmikola jmikola jmikola left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aleksandr-rudo @jmikola