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, à 15 h 48, Larry Garfield <la...@garfieldtech.com> a écrit : > 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’s own parser. 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 one > > 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 > 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 to > > one to libcurl functions : > > > > function curl_url(?string $url = null): CurlUrl|false {} > > function curl_url_set(CurlUrl $url, int $part, string $content, int > $flags > > = 0): bool {} > > function curl_url_get(CurlUrl $url, int $part, int $flags = 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 API > > provide higher level APIs as guzzle for example. (You can see the current > > implementation [5]) > > > > final class CurlUrl implements Stringable > > { > > public function __construct(?string $url = null) {} > > public function get(int $part, int $flags = 0): string {} > > public function set(int $part, string $content, int $flags = 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 the > > CurlHandle class would look like that (First implementation [6]) > > > > final class CurlHandle > > { > > public function __construct(?string $url = null) {} > > public function setOpt(int $option, mixed $value): bool {} > > public function getInfo(?int $option = 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 = 0): string {} > > Absolutely not. :-) It should be public function getHost(), public > function getScheme(), etc. The `int $part` bit is frankly bad design even > 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 that > 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 use > ->. 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php > >
