Skip to content
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

PHPC-2165: Expose server error replies in BulkWriteResult #1385

Merged

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Nov 24, 2022

PHPC-2165

Includes an update to libmongoc 1.24-dev

I only added a simple test for getErrorReplies as this will be comprehensively tested by the unified spec tests in PHPLIB.

@alcaeus alcaeus requested a review from jmikola November 24, 2022 13:23
@alcaeus alcaeus self-assigned this Nov 24, 2022
@alcaeus alcaeus removed the request for review from jmikola November 24, 2022 13:31
@alcaeus alcaeus force-pushed the feature/phpc-2165-expose-error-replies branch from 11980cb to 57e52fa Compare November 25, 2022 09:39
@alcaeus alcaeus requested a review from jmikola November 25, 2022 09:45
@jmikola
Copy link
Member

jmikola commented Nov 30, 2022

@alcaeus feel free to discard the first commit and rebase on master now that #1388 has been merged.

@alcaeus alcaeus force-pushed the feature/phpc-2165-expose-error-replies branch from 57e52fa to de878fe Compare November 30, 2022 08:37
@alcaeus alcaeus force-pushed the feature/phpc-2165-expose-error-replies branch from de878fe to 3d52b8d Compare December 1, 2022 14:16
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.

Ran this locally with Valgrind to check for leaks and looks clear apart from some likely false positives related to bson-dsl.h.

@@ -26,6 +26,8 @@ ZEND_END_ARG_INFO()

#define arginfo_class_MongoDB_Driver_WriteResult_getWriteErrors arginfo_class_MongoDB_Driver_WriteResult_getUpsertedIds

#define arginfo_class_MongoDB_Driver_WriteResult_getErrorReplies arginfo_class_MongoDB_Driver_WriteResult_getUpsertedIds
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize gen_stub.php re-used arginfo definitions but I just came across this code:

/* If there already is an equivalent arginfo structure, only emit a #define */
if ($generatedFuncInfo = findEquivalentFuncInfo($generatedFuncInfos, $funcInfo)) { ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was occasionally confused by this as well. TBH, I stopped reviewing the generated files and instead review the stubs, relying on CI to catch any outdated arginfo files.

@alcaeus alcaeus merged commit 5f3503c into mongodb:master Dec 6, 2022
@alcaeus alcaeus deleted the feature/phpc-2165-expose-error-replies branch December 6, 2022 07:18
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.

None yet

2 participants