Everything

php

Home » Project » Newsgroups » php.internals » Remove $age parameter of curl_version() »

Remove $age parameter of curl_version()

php.internals

Christoph Becker

7 years ago

Hi,

curl_version()[1] (of ext/curl) makes curl_version_info()[2] (of
libcurl) available to PHP userland.  The latter requires to pass an age
argument which usually is CURLVERSION_NOW, so that the information
returned by the runtime matches the declarations used during compile
time.  For C programs it is simply necessary to pass this information,
and it might make sense to pass something else than CURLVERSION_NOW, but
the PHP wrapper assumes that the return value of curl_version_info()
matches the compile time declarations anyway, so passing anything else
might give bad results.

Therefore I suggest to remove this parameter in the long run altogether.
For PHP 7.4 I suggest to deprecate using the parameter, and also to
ignore anything that is not CURLVERSION_NOW, and to raise a warning in
this case.  I.e. something like the following behavior:

// okay
curl_version();

// E_DEPRECATED: passing an argument is deprecated
curl_version(CURLVERSION_NOW);

// E_WARNING: argument ignored
curl_version($not_curlversion_now);

Thoughts?  Do I overlook something important?

[1] <https://www.php.net/manual/en/function.curl-version.php>
[2] <https://curl.haxx.se/libcurl/c/curl_version_info.html>

--
Christoph M. Becker

Bishop Bettini

7 years ago

On Wed, May 1, 2019 at 1:18 PM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

>
> curl_version()[1] (of ext/curl) makes curl_version_info()[2] (of
> libcurl) available to PHP userland.  The latter requires to pass an age
> argument which usually is CURLVERSION_NOW, so that the information
> returned by the runtime matches the declarations used during compile
> time.  For C programs it is simply necessary to pass this information,
> and it might make sense to pass something else than CURLVERSION_NOW, but
> the PHP wrapper assumes that the return value of curl_version_info()
> matches the compile time declarations anyway, so passing anything else
> might give bad results.
>

It looks like the ext/curl binding [1] handles run-time age values that
differ from compile-time, as it makes checks for if (d->age). Am I missing
something?

> Therefore I suggest to remove this parameter in the long run altogether.
>  For PHP 7.4 I suggest to deprecate using the parameter, and also to
> ignore anything that is not CURLVERSION_NOW, and to raise a warning in
> this case.  I.e. something like the following behavior:
>
>    // okay
>   curl_version();
>
>   // E_DEPRECATED: passing an argument is deprecated
>   curl_version(CURLVERSION_NOW);
>
>   // E_WARNING: argument ignored
>   curl_version($not_curlversion_now);
>
> Thoughts?  Do I overlook something important?
>

Well, what about:

if (false === curl_version(3)) {
throw new \Exception('Please rebuild PHP with curl at least
version 7.16.1');
}

Here the developer has used age as a proxy to a major release point of
curl, as opposed to say:

if (version_compare(curl_version()['version'], '7.16.1', '<') { throw new
Exception...; }

[1]: https://github.com/php/php-src/blob/master/ext/curl/interface.c#L1824

Christoph Becker

7 years ago

On 01.05.2019 at 19:40, Bishop Bettini wrote:

> On Wed, May 1, 2019 at 1:18 PM Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
>
>> curl_version()[1] (of ext/curl) makes curl_version_info()[2] (of
>> libcurl) available to PHP userland.  The latter requires to pass an age
>> argument which usually is CURLVERSION_NOW, so that the information
>> returned by the runtime matches the declarations used during compile
>> time.  For C programs it is simply necessary to pass this information,
>> and it might make sense to pass something else than CURLVERSION_NOW, but
>> the PHP wrapper assumes that the return value of curl_version_info()
>> matches the compile time declarations anyway, so passing anything else
>> might give bad results.
>
> It looks like the ext/curl binding [1] handles run-time age values that
> differ from compile-time, as it makes checks for if (d->age). Am I missing
> something?

Indeed, as the binding is written, it's unlikely that bad results are
returned.  Still, using a newer "age" than available at compile time
will neither provide the PHP program more information, nor would using
an older "age" have real benefits.

>> Thoughts?  Do I overlook something important?
>
> Well, what about:
>
> if (false === curl_version(3)) {
>     throw new \Exception('Please rebuild PHP with curl at least
> version 7.16.1');
> }

It seems to me that wouldn't work anyway, since curl_version() only
returns false if curl_version_info() returned NULL, but that won't
happen[2].

> [1]: https://github.com/php/php-src/blob/master/ext/curl/interface.c#L1824

[2]
<https://github.com/curl/curl/blob/d1b5cf830bfe169745721b21245d2217d2c2453e/lib/version.c#L390-L463>

--
Christoph M. Becker

Bishop Bettini

7 years ago

On Thu, May 2, 2019 at 3:58 AM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> On 01.05.2019 at 19:40, Bishop Bettini wrote:
>
> > On Wed, May 1, 2019 at 1:18 PM Christoph M. Becker <cmbecker69@gmx.de>
>
> >> Thoughts?  Do I overlook something important?
> >
> > Well, what about:
> >
> > if (false === curl_version(3)) {
> >     throw new \Exception('Please rebuild PHP with curl at least
> > version 7.16.1');
> > }
>
> It seems to me that wouldn't work anyway, since curl_version() only
> returns false if curl_version_info() returned NULL, but that won't
> happen[2].
>
> > [1]:
> https://github.com/php/php-src/blob/master/ext/curl/interface.c#L1824
>
> [2]
> <
> https://github.com/curl/curl/blob/d1b5cf830bfe169745721b21245d2217d2c2453e/lib/version.c#L390-L463
> >
>

Indeed. In that case, I cannot see any value from the $age parameter. It
seems to be only an overly-literal functionality translation. I'm in favor
of removal.

Nikita Popov

7 years ago

On Wed, May 1, 2019 at 7:19 PM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> Hi,
>
> curl_version()[1] (of ext/curl) makes curl_version_info()[2] (of
> libcurl) available to PHP userland.  The latter requires to pass an age
> argument which usually is CURLVERSION_NOW, so that the information
> returned by the runtime matches the declarations used during compile
> time.  For C programs it is simply necessary to pass this information,
> and it might make sense to pass something else than CURLVERSION_NOW, but
> the PHP wrapper assumes that the return value of curl_version_info()
> matches the compile time declarations anyway, so passing anything else
> might give bad results.
>
> Therefore I suggest to remove this parameter in the long run altogether.
>  For PHP 7.4 I suggest to deprecate using the parameter, and also to
> ignore anything that is not CURLVERSION_NOW, and to raise a warning in
> this case.  I.e. something like the following behavior:
>
>    // okay
>   curl_version();
>
>   // E_DEPRECATED: passing an argument is deprecated
>   curl_version(CURLVERSION_NOW);
>
>   // E_WARNING: argument ignored
>   curl_version($not_curlversion_now);
>
> Thoughts?  Do I overlook something important?
>

Sounds good to me.

Nikita

Sara Golemon

7 years ago

On Wed, May 1, 2019 at 12:18 PM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

>
> curl_version()[1] (of ext/curl) makes curl_version_info()[2] (of
> libcurl) available to PHP userland.  The latter requires to pass an age
> argument which usually is CURLVERSION_NOW, so that the information
> returned by the runtime matches the declarations used during compile
> time.  For C programs it is simply necessary to pass this information,
> and it might make sense to pass something else than CURLVERSION_NOW, but
> the PHP wrapper assumes that the return value of curl_version_info()
> matches the compile time declarations anyway, so passing anything else
> might give bad results.
>
> Wow... yeah. That's an example of being far too idiomatic with the

bindings. I didn't even know we accepted that arg.  Kill it with fire.

-Sara

Christoph Becker

7 years ago

On 01.05.2019 at 19:18, Christoph M. Becker wrote:

> curl_version()[1] (of ext/curl) makes curl_version_info()[2] (of
> libcurl) available to PHP userland.  The latter requires to pass an age
> argument which usually is CURLVERSION_NOW, so that the information
> returned by the runtime matches the declarations used during compile
> time.  For C programs it is simply necessary to pass this information,
> and it might make sense to pass something else than CURLVERSION_NOW, but
> the PHP wrapper assumes that the return value of curl_version_info()
> matches the compile time declarations anyway, so passing anything else
> might give bad results.
>
> Therefore I suggest to remove this parameter in the long run altogether.
>  For PHP 7.4 I suggest to deprecate using the parameter, and also to
> ignore anything that is not CURLVERSION_NOW, and to raise a warning in
> this case.  I.e. something like the following behavior:
>
>    // okay
>   curl_version();
>
>   // E_DEPRECATED: passing an argument is deprecated
>   curl_version(CURLVERSION_NOW);
>
>   // E_WARNING: argument ignored
>   curl_version($not_curlversion_now);
>
> Thoughts?  Do I overlook something important?
>
> [1] <https://www.php.net/manual/en/function.curl-version.php>
> [2] <https://curl.haxx.se/libcurl/c/curl_version_info.html>

I have submitted PR #4106[3] to address this issue.  Of course, ignoring
anything else than CURLVERSION_NOW would be a BC break, and is as such
debatable.  Also, the deprecation should be followed by an actual
removal sometime in the future, so likely there should be a respective
RFC -- or can we do without in this case?

[3] <https://github.com/php/php-src/pull/4106>

--
Christoph M. Becker