Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension

2024-02-18 Thread Rowan Tommins
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

2024-02-18 Thread Lynn
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

2024-02-18 Thread Rowan Tommins
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

2024-02-17 Thread Larry Garfield
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

2024-02-17 Thread Gina P. Banyard
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

2024-02-17 Thread Kalle Sommer Nielsen
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

2024-02-17 Thread Rowan Tommins
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] R: [RFC] OOP API for cURL extension

2024-02-17 Thread ssil...@libero.it
Hi Sara,
i like this proposal.

Silvio

From: Sara Golemon 
Sent: Wednesday, February 14, 2024 7:47 PM
To: PHP internals 
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


[PHP-DEV] Re: [RFC] OOP API for cURL extension

2024-02-17 Thread Rowan Tommins
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

2024-02-16 Thread Tim Düsterhus

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

2024-02-16 Thread Yousuf Tafhim
+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

2024-02-15 Thread Sara Golemon
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

2024-02-15 Thread Dik Takken

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


Re: [RFC] OOP API for cURL extension

2024-02-15 Thread Flávio Heleno
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


Re: [RFC] OOP API for cURL extension

2024-02-15 Thread Григорий Senior PHP / Разработчик Web
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.


Re: [RFC] OOP API for cURL extension

2024-02-15 Thread Christian Stoller
Am 15-Feb-2024 03:30:44 +0100 schrieb poll...@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


Re: [RFC] OOP API for cURL extension

2024-02-15 Thread Tim Starling

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


Re: [RFC] OOP API for cURL extension

2024-02-15 Thread Thomas Bley
> Sara Golemon  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


Re: [RFC] OOP API for cURL extension

2024-02-15 Thread Kamil Tekiela
I love it!

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


Re: [RFC] OOP API for cURL extension

2024-02-15 Thread Arvids Godjuks
On Thu, 15 Feb 2024 at 03:53, 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
>

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.godj...@gmail.com
Telegram: @psihius https://t.me/psihius


[RFC] OOP API for cURL extension

2024-02-14 Thread Sara Golemon
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