-
Notifications
You must be signed in to change notification settings - Fork 7.9k
fix bug 69587 (DateInterval properties and isset) #1877
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
It seems that the CI is broken for something else. |
I rebased master onto this pr, and the cr passed now. |
return retval; | ||
} | ||
|
||
zval rv; |
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.
move this declaration to top
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.
This is still not at the top, is it?
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.
The zval rv
still does not seem at the top.
Ping me when you're ready so I can have a review too please. |
hi, @derickr how's the review going? |
Code looks fine to me. |
It looks like the |
Yup, but I can fix that when merging the PR ;-) |
ZVAL_COPY(&tmp_member, member); | ||
convert_to_string(&tmp_member); | ||
member = &tmp_member; | ||
cache_slot = 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.
you could you zval_get_string like: https://github.com/php/php-src/blob/master/Zend/zend_object_handlers.c#L706
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.
well, I just followed the way converting strings in this file, should I replace them as well?
btw, it seems lxr.php.net is down for serveral days, is there any alternative?
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.
hmm, then nevermind, both way are right.
Merged. |
I guess this have also fixed https://bugs.php.net/bug.php?id=67621 |
fix bug 69587
there are some related bugs, and I'm working on them.