Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117970
MIME-Version: 1.0
References: <CAOfKkdRzoo7JuD07F8nkZBYvS5eDMiV2r6cNV-FgeLxBrdCsiQ@mail.gmail.com>
 <b8333cb6-eaa0-4170-a3ba-29eadb2fd8fa@www.fastmail.com>
In-Reply-To: <b8333cb6-eaa0-4170-a3ba-29eadb2fd8fa@www.fastmail.com>
Date: Thu, 16 Jun 2022 17:43:53 -0400
Message-ID: <CAOfKkdSWhQ5-wbnnWzb74VknGNVSM+2+5-sMoc-_yOEzk4JFFQ@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000005853fd05e197893f"
Subject: Re: [PHP-DEV] Discussion about new Curl URL API and ext/curl improvements
From: pierrick@php.net (Pierrick Charron)

--0000000000005853fd05e197893f
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thanks for taking the time to answer and give feedback, that's really
appreciated.

Firstly, about the procedural API, if everyone agrees that we should not
create it (and it looks like it) I will definitely not go against it. This
was just to make it consistent with the current ext/curl api. Reading the
thread about IntlDatePatternGenerator (thanks Mel), it's clear that the
procedural API for the new API will be discarded.

About making a "Good OOP API", of course the goal is to make a *Good* OOP
API. But there are things to take into consideration. The proposal here is
yes to expose the new Curl URL API which is quite small, but also to modify
the existing curl API to make it a little bit more digest for developers.
Good or bad the existing ext/curl is here and will continue to exist for
quite some time. It was designed (maybe on purpose or not) to be a low
level API with tons [1] of low level features, and it was clearly designed
as a thin wrapper/bridge over libcurl. I mean the fact is currently each
constant is the exact same one as in libcurl, and each function is a
wrapper over the exact same function in libcurl. Is it OK to have one part
that is a thin wrapper and another part that is a "remodeling" of the API ?
(This is not a rhetorical question, I really ask myself the question).

Yes we could create a completely new high level API using curl underneath
but I think that if this is what we want, we should create a brand new
extension and start from scratch and design it as a nice High level API
from the start. But that's clearly not what I'm pretending to do.

We could also keep the existing API as is and not improve it living only
with the procedural API, but I think that with minimal effort, we could get
some small (but still) benefits. Yes, CurlHandle for example would just be
a wrapper on the procedural API that happens to use `->` and throw
exceptions instead of returning null/false,  but one of the gains would be
to help developers with autocomplete to find methods they can call on the
handle.

I'm sorry if the fact that this proposal includes two different subjects is
confusing but I think that how we want to deal with the current API will
have an effect on how we want to implement the new one and keep the thing
consistent.

You only talked about the new CurlUrl API in your previous mail, but I'm
curious what you would do with the existing API ?

Pierrick

[1] https://github.com/php/php-src/blob/master/ext/curl/interface.c#L382

Le jeu. 16 juin 2022, =C3=A0 15 h 48, Larry Garfield <larry@garfieldtech.co=
m> a
=C3=A9crit :

> 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 par=
ser. 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 on=
e
> > 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. M=
y
> > 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
> extension.
> >
> > 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 is
> on
> > the same page).
> > - For consistency expose the new Curl URL API as functions mapped one t=
o
> > one to libcurl functions :
> >
> > function curl_url(?string $url =3D null): CurlUrl|false {}
> > function curl_url_set(CurlUrl $url, int $part, string $content, int
> $flags
> > =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 a=
n
> > object oriented style API. I would keep it minimal and let userlanAd AP=
I
> > provide higher level APIs as guzzle for example. (You can see the curre=
nt
> > 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 th=
e
> > 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 ?
> (That's
> > what I would do) Or should we return the same result from OO and
> procedural
> > 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 and
> say it should be a *good* OOP API, not just the Curl procedural API with
> funny syntax.
>
> For example:
>
> public function get(int $part, int $flags =3D 0): string {}
>
> Absolutely not. :-)  It should be public function getHost(), public
> function getScheme(), etc.  The `int $part` bit is frankly bad design eve=
n
> by procedural standards, and has no place in a modern library.  I don't
> even 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 that
> now, yay!)  Translating those into Curl's nutty flag API is something tha=
t
> should be done once, in the wrapping extension/object, and no PHP user
> space developer should have to think about it.
>
> Similarly, getErrno() should be replaced.  If a URL cannot even be parsed
> 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 instead of a constructor it had a static factory method
> (or multiple), then we could discuss those returning some other sentinel
> value instead to indicate failure, but "there was an error, go call this
> other thing to find 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 us=
e
> ->.  Design a good, usable, PHP-developer-friendly API, and put Curl behi=
nd
> it.  Trying to just expose a crappy API and make it slightly less crappy =
is
> not a good use of time.
>
> --Larry Garfield
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

--0000000000005853fd05e197893f--