Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122413
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-144-ge5821d614e-fm-20240125.002-ge5821d61
Precedence: bulk
MIME-Version: 1.0
Message-ID: <9bc2d842-5f56-4062-8b21-3b41310f2c86@app.fastmail.com>
In-Reply-To: 
 <CAESVnVpHRU8-Fc10XEdRkf3tU-5bXxO4tZ-CKwWJeUabB2LM4g@mail.gmail.com>
References: 
 <CAESVnVp11-EVBrhzSj16=Nd3fYE366piq6fG-9wV3+iH8TxchA@mail.gmail.com>
 <CAESVnVpHRU8-Fc10XEdRkf3tU-5bXxO4tZ-CKwWJeUabB2LM4g@mail.gmail.com>
Date: Sat, 17 Feb 2024 09:57:20 -0600
To: "php internals" <internals@lists.php.net>
Subject: Re: [PHP-DEV] Re: [RFC] OOP API for cURL extension
Content-Type: text/plain
From: larry@garfieldtech.com ("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