Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
On 18 February 2024 15:26:37 GMT, Lynn wrote: > Having a lot of setters for options might make it really hard to find the >methods you're looking for in terms of auto-complete in your IDE. I think it would be significantly better for that purpose than what we have now, because there would be a lot fewer methods than there are current option constants. Firstly, because most of the methods would cover multiple overlapping or related options - e.g. setHttpMethod(string $method) covers CURLOPT_POST, CURLOPT_PUT, CURLOPT_CUSTOMREQUEST, and CURLOPT_HTTPGET; setBasicAuth($username, $password) combines CURLOPT_HTTPAUTH, CURLOPT_USERNAME, and CURLOPT_PASSWORD. Secondly, because some functionality that's not used as often can just be left to the curl_setopt equivalents forever, e.g. we don't need new methods for CURLOPT_DNS_SHUFFLE_ADDRESSES, CURLOPT_HAPPY_EYEBALLS_TIMEOUT_MS, etc, etc. The initial aim could be to cover, say, the 10 most commonly used settings - things like choosing the request method, and including custom request headers. Over time, we could add more methods for common tasks, but continue adding constants / enum cases for more obscure features of the library. > Would it >make sense to split options into a separate object (or perhaps multiple), >that could in theory also be shared between different CurlHandle instances? While I'm not against splitting things up into more objects, I think that becomes a much bigger task to define what goes in each, and harder to do half-way. My gut feeling is that it would descend into a lot of bikeshedding, and stop us making progress; whereas adding a few methods for common use cases could present a real quick win. Regards, -- Rowan Tommins [IMSoP]
Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
On Sun, Feb 18, 2024 at 12:41 PM Rowan Tommins
wrote:
> On 17 February 2024 15:57:20 GMT, Larry Garfield
> wrote:
>
> >The RFC would also benefit greatly from some practical examples of using
> the new API. Right now it's not clear to me (as someone who almost never
> uses Curl directly) how/why I'd use any of these, since there's still "a
> whole crapton of int constants I don't understand floating around." The
> suggestion to use an Enum (or several) here is a good one and would help a
> lot with that, so I'm +1 there.
>
> To my mind, the *eventual* aim should be that users don't *need* a
> userland wrapper just to make a simple request in a readable way, and that
> setting raw curl options becomes an advanced feature that most users never
> need.
>
> I know a lot of people's minds will immediately go to request and response
> objects, but I think we can go a long way by just making well-named methods
> wrapping one or two curl options each, so that you could write this:
>
> $ch = new CurlHandle('https://example.com');
> $ch->setMethod('POST');
> $ch->setRequestBody('{"stuff":"here"}');
> $ch->setBasicAuth('admin', 'correct-horse-battery-staple');
> $result = $ch->executeAndReturn();
>
> Note that I am not saying every one of those methods needs to be added
> right now; adding a few at a time may be sensible to have time to discuss
> good names and signatures. But to me, renaming CURLOPT_POSTFIELDS to
> Curl\StringOptionsEnum::POSTFIELDS doesn't get us very far - users
> shouldn't need a raw curl setting for such a basic feature in the first
> place.
>
> Regards,
>
> --
> Rowan Tommins
> [IMSoP]
>
Having a lot of setters for options might make it really hard to find the
methods you're looking for in terms of auto-complete in your IDE. Would it
make sense to split options into a separate object (or perhaps multiple),
that could in theory also be shared between different CurlHandle instances?
Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
On 17 February 2024 15:57:20 GMT, Larry Garfield wrote:
>The RFC would also benefit greatly from some practical examples of using the
>new API. Right now it's not clear to me (as someone who almost never uses
>Curl directly) how/why I'd use any of these, since there's still "a whole
>crapton of int constants I don't understand floating around." The suggestion
>to use an Enum (or several) here is a good one and would help a lot with that,
>so I'm +1 there.
To my mind, the *eventual* aim should be that users don't *need* a userland
wrapper just to make a simple request in a readable way, and that setting raw
curl options becomes an advanced feature that most users never need.
I know a lot of people's minds will immediately go to request and response
objects, but I think we can go a long way by just making well-named methods
wrapping one or two curl options each, so that you could write this:
$ch = new CurlHandle('https://example.com');
$ch->setMethod('POST');
$ch->setRequestBody('{"stuff":"here"}');
$ch->setBasicAuth('admin', 'correct-horse-battery-staple');
$result = $ch->executeAndReturn();
Note that I am not saying every one of those methods needs to be added right
now; adding a few at a time may be sensible to have time to discuss good names
and signatures. But to me, renaming CURLOPT_POSTFIELDS to
Curl\StringOptionsEnum::POSTFIELDS doesn't get us very far - users shouldn't
need a raw curl setting for such a basic feature in the first place.
Regards,
--
Rowan Tommins
[IMSoP]
Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
On Thu, Feb 15, 2024, at 9:44 AM, Sara Golemon wrote: > Summarizing replies so far. Won't be able to update the RFC > immediately as my day job needs me, but some great discussions already, > gang. Thanks! > > * Define the conditions under which exceptions will be thrown (and > which exceptions) - I'll add these to the RFC, but in short: > * CurlException - Never, it's an interface type to group the other > exceptions. > * CurlHandleException - Whenever a CurlHandle::method() fails (in > lieu of returning false) > * CurlMultiException - Same, but for the CurlMultiHandle class. > * CurlShareException - Same, but for the CurlShareHandle class. > > * Naming styles, e.g getErrorNumber(): int vs errno() > Comment: Agreed with your points. We don't have to stick to a hard > line mapping of the function names. My initial translation did because > my bugbear is with the lack of fluency and all the rest is just window > dressing on that. > Proposal: I'll rename all the new APIs according to a get*/set* > scheme without abbreviations, e.g.: > * CurlHandle::getErrorNumber(): int > * CurlHandle::getError(): ?string > * static CurlHandle::getErrorTextFromCode(int $code): ?string > * CurlHandle::execute(): ?string // See late note about exec return > values > * CurlHandle::setOptionInt(int $option, int $value): CurlHandle > > * 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. > > * CurlHandle::exec() mixed typing of return values. > Comment: Agreed. The `true` return value becomes meaningless in the > RETURNTRANSFER==false case. > Proposal: Update the RFC for CurlHandle::execute() to return ?string. > > * https://php.watch/articles/php-curl-security-hardening > Comment: I'll read this later when I'm updating the RFC. > > * Prefer class constants to global constants. > Comment: I'm less compelled by this. The global scope is polluted > with the constants whether we add copies or not. Adding a second way > to spell the constant name only adds a second way to spell the constant > name. > Proposal: Change my mind? > > * Request and Response objects, along the lines of PSR-18 > Comment: I'd be in favor of that, but it's not the mountain I'm > personally trying to climb today. No offense taken if you vote no > because this isn't enough, but I don't have the cycles to go in that > hard. >Proposal: Write an RFC (and get it passed) and I can probably help > you implement it? > > -Sara Obligatory: This seems like something that could be done in user-space as a wrapper around Curl. (That's basically what many HTTP clients are.) Why should this be done in C? The reasoning for that needs to be included in the RFC. The RFC would also benefit greatly from some practical examples of using the new API. Right now it's not clear to me (as someone who almost never uses Curl directly) how/why I'd use any of these, since there's still "a whole crapton of int constants I don't understand floating around." The suggestion to use an Enum (or several) here is a good one and would help a lot with that, so I'm +1 there. Overall I'm not opposed, but there's more fleshing that is needed before it's ready. (Which seems like it's happening based on Sara's response above, so that's good. --Larry Garfield
Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
On Saturday, 17 February 2024 at 07:41, Kalle Sommer Nielsen
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
Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
Hi Sara
Den tors. 15. feb. 2024 kl. 21.08 skrev Sara Golemon :
> * 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.
--
regards,
Kalle Sommer Nielsen
ka...@php.net
[PHP-DEV] Re: [RFC] OOP API for cURL extension
On 16 February 2024 16:09:32 GMT, Rowan Tommins wrote: >public function executeAndReturn(): string >public function executeAndOutput(): void I guess I missed: public function executeToFile(Stream $fileHandle): void public function executeWithCallback(callable $wrIteFunction): void which would imply CURLOPT_FILE and CURLOPT_WRITEFUNCTION, respectively. From what I can see, these four modes are actually mutually exclusive (populating ch->handlers.write->method) with whichever option is touched last governing the actual behaviour of curl_exec(). For instance, setting CURLOPT_FILE to null or CURLOPT_RETURNTRANSFER to false always selects stdout mode, effectively clearing any value set with CURLOPT_WRITEFUNCTION. Having separate execute methods would make that much more obvious. Incidentally, I notice there is currently some code in _php_curl_verify_handlers where a bad stream in CURLOPT_FILE will fall back to writing the result to stdout. Is it me, or is that a really terrible idea, potentially exposing private data to the user? Should that scenario be promoted to an immediate false return in curl_exec, and Error in the new OO wrapper? Regards, -- Rowan Tommins [IMSoP]
[PHP-DEV] Re: [RFC] OOP API for cURL extension
On 15 February 2024 15:44:13 GMT, Sara Golemon wrote: >* CurlHandle::exec() mixed typing of return values. > Comment: Agreed. The `true` return value becomes meaningless in the >RETURNTRANSFER==false case. > Proposal: Update the RFC for CurlHandle::execute() to return ?string. Should we take this a step further, and remove CURLOPT_RETURNTRANSFER as a valid option on the object completely? Instead of an overloaded exec() method, provide: public function executeAndReturn(): string public function executeAndOutput(): void Perhaps the option could be accepted in the relevant setOpt methods, but issue a warning that it has no effect. Since both the default for the option and the name of the method are changing anyway, I don't think this significantly affects the migration effort for the tiny minority of cases where you actually want the direct output behaviour. Regards, -- Rowan Tommins [IMSoP]
Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
Hi On 2/15/24 16:44, Sara Golemon wrote: * Define the conditions under which exceptions will be thrown (and which exceptions) - I'll add these to the RFC, but in short: * CurlException - Never, it's an interface type to group the other exceptions. Interface or base Exception? I would suggest base exception for consistency with \Random\RandomException and \DateException and because of the reasoning I gave in this PR: https://github.com/php/php-src/pull/9220 * CurlHandleException - Whenever a CurlHandle::method() fails (in lieu of returning false) * CurlMultiException - Same, but for the CurlMultiHandle class. * CurlShareException - Same, but for the CurlShareHandle class. You already said that you would clarify, but I'm not sure if I find the distinction between Handle/Multi/Share useful. An invalid option being passed to curl_setopt() and curl_multi_setopt() is an invalid option in both cases. No need to differentiate the exception type, because you know what function you called. In fact it should probably be a ValueError and not even something ext/curl-specific. Likewise a connection failure would be a connection failure, no matter if it is emitted by a handle or a multihandle: This could be a CurlConnectException (or possible a namespaced Curl\ConnectException). Best regards Tim Düsterhus
[PHP-DEV] Re: [RFC] OOP API for cURL extension
+1 from me too
On Thu, Feb 15, 2024 at 5:30 PM Flávio Heleno
wrote:
> On Wed, Feb 14, 2024 at 10:44 PM Sara Golemon wrote:
>
>> Good afternoon folks, I'd like to open discussion on adding OOP APIs to
>> the cURL extension.
>> https://wiki.php.net/rfc/curl-oop
>>
>> This has been a long standing bug-bear of mine, and I think its time has
>> come.
>>
>> try {
>> (new \CurlHandle)->setOpt(YOUR_VOTE, true)->exec();
>> } catch (\CurlHandleException $ex) {
>> assert(false); // Why not?!
>> }
>>
>> -Sara
>>
>
> Although I do not have voting karma, that'd be a +1 from me!
>
> --
> Atenciosamente,
>
> Flávio Heleno
>
--
Regards
Muhammad Yousuf Tafhim
Full Stack Developer | Moodle Developer | WordPress Developer
0092-321-2474-383 yousuf.taf...@gmail.com
[image: twitter] [image: linkedin]
[PHP-DEV] Re: [RFC] OOP API for cURL extension
Summarizing replies so far. Won't be able to update the RFC immediately as my day job needs me, but some great discussions already, gang. Thanks! * Define the conditions under which exceptions will be thrown (and which exceptions) - I'll add these to the RFC, but in short: * CurlException - Never, it's an interface type to group the other exceptions. * CurlHandleException - Whenever a CurlHandle::method() fails (in lieu of returning false) * CurlMultiException - Same, but for the CurlMultiHandle class. * CurlShareException - Same, but for the CurlShareHandle class. * Naming styles, e.g getErrorNumber(): int vs errno() Comment: Agreed with your points. We don't have to stick to a hard line mapping of the function names. My initial translation did because my bugbear is with the lack of fluency and all the rest is just window dressing on that. Proposal: I'll rename all the new APIs according to a get*/set* scheme without abbreviations, e.g.: * CurlHandle::getErrorNumber(): int * CurlHandle::getError(): ?string * static CurlHandle::getErrorTextFromCode(int $code): ?string * CurlHandle::execute(): ?string // See late note about exec return values * CurlHandle::setOptionInt(int $option, int $value): CurlHandle * 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. * CurlHandle::exec() mixed typing of return values. Comment: Agreed. The `true` return value becomes meaningless in the RETURNTRANSFER==false case. Proposal: Update the RFC for CurlHandle::execute() to return ?string. * https://php.watch/articles/php-curl-security-hardening Comment: I'll read this later when I'm updating the RFC. * Prefer class constants to global constants. Comment: I'm less compelled by this. The global scope is polluted with the constants whether we add copies or not. Adding a second way to spell the constant name only adds a second way to spell the constant name. Proposal: Change my mind? * Request and Response objects, along the lines of PSR-18 Comment: I'd be in favor of that, but it's not the mountain I'm personally trying to climb today. No offense taken if you vote no because this isn't enough, but I don't have the cycles to go in that hard. Proposal: Write an RFC (and get it passed) and I can probably help you implement it? -Sara
[PHP-DEV] Re: [RFC] OOP API for cURL extension
On 14-02-2024 19:47, Sara Golemon wrote:
Good afternoon folks, I'd like to open discussion on adding OOP APIs to the
cURL extension.
https://wiki.php.net/rfc/curl-oop
This has been a long standing bug-bear of mine, and I think its time has
come.
try {
(new \CurlHandle)->setOpt(YOUR_VOTE, true)->exec();
} catch (\CurlHandleException $ex) {
assert(false); // Why not?!
}
-Sara
I love the idea of an OOP API. Personally I use Python's Requests
library a lot, it could offer some inspiration. I would really like to
simply write:
Curl::get('https://...', params: ['key' => 'value'])->json()
Curl::post(...)
...
Some more specific exceptions may be nice too, to easily differentiate
between errors you might want to retry (network errors) and programming
errors.
Regards,
Dik
