-
Notifications
You must be signed in to change notification settings - Fork 209
PHPC-1713: Ensure Cursor::current returns null on invalid positions #1186
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
Conversation
src/MongoDB/Cursor.c
Outdated
|
||
if (data) { | ||
ZVAL_COPY_DEREF(return_value, data); | ||
if (Z_ISUNDEF_P(data)) { |
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.
At first glance, I wondered if it would be possible for this function to still leave return_value
unset.
php_phongo_cursor_get_current_data
always returns a non-null pointer to a block of memory within intern
, which itself is never null (we'd hit a segfault if so). In that case, I think can should drop the outer if (data)
for readability -- optionally leave a comment that "data points to some memory within intern and is always non-null".
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.
Removed the check 👍
tests/cursor/bug1713-001.phpt
Outdated
@@ -0,0 +1,24 @@ | |||
--TEST-- | |||
MongoDB\Driver\Cursor PHPC-171: The cursor current method does not return anything |
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.
By convention, the JIRA ticket is used as the test name prefix. Non-ticket tests start with a class/method name. I'd suggest:
PHPC-1713: MongoDB\Driver\Cursor::current() does not return anything
tests/cursor/bug1713-001.phpt
Outdated
--SKIPIF-- | ||
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?> | ||
<?php skip_if_not_live(); ?> | ||
<?php skip_if_not_clean(); ?> |
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.
Technically, I don't think this matters since we don't actually care if the collection is dropped. Whether it exists or not, we're just testing that php_phongo_cursor_t.visitor_data.zchild
is zeroed out before the first call to rewind()
or next()
.
That said, no objections to keeping this in 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.
Noted. I've dropped the call, no reason to clear the database if we don't care about its contents.
@jmikola realised I (once again) targeted the wrong branch with the PR. Will rebase onto v1.9 and merge after that. |
PHPC-1713
Fixes #1183.