Hi list,
I specifically mean to ask @derick about this issue I'm having, since he is
the maintainer of ext/date.
I wrote this pull request for the DateTimeZone::getOffset method to accept
a DateTimeInterface instead of a DateTime object:
https://github.com/php/php-src/pull/831
Instead of using ZEND_ARG_INFO, I use ZEND_ARG_OBJ_INFO rather than relying
on zpp only. It makes the code consistent with the type hinting errors that
should arise, and also gives a correct reflection.
However, the rest of the code in this extension uses ZEND_ARG_INFO, only
throwing warnings when the arguments are not of the right kind.
Should I use ZEND_ARG_INFO and rely on zpp's warning, or should I use
ZEND_ARG_OBJ_INFO, and eventually translate the other methods to use it
too? This'd be out of this PR of course, but it makes sense to streamline
the methods of ext/date.
In my opinion, using ZEND_ARG_OBJ_INFO means that we're going to the right
direction: fixing the type hints and the reflection of the classes.
The PR could be backported to 5.* by using ZEND_ARG_INFO to avoid BC breaks.
Regards,
Florian Margaine
Hi Florian,
I specifically mean to ask @derick about this issue I'm having, since
he is the maintainer of ext/date.I wrote this pull request for the DateTimeZone::getOffset method to
accept a DateTimeInterface instead of a DateTime object:
https://github.com/php/php-src/pull/831Instead of using ZEND_ARG_INFO, I use ZEND_ARG_OBJ_INFO rather than
relying on zpp only. It makes the code consistent with the type
hinting errors that should arise, and also gives a correct reflection.However, the rest of the code in this extension uses ZEND_ARG_INFO,
only throwing warnings when the arguments are not of the right kind.Should I use ZEND_ARG_INFO and rely on zpp's warning, or should I use
ZEND_ARG_OBJ_INFO, and eventually translate the other methods to use
it too? This'd be out of this PR of course, but it makes sense to
streamline the methods of ext/date.
I think it should use ZEND_ARG_OBJ_INFO. However, I am not sure whether
we can do that in 5.x because of BC reasons. It's these tiny mistakes
that are the larger BC problems. So I would suggest that you make a PR
for 5.6 without ZEND_ARG_OBJ_INFO, and one for 7 with ZEND_ARG_OBJ_INFO.
I think the patch looks mostly good too. I would recommend you squash
the commits into a single commit though.
cheers,
Derick
Hi Derick,
Thanks for your answer!
Indeed, I did plan on doing another PR for 5.6 while keeping ZEND_ARG_INFO.
I'll review your notes and take care of them.
Regards,
Florian Margaine
Hi Florian,
I specifically mean to ask @derick about this issue I'm having, since
he is the maintainer of ext/date.I wrote this pull request for the DateTimeZone::getOffset method to
accept a DateTimeInterface instead of a DateTime object:
https://github.com/php/php-src/pull/831Instead of using ZEND_ARG_INFO, I use ZEND_ARG_OBJ_INFO rather than
relying on zpp only. It makes the code consistent with the type
hinting errors that should arise, and also gives a correct reflection.However, the rest of the code in this extension uses ZEND_ARG_INFO,
only throwing warnings when the arguments are not of the right kind.Should I use ZEND_ARG_INFO and rely on zpp's warning, or should I use
ZEND_ARG_OBJ_INFO, and eventually translate the other methods to use
it too? This'd be out of this PR of course, but it makes sense to
streamline the methods of ext/date.I think it should use ZEND_ARG_OBJ_INFO. However, I am not sure whether
we can do that in 5.x because of BC reasons. It's these tiny mistakes
that are the larger BC problems. So I would suggest that you make a PR
for 5.6 without ZEND_ARG_OBJ_INFO, and one for 7 with ZEND_ARG_OBJ_INFO.I think the patch looks mostly good too. I would recommend you squash
the commits into a single commit though.cheers,
Derick