[RFC] OOP API for cURL extension

Kamil Tekiela

2 years ago

I love it!

When is CurlMultiException and CurlShareException thrown? I feel like
this part in general is not very clear in the RFC.

Tim Starling

2 years ago

On 15/2/24 05: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
>

Thanks for making this. It's certainly an improvement, but it's
disappointing to see such a conservative change when the extension
interface is so weird and awkward. I'm sure you know what I mean.

It's easy to forget how bad it is until you have to do something
without a framework, or explain it to a new developer.

I guess the proper fix would be to add a new extension with new names
for everything, with Request and Response objects, along the lines of
PSR-18. But if I can think of a few minor backwards-compatible changes
which would improve things a bit. For example, getInfo() could be
split out to separate read-only properties. And setOpt() could be
split out to separate chainable mutator methods. That would improve
type inference in surrounding code.

I see CurlHandle::exec() does not have the same return value as
curl_exec() in your proposal, since errors are promoted to exceptions.
But it still has a union return type. I would take it one step
further, and have it unconditionally return either a string or void.

Having it return a string is defensible if you consider it to be
returning the final value of the body buffer. Enable
CURLOPT_RETURNTRANSFER by default. If CURLOPT_WRITEFUNCTION or
CURLOPT_FILE is set and thus the buffer doesn't get appended to,
naturally curl_exec() will return an empty string.

-- Tim Starling

Christian Stoller

2 years ago

Am 15-Feb-2024 03:30:44 +0100 schrieb pollita@php.net:
> 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

Hi Sara,

I like your proposal generally. But I'd suggest to rethink the names of the methods. I understand that you want to reuse the same names of the procedural version and the underlying lib. But this is the chance to optimize the naming and reduce inconsistence. In the future this will become harder.

For example there is the getter "getInfo" and some getters without the "get" prefix, like "errno", "error" and "strerror". I'd suggest to unify them. Either use the "get" prefix for all of those or for none. Personally, I have no particular preference for one of the two options.

The second thing I propose to change is the usage of abbreviations. I'd suggest the following mapping for methods of CurlHandle:
- errno => errorNumber or getErrorNumber
- error => errorMessage or getErrorMessage
- strerror => errorMessageByNumber or getErrorMessageByNumber
- exec => execute
- setOpt => setOption
- setOptArray => setOptionArray

There may be better proposals from others.

What do you think?

Best regards,
Christian

Arvids Godjuks

2 years ago

On Thu, 15 Feb 2024 at 03:53, Sara Golemon <pollita@php.net> 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
>

Good morning from this side of the globe Sara! :)

"Hell... It's about damn time" (c) Tychus, StarCraft 2.

The proposed API looks good to me and covers it all as far as I can tell.

--

Arvīds Godjuks
+371 26 851 664
arvids.godjuks@gmail.com
Telegram: @psihius https://t.me/psihius

Григорий Senior PHP / Разработчик Web

2 years ago

Working with remote servers is a little bit harder than just catching the
exception.

Just implement OOP stuff gives no benefit except "do not read the docs, but
use IDE". Usually count of curl options is so big that it wont help.

And also, there's multicurl too.
There's batch calling with limit too.
There's errors of different types. I mean curl errors on create, curl
errors on execute, http errors (network), response errors (invalid data,
misformatted data), and api errors, that could be sent even with 200 code.

How try/catch will solve? Easy solution, too many discussion points.

Unnamed Person

2 years ago

> Sara Golemon <pollita@php.net> hat am 14.02.2024 19:47 CET geschrieben:
>
>
> 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

Thanks a lot for the rfc.

Regarding secure default options, I'd like to mention this article:
https://php.watch/articles/php-curl-security-hardening

Since CurlHandle::setOpt() no longer returns true on success, it would be good to have an example in the RFC how error handling should be implemented in user land regarding setting invalid, deprecated or unknown options (e.g. SSLVERSION received some changes over the years: https://curl.se/libcurl/c/CURLOPT_SSLVERSION.html).

Also I'd prefer to have class constants for the options, e.g. CURLOPT::TIMEOUT instead of CURLOPT_TIMEOUT.

Static code analysis for curl_getinfo() is currently difficult to implement (see https://github.com/phpstan/phpstan-src/blob/1.11.x/src/Type/Php/CurlGetinfoFunctionDynamicReturnTypeExtension.php). So I'd suggest adding sth. like curl_getinfo_all():CurlInfo-Class if possible.

Maybe Ondrej also has some suggestions regarding a more object oriented version or curl_setopt(CurlOption-Class)? (see https://github.com/phpstan/phpstan-src/blob/1.11.x/src/Reflection/ParametersAcceptorSelector.php#L562)

Regards
Thomas

Flávio Heleno

2 years ago

On Wed, Feb 14, 2024 at 10:44 PM Sara Golemon <pollita@php.net> 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

Yousuf Tafhim

2 years ago

+1 from me too

On Thu, Feb 15, 2024 at 5:30 PM Flávio Heleno <flaviohbatista@gmail.com>
wrote:

> On Wed, Feb 14, 2024 at 10:44 PM Sara Golemon <pollita@php.net> 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.tafhim@gmail.com
[image: twitter] <https://twitter.com/yousuftafhim> [image: linkedin]
<https://www.linkedin.com/in/yousuftafhim/>

Dik Takken

2 years ago

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

Sara Golemon

2 years ago

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

Tim Düsterhus

2 years ago

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

Rowan Collins

2 years ago

On 15 February 2024 15:44:13 GMT, Sara Golemon <pollita@php.net> 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]

Rowan Collins

2 years ago

On 16 February 2024 16:09:32 GMT, Rowan Tommins <rowan.collins@gmail.com> 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]

Kalle Sommer Nielsen

2 years ago

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.

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Gina P. Banyard

2 years ago

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

Larry Garfield

2 years ago

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

Rowan Collins

2 years ago

On 17 February 2024 15:57:20 GMT, Larry Garfield <larry@garfieldtech.com> 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]

Lynn

2 years ago

On Sun, Feb 18, 2024 at 12:41 PM Rowan Tommins <rowan.collins@gmail.com>
wrote:

> On 17 February 2024 15:57:20 GMT, Larry Garfield <larry@garfieldtech.com>
> 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?

Rowan Collins

2 years ago

On 18 February 2024 15:26:37 GMT, Lynn <kjarli@gmail.com> 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]

Silvio Sparapano

2 years ago

Hi Sara,
i like this proposal.

Silvio
________________________________
From: Sara Golemon <pollita@php.net>
Sent: Wednesday, February 14, 2024 7:47 PM
To: PHP internals <internals@lists.php.net>
Subject: [RFC] OOP API for cURL extension

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