Skip to content

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Jan 4, 2018

@jmikola jmikola requested a review from derickr January 4, 2018 23:23
Copy link
Member Author

Choose a reason for hiding this comment

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

Unlike the first test, this one omits the initial call to rewind() and tests that we preserve existing behavior by not requiring the rewind handler to ever be invoked (only relevant when wrapping a cursor with IteratorIterator).

Copy link
Member Author

Choose a reason for hiding this comment

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

Future improvement: refactor phongo_cursor_advance_and_check_for_error() so that it can take an optional bson_t *. Then phongo_execute_query() and php_phongo_cursor_iterator_rewind() could pass in null, since they only care about error checking and don't need to access the document. Meanwhile, php_phongo_cursor_iterator_move_forward() could pass in a bson_t * and, on success, use that to populate the current element, as is done with:

php_phongo_bson_to_zval_ex(bson_get_data(doc), doc->len, &cursor->visitor_data);

php_phongo.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It should not destroy the cursor, as phongo_cursor_advance_and_check_for_error already does that.

Copy link
Member Author

Choose a reason for hiding this comment

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

phongo_cursor_advance_and_check_for_error() no longer destroys the cursor. That change was made because we don't want to destroy it from within the iterator handlers.

php_phongo.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this down here made it impossible to see the changes in the diff, can you please put it back in its original location? That way, I would have spotted that you removed the mongoc_cursor_destroy(cursor); from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really unrelated to the patch, you just removed phongo_bulkwrite_init as it only did this one thing. I would suggest you don't do this in this single commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but shouldn't you destroy the cursor now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Once we create a MongoDB\Driver\Cursor with the mongoc_cursor_t, the only place we destroy the libmongoc cursor is from php_phongo_cursor_free_object().

@jmikola
Copy link
Member Author

jmikola commented Jan 6, 2018

The bulk write refactoring was split out into #731.

@jmikola jmikola merged commit d8d44f0 into mongodb:master Jan 8, 2018
jmikola added a commit that referenced this pull request Jan 8, 2018
@jmikola jmikola deleted the phpc-1050 branch January 8, 2018 15:27
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