Re: Re: ext/curl update

I seem to have created some confusion here: The reason _my_ patch for Server Push isn't merged is tests for it were requested and are blocking it. I'm not saying tests for these constants should be added. - Davey On Wed, Apr 27, 2016 at 4:15 PM, Pierrick Charron <pierrick@adoy.net> wrote: > Sorry for the 2 mails but I forgot to give you the URL : > > https://github.com/php/php-src/pull/1890/files > > On 27 April 2016 at 19:14, Pierrick Charron <pierrick@adoy.net> wrote: > >> Hi Anatol, >> >> I created a new patch from the one first published but this time this one >> target 7.0 and only expose new constants to that do not require any logic >> on the extension side. >> These constants are just exposed if they are available in the version >> installed and are bridge in the curl_setop function. >> >> If that's OK I'll commit this on 7.0 and merge it also on master. Then >> I'll work on adding new things that require logic and clean those for 7.1 >> and add tests if possible. >> >> Regards >> Pierrick >> >> On 27 April 2016 at 12:55, Anatol Belski <anatol.php@belski.net> wrote: >> >>> >>> >>> > -----Original Message----- >>> > From: pierrick@webstart.fr [mailto:pierrick@webstart.fr] On Behalf Of >>> Pierrick >>> > Charron >>> > Sent: Wednesday, April 27, 2016 6:20 PM >>> > To: Anatol Belski <anatol.php@belski.net> >>> > Cc: Davey Shafik <davey@php.net>; PHP internals < >>> internals@lists.php.net>; >>> > pajoye@php.net >>> > Subject: Re: [PHP-DEV] Re: ext/curl update >>> > >>> > Yep I'll check if I can add some test that could be easy to implement >>> using >>> > curl_easy_getinfo or using the php local server. Otherwise not sure we >>> could >>> > easily compile PHP with all those libcurl versions... >>> > >>> > >>> > On 27 April 2016 at 11:37, Anatol Belski <anatol.php@belski.net >>> > <mailto:anatol.php@belski.net> > wrote: >>> > >>> > >>> > Hi, >>> > >>> > > -----Original Message----- >>> > > From: pierrick@webstart.fr >>> > > <mailto:pierrick@webstart.fr> >>> > [mailto:pierrick@webstart.fr <mailto:pierrick@webstart.fr >>> >> ] On >>> Behalf Of >>> > Pierrick >>> > > Charron >>> > > Sent: Wednesday, April 27, 2016 2:20 PM >>> > > To: Anatol Belski <anatol.php@belski.net >>> > <mailto:anatol.php@belski.net> > >>> > > Cc: Davey Shafik <davey@php.net >>> > > <mailto:davey@php.net> >; PHP >>> > internals <internals@lists.php.net >>> > <mailto:internals@lists.php.net> >; >>> > > pajoye@php.net >>> > > <mailto:pajoye@php.net> >>> > > Subject: Re: [PHP-DEV] Re: ext/curl update >>> > > >>> > > I agree, but I don't really now how I could test those things >>> since they >>> > almost all >>> > > of the time only affect how libcurl will handle the >>> request/cache and >>> > we have no >>> > > way to retrieve options like curl_easy_getopt or something >>> similar. >>> > > >>> > > On 27 April 2016 at 02:46, Anatol Belski < >>> anatol.php@belski.net >>> > <mailto:anatol.php@belski.net> >>> > > <mailto:anatol.php@belski.net >>> > > <mailto:anatol.php@belski.net> >>> > > >>> > wrote: >>> > > >>> > > >>> > > Hi, >>> > > >>> > > > -----Original Message----- >>> > > > From: me@daveyshafik.com <: >>> > > > ;Ï‹„êè£)ßNÊÂmailto:me@daveyshafik.com> >>> > <mailto:me@daveyshafik.com >>> > <mailto:me@daveyshafik.com> > >>> > > [mailto:me@daveyshafik.com >>> > > <mailto:me@daveyshafik.com> >>> > <mailto:me@daveyshafik.com >>> > <mailto:me@daveyshafik.com> > ] On Behalf >>> Of >>> > > Davey >>> > > > Shafik >>> > > > Sent: Sunday, April 24, 2016 2:25 AM >>> > > > To: Pierrick Charron <pierrick@adoy.net >>> > <mailto:pierrick@adoy.net> >>> > <mailto:pierrick@adoy.net >>> > <mailto:pierrick@adoy.net> > > >>> > > > Cc: PHP internals <internals@lists.php.net >>> > <mailto:internals@lists.php.net> >>> > >>> > > <mailto:internals@lists.php.net >>> > > <mailto: >>> internals@lists.php.net> > >; >>> > pajoye@php.net <mailto:pajoye@php.net> >>> > <mailto:pajoye@php.net >>> > <mailto:pajoye@php.net> > >>> > > > Subject: [PHP-DEV] Re: ext/curl update >>> > > > >>> > > > Hi Pierrick, >>> > > > >>> > > > This should be in master for 7.1, alongside my RFC'ed >>> patch for >>> > server >>> > > push >>> > > > support. >>> > > > >>> > > > You emailed me directly about the aforementioned patch >>> so I'll >>> > just >>> > > respond >>> > > > here as it's relevant: >>> > > > >>> > > > The patch should hit in 7.1 but it has been requested >>> that tests be >>> > > added — and >>> > > > we can't add tests with a server push supporting >>> HTTP/2 server >>> > against >>> > > which to >>> > > > push. >>> > > > >>> > > As from the patch, many constants have nothing to do >>> with HTTP/2 >>> > > implementation and add just name/value without any further >>> logic. If >>> > there were >>> > > a reduced patch with only such cases, it would be acceptable >>> for 7.0 >>> > as well and >>> > > there were probably no collisions expected. What do you think? >>> > > >>> > >>> > So far I understood tests are exactly about HTTP2. Not sure how >>> you >>> > would tests all the constants present in libcurl. Would need to >>> rebuild with a >>> > dozen libcurl versions, but the documentation and compile time version >>> check >>> > are already reliable things. >>> > >>> But if you can fish out only the cases with name/value which don't >>> interfere with the HTTP2 work, so it's fine to add. OFC it were absurd to >>> recompile with all libcurl versions :) especially as an excellent >>> documentation to every option is presend on the cURL side. If that's only >>> an option that say affects the curl behavior and don't require any extra >>> handling, I don't think it is critical. On the other hand, if an option >>> requires some pre/post handling an thus some extra implementation - then >>> OFC it should be urgently suggested to have a good test. >>> >>> Thanks >>> >>> Anatol >>> >>> >> >