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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >> >>> >>> >>> > -----Original Message----- >>> > From: [email protected] [mailto:[email protected]] On Behalf Of >>> Pierrick >>> > Charron >>> > Sent: Wednesday, April 27, 2016 6:20 PM >>> > To: Anatol Belski <[email protected]> >>> > Cc: Davey Shafik <[email protected]>; PHP internals < >>> [email protected]>; >>> > [email protected] >>> > 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 <[email protected] >>> > <mailto:[email protected]> > wrote: >>> > >>> > >>> > Hi, >>> > >>> > > -----Original Message----- >>> > > From: [email protected] <mailto:[email protected]> >>> > [mailto:[email protected] <mailto:[email protected]> ] On >>> Behalf Of >>> > Pierrick >>> > > Charron >>> > > Sent: Wednesday, April 27, 2016 2:20 PM >>> > > To: Anatol Belski <[email protected] >>> > <mailto:[email protected]> > >>> > > Cc: Davey Shafik <[email protected] <mailto:[email protected]> >; PHP >>> > internals <[email protected] <mailto:[email protected]> >; >>> > > [email protected] <mailto:[email protected]> >>> > > 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 < >>> [email protected] >>> > <mailto:[email protected]> >>> > > <mailto:[email protected] <mailto:[email protected]> >>> > > >>> > wrote: >>> > > >>> > > >>> > > Hi, >>> > > >>> > > > -----Original Message----- >>> > > > From: [email protected] <mailto:[email protected]> >>> > <mailto:[email protected] <mailto:[email protected]> > >>> > > [mailto:[email protected] <mailto:[email protected]> >>> > <mailto:[email protected] <mailto:[email protected]> > ] On Behalf >>> Of >>> > > Davey >>> > > > Shafik >>> > > > Sent: Sunday, April 24, 2016 2:25 AM >>> > > > To: Pierrick Charron <[email protected] >>> > <mailto:[email protected]> <mailto:[email protected] >>> > <mailto:[email protected]> > > >>> > > > Cc: PHP internals <[email protected] >>> > <mailto:[email protected]> >>> > >>> > > <mailto:[email protected] <mailto: >>> [email protected]> > >; >>> > [email protected] <mailto:[email protected]> <mailto:[email protected] >>> > <mailto:[email protected]> > >>> > > > 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 >>> >>> >> >
