Remove $age parameter of curl_version()

  105548
May 1, 2019 17:18 cmbecker69@gmx.de ("Christoph M. Becker")
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
  105549
May 1, 2019 17:40 bishop@php.net (Bishop Bettini)
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
  105554
May 2, 2019 07:58 cmbecker69@gmx.de ("Christoph M. Becker")
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]. [2] <https://github.com/curl/curl/blob/d1b5cf830bfe169745721b21245d2217d2c2453e/lib/version.c#L390-L463> -- Christoph M. Becker
  105566
May 2, 2019 17:00 bishop@php.net (Bishop Bettini)
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.
  105550
May 1, 2019 19:07 nikita.ppv@gmail.com (Nikita Popov)
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
  105553
May 2, 2019 04:13 pollita@php.net (Sara Golemon)
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
  105580
May 3, 2019 10:44 cmbecker69@gmx.de ("Christoph M. Becker")
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