Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:77458
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain margaine.com from 209.85.223.174 cause and error)
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.11.1409221225040.4267@whisky.home.derickrethans.nl>
References: <CABRjmp95+ZaYKvEv7wuaf4mbYS5LPOLBk77UWOUujdSjvUk8ew@mail.gmail.com>
 <alpine.DEB.2.11.1409221225040.4267@whisky.home.derickrethans.nl>
Date: Mon, 22 Sep 2014 13:58:33 +0200
Message-ID: <CABRjmp-7pwk5MdmQROZ8dXDwovL1HTNqQd91Htu4Uo_ZZBJ2qQ@mail.gmail.com>
To: Derick Rethans <derick@php.net>
Cc: PHP Developers Mailing List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a113491e4b7aa190503a62c79
Subject: Re: [PHP-DEV] ext/date arguments handling
From: florian@margaine.com (Florian Margaine)

--001a113491e4b7aa190503a62c79
Content-Type: text/plain; charset=UTF-8

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 <derick@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
>

--001a113491e4b7aa190503a62c79--