Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[RFC] OOP API for cURL extension #13394

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

Open
sgolemon wants to merge 8 commits into php:master
base: master
Choose a base branch
Loading
from sgolemon:curli
Open

[RFC] OOP API for cURL extension #13394

sgolemon wants to merge 8 commits into php:master from sgolemon:curli

Conversation

Copy link
Member

@sgolemon sgolemon commented Feb 14, 2024

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

nielsdos, KennedyTedesco, lucasxciv, javiereguiluz, mechelon, mallardduck, nask0, maxalmonte14, mostafaqanbaryan, professor93, and 5 more reacted with thumbs up emoji fruitl00p, KennedyTedesco, mechelon, mallardduck, marcosmarcolin, maxalmonte14, ging-dev, professor93, and ddevsr reacted with hooray emoji adoy, KennedyTedesco, mechelon, talyssonlima, mallardduck, maxalmonte14, pronskiy, ging-dev, professor93, and ddevsr reacted with heart emoji jorgsowa, KennedyTedesco, javiereguiluz, mechelon, mallardduck, maxalmonte14, ging-dev, professor93, and ddevsr reacted with rocket emoji
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`
Copy link
Member

adoy commented Feb 15, 2024
edited
Loading

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.

Copy link
Member Author

sgolemon commented Feb 15, 2024
edited
Loading

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. 👍

adoy reacted with thumbs up emoji


RETURN_NULL();

RETURN_NULL();
Copy link
Member

@kocsismate kocsismate Feb 18, 2024
edited
Loading

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.

{
/**
* 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
Reviewers

@kocsismate kocsismate kocsismate left review comments

@adoy adoy Awaiting requested review from adoy adoy is a code owner

+2 more reviewers

@rosier rosier rosier left review comments

@xPaw xPaw xPaw left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /