Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117999
MIME-Version: 1.0
References: <CAOfKkdRzoo7JuD07F8nkZBYvS5eDMiV2r6cNV-FgeLxBrdCsiQ@mail.gmail.com>
 <b8333cb6-eaa0-4170-a3ba-29eadb2fd8fa@www.fastmail.com> <CAOfKkdSWhQ5-wbnnWzb74VknGNVSM+2+5-sMoc-_yOEzk4JFFQ@mail.gmail.com>
 <CALKiJKqy2asFNUFb+JdFuTLwiLGWdhq+XvJhX482n5w8cTADdg@mail.gmail.com>
In-Reply-To: <CALKiJKqy2asFNUFb+JdFuTLwiLGWdhq+XvJhX482n5w8cTADdg@mail.gmail.com>
Date: Sat, 18 Jun 2022 22:39:06 -0400
Message-ID: <CAOfKkdT4bMi+kmsvdFYQWwgPw4MpgLx8eC+FDDX-cAJZeZRD2A@mail.gmail.com>
To: Rowan Tommins <rowan.collins@gmail.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000ce04b505e1c3e4fd"
Subject: Re: [PHP-DEV] Discussion about new Curl URL API and ext/curl improvements
From: pierrick@php.net (Pierrick Charron)

--000000000000ce04b505e1c3e4fd
Content-Type: text/plain; charset="UTF-8"

Hi Rowan, all,

Thanks for your responses and comments.


> I think it's perfectly reasonable for CurlUrl to be designed the same way,
> regardless of what else we do with the extension.
>

As suggested by most of you, I created a new version of the proposed new
URL API with a friendlier interface [1]. A quick descriptions of the
changes I made :
- The curl_url_* procedural API was removed
- The CurlUrl::set() and ::get() methods with the CURLUPART_ constants were
replaced by more explicit setters and getters (ex: setHost() and getHost()).
- All the CURLU_* constants are now CurlUrl class constants (ex:
CURLU_GUESS_SCHEME => CurlUrl::GUESS_SCHEME)
- All the CURLUE_* constants are now CurlUrlException class constants (ex:
CURLUE_MALFORMED_INPUT => CurlUrlException::MALFORMED_INPUT)

Does it look more like what you were expecting ? Any other improvements
that should be made ?

This feels like a very small gain, since users can already type "curl_" and
> get a list of all curl functions. It also has real costs: users may be
> confused why there are two ways of writing the same thing; and it sets the
> precedent that methods on CurlHandle should mirror curl_* functions, even
> though we know those have terrible usability.
>

This is not the only change that I would like to introduce with a new OO
API. One other change that I would like to introduce is a better type
checking of `CurlHandle::setOpt()` when declaring strict_types to 1 (I know
Sara once opened a PR about this [2]). The current signature of the
procedural API is `curl_setopt(CurlHandle $handle, int $option, mixed
$value): bool`. The 3rd argument being mixed never throws any TypeError,
and adding this to the current procedural API would be a breaking change.
If we add a new OOP API this kind of change is possible and we can improve
the error message like : "Argument 2 passed to CurlHandle::setOpt() for
option CURLOPT_BUFFERSIZE must be of type int, string given"

However, knowing that the feature freeze for 8.2 is in a month, I will put
more emphasis on the Curl URL API than on a new OO API for existing Curl
API.

Thanks again for your feedback

Pierrick

[1] https://github.com/adoy/php-src/pull/1/files
[2] https://github.com/php/php-src/pull/2495

--000000000000ce04b505e1c3e4fd--