Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » ext/curl update »

ext/curl update

php.internals

Pierrick Charron

10 years ago
Hi internals, I took some time to add some easy to implement new "features" that were implemented in libcurl but missing in ext/curl. Most of them are just exposing a new constant in ext/curl and dispatched in the curl_setopt function. I created a branch over master but the patch is applicable without conflict the 7.0 branch. https://github.com/php/php-src/compare/master...adoy:curl-update I was wondering since it's not a big patch and it only introduce new constants : - Should I commit this patch to 7.0 and master or only master - Should I do a RFC to include this patch ? Thanks for your feedback Pierrick

Davey

10 years ago
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. I'm working with Pierre Joye (CC'ed) to try and update the cli-server to use libnghttp2 to turn it into a fully fledged HTTP/2 compliant server with support for server push and things like multiplexing (see: http://wiki.php.net/rfc/cli_server_http2). Curl also uses libnghttp2 for it's HTTP/2 support. I hope to eventually (7.2+) use libnghttp2 to add an ext/nghttp2 HTTP client and update the HTTP streams layer to support HTTP/2 also. I'd welcome your collaboration on any of this. - Davey On Sat, Apr 23, 2016 at 12:30 PM, Pierrick Charron <pierrick@adoy.net> wrote:

Pierrick Charron

10 years ago
Hi Davey, Thanks for your answer. I still have small projects on my pipeline that I want to finish but I'll start looking at libnghttp2 and if I get some time I'll recontact you to maybe help you on remaining tasks :-) Do you have specifics stuffs that you would like me to look at ? About the patch if there is no objection i'll commit it to the 7.1 branch soon. Pierrick On 23 April 2016 at 20:24, Davey Shafik <davey@php.net> wrote:

Leigh

10 years ago
On Sun, 24 Apr 2016 at 01:25 Davey Shafik <davey@php.net> wrote:
> Hi Pierrick, > > This should be in master for 7.1, alongside my RFC'ed patch for server push > support.
I don't see why the additional constants that don't have a direct dependency on your server push patch can't be committed to 7.0 as well. The patch is pretty non-invasive. 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. >
The only new logic seems to be around CURLMOPT_PIPELINING_*_BL and CURLOPT_STREAM_DEPENDS(_E). I don't know what these do, are they related to server push? Is it possible to add deterministic tests for them? Maybe in 7.1 we could bump the minimum version of cURL required too. #ifdef version checks going back over 12 years seems unnecessary :)

Anatol Belski

10 years ago
Hi,
> -----Original Message----- > From: 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> > Cc: PHP internals <internals@lists.php.net>; 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? Regards Anatol

Pierrick Charron

10 years ago
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> wrote:

Anatol Belski

10 years ago
Hi,
> -----Original Message----- > From: 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> > Cc: Davey Shafik <davey@php.net>; PHP internals <internals@lists.php.net>; > 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> > wrote: > > > Hi, > > > -----Original Message----- > > From: 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> > > > Cc: PHP internals <internals@lists.php.net > <mailto:internals@lists.php.net> >; 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. Thanks anatol

Pierrick Charron

10 years ago
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> wrote:

Anatol Belski

10 years ago
> -----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

Pierrick Charron

10 years ago
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:

Pierrick Charron

10 years ago
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:

Davey

10 years ago
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: