Skip to content

ext/curl: Add CURLOPT_PREREQFUNCTION #13255

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

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Conversation

Ayesh
Copy link
Member

@Ayesh Ayesh commented Jan 26, 2024

Curl >= 7.80.0 supports declaring a function for CURLOPT_PREREQFUNCTION option that gets called after Curl establishes a connection (including the TLS handshake for HTTPS connections), but before the actual request is made.

The callable must return either CURL_PREREQFUNC_OK or CURL_PREREQFUNC_ABORT to allow or abort the request.

This adds support for it to PHP with required ifdef.

@devnexen
Copy link
Member

Looking good overall, @mvorisek remark is valid.

@Ayesh Ayesh force-pushed the curl-prereq-func branch 3 times, most recently from f0efdd4 to 1e856f5 Compare January 26, 2024 17:56
@SakiTakamachi
Copy link
Member

In my opinion, #if LIBCURL_VERSION_NUM >= 0x075000 is a hexadecimal literal, so it might be better to write the version in the comments as elsewhere.

@Ayesh Ayesh force-pushed the curl-prereq-func branch 3 times, most recently from 81c6b26 to c7bb6f0 Compare January 27, 2024 12:10
@Ayesh
Copy link
Member Author

Ayesh commented Jan 27, 2024

Thank you @SakiTakamachi - I added comments to indicate it's for Curl 7.80.0 throughout everywhere I had ifdef calls.

@Ayesh Ayesh force-pushed the curl-prereq-func branch 2 times, most recently from eafa628 to c388954 Compare February 5, 2024 09:51
@Girgias
Copy link
Member

Girgias commented Feb 5, 2024

Can you wait for #13291 and rebase on top of this when it is merged? As the refactoring should simplify the code.

@Ayesh
Copy link
Member Author

Ayesh commented Feb 5, 2024

Awesome, thank you! I can also learn how to do it from your PR too, so this is perfect.

@Ayesh Ayesh marked this pull request as draft February 5, 2024 13:50
@Ayesh Ayesh force-pushed the curl-prereq-func branch 2 times, most recently from 4f1e413 to 545e929 Compare March 11, 2024 15:26
@Ayesh Ayesh force-pushed the curl-prereq-func branch 2 times, most recently from 60eda9b to e53986f Compare March 19, 2024 17:58
@Ayesh Ayesh force-pushed the curl-prereq-func branch from e53986f to 11246af Compare May 6, 2024 23:44
@Ayesh
Copy link
Member Author

Ayesh commented May 6, 2024

@Girgias now that #13291 is merged, I rebased and changed this to use your new FCC pattern. Could you review when you get some time please? Thank you.

@Ayesh Ayesh marked this pull request as ready for review May 6, 2024 23:51
@Ayesh Ayesh force-pushed the curl-prereq-func branch from 11246af to 3350b4a Compare May 6, 2024 23:59
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Minor nits, can you also add a trampoline callback test case?

@Ayesh Ayesh force-pushed the curl-prereq-func branch 2 times, most recently from 962835e to 68e357c Compare August 21, 2024 20:17
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@Ayesh Ayesh force-pushed the curl-prereq-func branch from 68e357c to fce7d17 Compare August 21, 2024 20:29
@Ayesh
Copy link
Member Author

Ayesh commented Aug 21, 2024

Thank you @Girgias - I made a bunch of force-pushes with a few more added tests, but I tested them locally. fingers crossed CI marks them green too.

@Ayesh
Copy link
Member Author

Ayesh commented Aug 21, 2024

curl_setopt($ch, CURLOPT_PREREQFUNCTION, null) seems to segfault, I will sort out and fix soon :(

Comment on lines +36 to +37
var_dump(filter_var($args[1], FILTER_VALIDATE_IP) !== false);
var_dump(filter_var($args[2], FILTER_VALIDATE_IP) !== false);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to have the test not rely on the filter extension? Found this out while trying it locally without ext/filter compiled.

Otherwise it needs to be added to the EXTENSIONS section as I did.

@Ayesh Ayesh force-pushed the curl-prereq-func branch 4 times, most recently from b74ce81 to 885364c Compare August 25, 2024 07:58
Curl >= 7.80.0 supports declaring a function for `CURLOPT_PREREQFUNCTION` option
that gets called after Curl establishes a connection (including the TLS handshake
for HTTPS connections), but before the actual request is made.

The callable must return either `CURL_PREREQFUNC_OK` or `CURL_PREREQFUNC_ABORT` to
allow or abort the request.

This adds support for it to PHP with required ifdef.

 - libc: https://curl.se/libcurl/c/CURLOPT_PREREQFUNCTION.html

Co-Authored-By: Gina Peter Bnayard <girgias@php.net>
@Ayesh Ayesh force-pushed the curl-prereq-func branch from 885364c to 882e64e Compare August 25, 2024 14:08
@Ayesh
Copy link
Member Author

Ayesh commented Aug 25, 2024

CI green finally, and curl_setopt($ch, CURLOPT_PREREQFUNCTION, null); curl_exec($ch); works now :)

@Girgias Girgias merged commit a3b7cc2 into php:master Aug 26, 2024
9 of 10 checks passed
@Ayesh Ayesh deleted the curl-prereq-func branch August 26, 2024 12:35
isfedorov pushed a commit to JetBrains/phpstorm-stubs that referenced this pull request Aug 26, 2024
`CURLOPT_PREREQFUNCTION` is a new Curl option added in PHP 8.4, along
with two constants `CURL_PREREQFUNC_ABORT` and `CURL_PREREQFUNC_OK`.

 - [php/php-src#13255](php/php-src#13255)
 - [libcurl doc - `CURLOPT_PREREQFUNCTION`](https://curl.se/libcurl/c/CURLOPT_PREREQFUNCTION.html)
 - [PHP.Watch Codex - `CURLOPT_PREREQFUNCTION`](https://php.watch/codex/CURLOPT_PREREQFUNCTION)
 - [PHP.Watch Codex - `CURL_PREREQFUNC_ABORT`](https://php.watch/codex/CURL_PREREQFUNC_ABORT)
 - [PHP.Watch Codex - `CURL_PREREQFUNC_OK`](https://php.watch/codex/CURL_PREREQFUNC_OK)
Ayesh added a commit to Ayesh/php-src that referenced this pull request Aug 30, 2024
nielsdos pushed a commit that referenced this pull request Sep 1, 2024
Ayesh added a commit to Ayesh/php-src that referenced this pull request Nov 14, 2024
…tions

This is a follow-up to phpGH-13255.

When the `CURLOPT_PREREQFUNCTION` callback does not return any value
or return an invalid value, we throw a `ValueError` or a `TypeError`
exception. However, it does not prevent the Curl request from going
forward because our default return value is `CURL_PREREQFUNC_OK`.

This changes the default value to `CURL_PREREQFUNC_ABORT`.
Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 14, 2024
Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 15, 2024
Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 15, 2024
Ayesh added a commit to Ayesh/doc-en that referenced this pull request Nov 16, 2024
Girgias pushed a commit to php/doc-en that referenced this pull request Nov 16, 2024
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.

5 participants