RE: [PHP-DEV] Re: ext/curl update

Hi Davey, > -----Original Message----- > From: me@daveyshafik.com [mailto:me@daveyshafik.com] On Behalf Of Davey > Shafik > Sent: Thursday, April 28, 2016 11:30 PM > To: Pierrick Charron <pierrick@adoy.net> > Cc: Anatol Belski <anatol.php@belski.net>; PHP internals > <internals@lists.php.net>; pajoye@php.net > Subject: Re: [PHP-DEV] 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. > Yes, I was also reading this, that the "tests" was related to your PR. Cleared out now, anyway. Regards anatol > - 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 > >>> > > > <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 > >>> > >>> > >> > >