Re: Re: [RFC] OOP API for cURL extension

On Saturday, 17 February 2024 at 07:41, Kalle Sommer Nielsen <kalle@php.net> wrote: > Hi Sara > > Den tors. 15. feb. 2024 kl. 21.08 skrev Sara Golemon pollita@php.net: > > > * Better typing for setOpt() methods. > > Comment: Yep, this is a good and fair point. It's going to take a little more dicing > > up of the current implementation, but hopefully not too rough.. > > Proposal: Keep untyped setOption() method (1/ Easier migration of existing 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 there IS a setOptArray? > > Maybe we just don't mirror that one, or we call it setOptionMany(). Yeah, I like Many for the > > multiple option set, and Array for the 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. > > > 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): static { > if (is_int($option)) { $option = CurlOption::createFromConstant($option); } // ... } > > 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. > > 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. > 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 handle 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_ attempting 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