On Saturday, 17 February 2024 at 07:41, Kalle Sommer Nielsen <ka...@php.net>
wrote:
> Hi Sara
>
> Den tors. 15. feb. 2024 kl. 21.08 skrev Sara Golemon poll...@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
