Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] OOP API for cURL extension #13394

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[RFC] OOP API for cURL extension #13394

wants to merge 8 commits into from

Conversation

sgolemon
Copy link
Contributor

This implementation goes with https://wiki.php.net/rfc/curl-oop and is currently a WIP.

This patch provides the following:

* A tree of four exceptions:
```
CurlException
|- CurlHandleException
|- CurlMultiException
\- CurlShareException
```

* Direct aliasing of most functions to class methods, e.g.
  `curl_exec(CurlHandle $ch): string|bool` -> `CurlHandle::exec(): string|bool`
* Remapping of `*_init()` functions to constructors, e.g.
  `curl_init(?string $url = null): CurlHandle` -> `CurlHandle::__constructor(?string $url = null)`
* Promotion of error returning function to use of exceptions where appropriate
* Promotion of certain functions to allow fluent calling, e.g.
```php
curl_setopt($ch, CURLOPT_FOO, "bar");
curl_setopt($ch, CURLOPT_BAZ, "qux");

$ch->setOpt(CURLOPT_FOO, "bar")->setOpt(CURLOPT_BAZ, "qux");
```
* Addition of "last error" functions/methods returning string:
  * `curl_multi_error(CurlMultiHandle $mh): string` / `CurlMultiHandle::error(): string`
  * `curl_share_error(CurlShareHandle $sh): string` / `CurlShareHandle::error(): string`
@adoy
Copy link
Member

adoy commented Feb 15, 2024

Thanks @sgolemon for taking this on :) I attempted it about two years ago, but faced diverse opinions and failed to convince others of its value. However, I'm confident you'll do a better job. You might want to review the thread for insights to prepare for the discussion.

One aspect I dislike about the current procedural interface (and it's not related to the fact that it's procedural) is the lack of enforced types for options. Do you think it's a good idea to enforce this in the new OOP interface, or should we maintain them as simple "aliases" to keep it consistant ?

Feel free if I can be of any help :) my branch is always available but I don't think there is much you've not already done.

@sgolemon
Copy link
Contributor Author

sgolemon commented Feb 15, 2024

Ooooh, thanks for the link. I saw the prior RFC, but skipped past the title since it seemed to only be about the URL parsing API.

I'll read through the thread and see what's most useful to pull in. 👍


RETURN_NULL();

RETURN_NULL();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: double null :)

* Returns self to match setOpt() behavior.
* @throws CurlHandleException
*/
public function setOptArray(array $option): \CurlHandle {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function setOptArray(array $option): \CurlHandle {}
public function setOptArray(array $options): \CurlHandle {}

Same as in curl_setopt_array

public function unescape(string $str): string {}

/**
* If CURLOPT_RETURNTRANSFER is True, returns the result on success,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is directly specifying current behaviour, but I wish the return wasn't ambigious based on the current option.

@@ -3567,6 +3635,53 @@ final class CurlHandle
*/
final class CurlMultiHandle
{
/**
* Returns self for fluent calling.
* @throws CurlException.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be CurlMultiException (and in methods below)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants