# ext/curl update - Thread: https://externals.io/message/92666 - Original archive: https://news-web.php.net/php.internals/92666 - Started by Pierrick Charron on 2016-04-23T19:30:59+00:00 - Messages: 14 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #92666 — Pierrick Charron — 2016-04-23T19:30:59+00:00 URL: https://externals.io/message/92666#92666 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 --- ## #92670 — Davey Shafik — 2016-04-24T00:24:46+00:00 URL: https://externals.io/message/92666#92670 · In reply to: #92666 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 wrote: > 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 --- ## #92672 — Pierrick Charron — 2016-04-24T03:03:30+00:00 URL: https://externals.io/message/92666#92672 · In reply to: #92670 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 > 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 > wrote: > > > […] --- ## #92725 — Leigh — 2016-04-25T11:31:48+00:00 URL: https://externals.io/message/92666#92725 · In reply to: #92670 > 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 :) --- ## #92832 — Anatol Belski — 2016-04-27T06:46:42+00:00 URL: https://externals.io/message/92666#92832 · In reply to: #92670 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 > Cc: PHP internals ; 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 --- ## #92837 — Pierrick Charron — 2016-04-27T12:19:56+00:00 URL: https://externals.io/message/92666#92837 · In reply to: #92832 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. > Hi, > > > […] > push > > […] > respond > > […] > added — and > > […] > which to > > […] > 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 --- ## #92841 — Anatol Belski — 2016-04-27T15:37:50+00:00 URL: https://externals.io/message/92666#92841 · In reply to: #92837 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 > Cc: Davey Shafik ; PHP internals ; > 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 > wrote: > > > Hi, > > > […] > [mailto:me@daveyshafik.com ] On Behalf Of > Davey > > […] > >; pajoye@php.net > > […] > push > > […] > respond > > […] > added — and > > […] > which to > > […] > 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 --- ## #92842 — Pierrick Charron — 2016-04-27T16:19:37+00:00 URL: https://externals.io/message/92666#92842 · In reply to: #92841 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... > Hi, > > > […] > Pierrick > > […] > almost all > > […] > have no > > […] > pierrick@adoy.net> > > > […] > pajoye@php.net> > > […] > server > > […] > just > > […] > be > > […] > against > > […] > there were > > […] > well and > > […] > 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 --- ## #92843 — Anatol Belski — 2016-04-27T16:55:29+00:00 URL: https://externals.io/message/92666#92843 · In reply to: #92842 > -----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 > Cc: Davey Shafik ; PHP internals ; > 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 > wrote: > > > Hi, > > > […] > [mailto:pierrick@webstart.fr ] On Behalf Of > Pierrick > > […] > > > > […] > internals >; > > […] > almost all > > […] > we have no > > […] > > > […] > wrote: > > […] > > > > […] > > ] On Behalf Of > > […] > > > > > […] > > > > […] > pajoye@php.net > > > […] > server > > […] > just > > […] > against > > […] > there were > > […] > as well and > > […] > > 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 --- ## #92855 — Pierrick Charron — 2016-04-27T23:14:29+00:00 URL: https://externals.io/message/92666#92855 · In reply to: #92843 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 > > > > […] > Pierrick > > […] > using > > […] > could > > […] > Of > > […] > since they > > […] > request/cache and > > […] > similar. > > […] > patch for > > […] > so I'll > > […] > that tests be > > […] > server > > […] > HTTP/2 > > […] > logic. If > > […] > 7.0 > > […] > with a > > […] > check > > […] > 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 --- ## #92856 — Pierrick Charron — 2016-04-27T23:15:06+00:00 URL: https://externals.io/message/92666#92856 · In reply to: #92855 Sorry for the 2 mails but I forgot to give you the URL : https://github.com/php/php-src/pull/1890/files > 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 > > > > > […] --- ## #92906 — Davey Shafik — 2016-04-28T21:30:18+00:00 URL: https://externals.io/message/92666#92906 · In reply to: #92856 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 > Sorry for the 2 mails but I forgot to give you the URL : > > https://github.com/php/php-src/pull/1890/files > > > > > […] --- ## #92961 — Anatol Belski — 2016-04-29T21:30:24+00:00 URL: https://externals.io/message/92666#92961 · In reply to: #92906 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 > Cc: Anatol Belski ; PHP internals > ; 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 > > > > > […] > have a good test. > > […] --- ## #93088 — Nikita Popov — 2016-05-05T13:32:17+00:00 URL: https://externals.io/message/92666#93088 · In reply to: #92906 > 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. > For the record, I'm fine with landing your server push patch based on review and manual tests only. It is not an ideal situation, but I don't think we should block this kind of trivial change on the implementation of full-blown HTTP 2.0 support in the built-in server. The latter is a much bigger project, I'm sure it's going to be somewhat controversial (pulls in new dependencies) and I don't see how this can land on the 7.1 timeline (unless someone is actively working on a patch already?) Nikita