Re: ext/date arguments handling

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