ext/curl update View as Markdown

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

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

-----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

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