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* On Mon, Sep 22, 2014 at 1:29 PM, Derick Rethans <der...@php.net> wrote: > Hi Florian, > > On Sun, 21 Sep 2014, Florian Margaine wrote: > > > 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. > > 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 >
