Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117969
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-712-gb9e94258b0-fm-20220610.001-gb9e94258
Mime-Version: 1.0
Message-ID: <b8333cb6-eaa0-4170-a3ba-29eadb2fd8fa@www.fastmail.com>
In-Reply-To: 
 <CAOfKkdRzoo7JuD07F8nkZBYvS5eDMiV2r6cNV-FgeLxBrdCsiQ@mail.gmail.com>
References: 
 <CAOfKkdRzoo7JuD07F8nkZBYvS5eDMiV2r6cNV-FgeLxBrdCsiQ@mail.gmail.com>
Date: Thu, 16 Jun 2022 14:47:41 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Discussion about new Curl URL API and ext/curl improvements
From: larry@garfieldtech.com ("Larry Garfield")

On Thu, Jun 16, 2022, at 2:10 AM, Pierrick Charron wrote:
> Hi internals,
>
> Since its version 6.62.0 [1], libcurl features a brand new URL API [2]=
 that
> can be used to parse and generate URLs, using libcurl=E2=80=99s own pa=
rser. One of
> the goals of this API is to tighten a problematic vulnerable area for
> applications where the URL parser library would believe one thing and
> libcurl another. This could and has sometimes led to security problems=
 [3].
> The new API is handled base and composed of 5 new functions [4] (and o=
ne
> more since 7.80.0 to get more verbose information upon error).
>
> I started to work on an implementation [5] to expose this API to PHP
> userland in the curl extension so that PHP users can benefit from it. =
My
> first reflex was to stay consistent on how the extension is currently
> working and do a one to one mapping of the new curl functions. I got
> feedback from both Ilija and Christoph saying that the API is, I quote=
, "a
> bit clunky" which I can't disagree with.
>
> For a long time, ext/curl worked with handles as "curl resources" and
> functions mapped to the libcurl functions, but since PHP8.0 "curl
> resources" were converted to opaque classes with no methods. So my main
> goal here is to come up with a solution for the new Curl URL API, but =
maybe
> also to be consistent, make some changes to existing curl classes like
> `CurlHandle` to add methods and improve the current state of the exten=
sion.
>
> Here is the solution I would propose :
> - First of course keep all the current ext/curl functions as is not to
> break any compatibility (Seems obvious but just to be sure everybody i=
s on
> the same page).
> - For consistency expose the new Curl URL API as functions mapped one =
to
> one to libcurl functions :
>
> function curl_url(?string $url =3D null): CurlUrl|false {}
> function curl_url_set(CurlUrl $url, int $part, string $content, int $f=
lags
> =3D 0): bool {}
> function curl_url_get(CurlUrl $url, int $part, int $flags =3D 0):
> string|false {}
> function curl_url_errno(CurlUrl $url): int {}
> function curl_url_strerror(int $error_code): ?string {}
>
> - Add methods to the CurlUrl object to make it less opaque and expose =
an
> object oriented style API. I would keep it minimal and let userlanAd A=
PI
> provide higher level APIs as guzzle for example. (You can see the curr=
ent
> implementation [5])
>
> final class CurlUrl implements Stringable
> {
>     public function __construct(?string $url =3D null) {}
>     public function get(int $part, int $flags =3D 0): string {}
>     public function set(int $part, string $content, int $flags =3D 0):=
 void {}
>     public function getErrno(): int {}
>     public function __toString(): string {}
>     public function __clone() {}
> }
>
> - It would also be nice to add this object oriented API for existing
> CurlHandle, CurlMultiHandle and CurlShareHandle classes. For example t=
he
> CurlHandle class would look like that (First implementation [6])
>
> final class CurlHandle
> {
>     public function __construct(?string $url =3D null) {}
>     public function setOpt(int $option, mixed $value): bool {}
>     public function getInfo(?int $option =3D null): mixed {}
>     public function exec(): string|bool {}
>     public function escape(string $string): string|false {}
>     public function unescape(string $string): string|false {}
>     public function pause(int $flags): int {}
>     public function getErrno(): int {}
>     public function reset(): void;
>     public function setOptArray(array $options): bool {}
>     public function upkeep(): bool {}
>     public function __clone() {}
> }
>
> As of right now I still have some unanswered questions like how should=
 we
> handle errors on the new CurlUrl API ?
> - Throw `CurlUrlException` on both the procedural and object oriented =
style
> API (that's how current implementation works [5])
> - Throw `CurlUrlException` with the oriented style API, but return for
> example boolean/null on errors when user uses the procedural API ?
> - Always return null/booleans using both object oriented API and
> procedural API ?
>
> The same question applies if we add a new object oriented style API on
> existing classes. Current functions MUST stay unchanged but should we =
throw
> `CurlException` when the user uses the object oriented style API ? (Th=
at's
> what I would do) Or should we return the same result from OO and proce=
dural
> API ? Should we make the new CurlApi consistent with that ?
>
> What are your thoughts about all this ? Any feedback on any of those
> subjects are more than welcome.
>
> Pierrick


Like most of the responders so far, I would say skip the procedural API,=
 just go OOP and be done with it.  However, I would go a step further an=
d say it should be a *good* OOP API, not just the Curl procedural API wi=
th funny syntax.

For example:

public function get(int $part, int $flags =3D 0): string {}

Absolutely not. :-)  It should be public function getHost(), public func=
tion getScheme(), etc.  The `int $part` bit is frankly bad design even b=
y procedural standards, and has no place in a modern library.  I don't e=
ven know what the $flags are or could be, which means that's also a bad =
design.  They should either be omitted, handled via separate methods, or=
 replaced with named arguments with reasonable defaults.  (We can do tha=
t now, yay!)  Translating those into Curl's nutty flag API is something =
that should be done once, in the wrapping extension/object, and no PHP u=
ser space developer should have to think about it.

Similarly, getErrno() should be replaced.  If a URL cannot even be parse=
d at all, that's an exception.  Not every error should be an exception (=
I recently wrote extensively on the subject here: https://peakd.com/hive=
-168588/@crell/much-ado-about-null), but in this case "I don't know WTF =
to do with this string you gave my constructor" is an exception.  If ins=
tead of a constructor it had a static factory method (or multiple), then=
 we could discuss those returning some other sentinel value instead to i=
ndicate failure, but "there was an error, go call this other thing to fi=
nd out what it was" is a fundamentally flawed API design that should not=
 be replicated.

In short, don't put a wrapper on the existing Curl API that happens to u=
se ->.  Design a good, usable, PHP-developer-friendly API, and put Curl =
behind it.  Trying to just expose a crappy API and make it slightly less=
 crappy is not a good use of time.

--Larry Garfield