Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122410
Date: Sat, 17 Feb 2024 13:27:09 +0000
To: Kalle Sommer Nielsen <kalle@php.net>
Cc: Sara Golemon <pollita@php.net>, PHP internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
Message-ID: <UUymSmDI3YE6ktBh8obrkpFqDXd4xL-XRhgl2nyqYAr-72-ccSVwu7Fj8WByDoifMk387X86qXCv9frpb-FNa8r8MMgGIetjLISmVl6apEw=@gpb.moe>
In-Reply-To: <CAJW__o3QKk+ERx0ydtGHdGTA9zBwabEY7vZgDwNwDHoPwEMvWQ@mail.gmail.com>
References: <CAESVnVp11-EVBrhzSj16=Nd3fYE366piq6fG-9wV3+iH8TxchA@mail.gmail.com> <CAESVnVpHRU8-Fc10XEdRkf3tU-5bXxO4tZ-CKwWJeUabB2LM4g@mail.gmail.com> <CAJW__o3QKk+ERx0ydtGHdGTA9zBwabEY7vZgDwNwDHoPwEMvWQ@mail.gmail.com>
Feedback-ID: 96993444:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: internals@gpb.moe ("Gina P. Banyard")

On Saturday, 17 February 2024 at 07:41, Kalle Sommer Nielsen <kalle@php.net=
> wrote:

> Hi Sara
>=20
> Den tors. 15. feb. 2024 kl. 21.08 skrev Sara Golemon pollita@php.net:
>=20
> > * Better typing for setOpt() methods.
> > Comment: Yep, this is a good and fair point. It's going to take a littl=
e more dicing up of the current implementation, but hopefully not too rough=
.
> > Proposal: Keep untyped setOption() method (1/ Easier migration of exist=
ing code, 2/ Some options may prove difficult to type), but add:
> > * CurlHandle::setOptionBool(int $option, bool $value): CurlHandle
> > * CurlHandle::setOptionInt(int $option, int $value): CurlHandle
> > * CurlHandle::setOptionString(int $option, string $value): CurlHandle
> > * etc... as needed, will this get weird when we get to Array since ther=
e IS a setOptArray? Maybe we just don't mirror that one, or we call it setO=
ptionMany(). Yeah, I like Many for the multiple option set, and Array for t=
he single option of array type.
> > Internally, the setopt handler will be split by type so that each typed=
 setting can call explicitly, and the untyped one can call all.
>=20
>=20
> What about making the CURL options an enumeration? It would allow us
> to typehint the setOptions() method and we can also make it backwards
> compatible with the existing global constants:
> `php public function setOption(CurlOption|int $option, mixed $value): sta=
tic { if (is_int($option)) { $option =3D CurlOption::createFromConstant($op=
tion); } // ... }`
>=20
> The enumeration would then have a static method to transform the
> global constant into a native type (which we can do internally).
> Naming is obvious just TBD. The biggest potential issue I can see with
> this approach is the many conditionally defined constants from older
> CURL versions/feature flags from compile time.
>=20
> From the many type variants of setOptions(), I am uncertain about
> that, because with an enumeration, we would still need to have some
> sort of whitelisting of what options can be passed into the method.
>=20

I was also going to suggest to use enums for the options, and have them be =
grouped by what value type they need.
This would simplify the C implementation a lot, as most of the logic to han=
dle the options and the value type would be done by the engine and ZPP.

I don't think it is _that_ problematic to have enum cases be conditionally =
defined as trying to use a case/constant that does not exist already throws=
 an Error in PHP 8 if the constant does not exist.

I will note that this approach _technically_ breaks the handling of options=
 that require a callables, as the callable is checked to exist _when_ attem=
pting to call it and not when actually passing it to curl_setopt.
But this should change anyway if I merge my PR which "fixes" this. [1]


Best regards,

Gina P. Banyard

[1] https://github.com/php/php-src/pull/13291