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

Fix GH-11755: Expose CURL_HTTP_VERSION_3 constant #12000

Closed
wants to merge 1 commit into from

Conversation

adoy
Copy link
Member

@adoy adoy commented Aug 19, 2023

This commit will expose CURL_HTTP_VERSION_3 and CURL_HTTP_VERSION_3ONLY constants when available.

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.

This makes sense to me, and we have added support for cURL constants in patch releases.

But those will need to be clearly documented.

However, maybe it should only be 8.2+? I don't think allo of the cURL constants that were merged in 8.2 were backported to 8.1.

@adoy
Copy link
Member Author

adoy commented Aug 20, 2023

You're right few constants were not backported. Usually my goto on including new constants is does it requires new code other than just exposing the constant. But I would be fine with 8.2 only anyway 8.1 is on his way out.

@GrahamCampbell
Copy link
Contributor

CURL_VERSION_HTTP3 only exists on PHP 8.2 and not 8.1, so it would be very odd to add this to PHP 8.1. PHP 8.2 or 8.3 are the only possible options IMO.

@GrahamCampbell
Copy link
Contributor

What can I do to help move this along?

@Girgias
Copy link
Member

Girgias commented Sep 20, 2023

This requires RM approval:

@GrahamCampbell
Copy link
Contributor

I don't think it makes sense to apply this to PHP 8.1 unless we also backport the other curl constants.

@GrahamCampbell
Copy link
Contributor

If we just go for 8.2, can we proceed here right now, then merge up branches as usual into 8.3?

@Girgias
Copy link
Member

Girgias commented Sep 20, 2023

No, this needs RM approval.

@bukka
Copy link
Member

bukka commented Sep 20, 2023

Isn't this still experimental - https://curl.se/docs/http3.html ? If so, I don't think we should introduce until it is stable.

I think this is something that should go only to 7.4 - it is messy to introduce new features to stable versions - I know that we sometime do it but in my opinion we should stop doing that and introduce features only to minor version untill the first RC.

@bukka
Copy link
Member

bukka commented Sep 20, 2023

Also shouldn't there be some checks if curl was actually build with http3 support? I guess this won't be probably necessary once it's stable but it seems to me that this will be mostly unavailable on most distros (haven't checked though)

@patrickallaert
Copy link
Contributor

I don't think it makes sense to apply this to PHP 8.1 unless we also backport the other curl constants.

I fully agree with this ☝️

@Ayesh
Copy link
Member

Ayesh commented Oct 29, 2023

I mistakenly made a PR (#12543, now closed) that essentially did the same. I also made a (now merged) PR that should update the regex used in Curl sync-constants.php file so the script can highlight missing CURL_HTTP_* constants as well.

I spent an embarrassing amount of time last week getting PHP to support HTTP/3, and would like to submit some thoughts.


Isn't this still experimental - https://curl.se/docs/http3.html ? If so, I don't think we should introduce until it is stable.

HTTP/3 support in Curl itself is experimental, not to mention the standard. I had segfaults when using patched OpenSSL+ngtcp2+nghttp3+Curl. I could, however, get HTTP/3 to work with WolfSSL (where Daniel himself works at).

However, the fact that we declare this constant or not does not determine if Curl can speak HTTP/3. We already have CURL_VERSION_HTTP3 that is a feature flag. We also show HTTP/3 under the "Features" list in phpinfo pages.

In fact, even without this PR, it is possible to speak HTTP/3 by directly passing the constant value:

curl_setopt($ch, CURLOPT_HTTP_VERSION, 30);
// true

This behavior is similar for HTTP/2 as well. Even if ext-curl is not built with HTTP/2 support, this constant is declared.

Also shouldn't there be some checks if curl was actually build with http3 support?

We apparently don't have a solid way to determine HTTP/2 support either. Curl will not use it if there is no HTTP/2 or HTTP/3 support, so passing this option does not usually cause any harm. Curl will fail with the ...3ONLY option though.

The only two programmatic ways to determine support are:

  1. curl_setopt($ch, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_3); === true
  2. curl_version()['features'] & CURL_VERSION_HTTP3 !== 0

phpinfo pages should also show it if supported:

image

@GrahamCampbell
Copy link
Contributor

cc @nicolas-grekas r.e. Symfony HTTP client.

@Ayesh
Copy link
Member

Ayesh commented Oct 30, 2023

FWIW, I just finished a somewhat lengthy article HTTP/3 Request with PHP Curl Extension

@Ayesh
Copy link
Member

Ayesh commented Dec 7, 2023

Curl 8.5.0 was released yesterday, marking HTTP/3 implementation as no longer experimental. All ngtcp2+nghttp3+{wolfSSL, quictls, BoringSSL, libressl, AWS-LC,GnuTLS} combinations (with OpenSSL support notable being still incomplete due to OpenSSL lacking proper HTTP/3 support) should now be stable enough.

I wanted to make a subtle ping to reconsider merging this PR, as a first step in hope to have complete HTTP/3 support in PHP 8.4. Thank you.

@bukka
Copy link
Member

bukka commented Jan 12, 2024

I think it's fine to add this but only for master.

Ayesh added a commit to Ayesh/php-src that referenced this pull request Feb 6, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

1: https://curl.se/libcurl/c/libcurl-errors.html
2: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Feb 6, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Feb 7, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Feb 7, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Feb 7, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Mar 6, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Mar 11, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Mar 12, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Mar 16, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Mar 19, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Mar 19, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request Apr 10, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
Ayesh added a commit to Ayesh/php-src that referenced this pull request May 6, 2024
ext/curl now requires libcurl 7.61.0, which means a lot of CURL* constants can be re-organized.

This commit re-arranges all of the CURLE_* constants to a single part of the `curl.stub.php` file,
and rearranges them to match the order of the error constants in the libcurl documentation[^1].

Constants that are deprecated[^2] are moved shifted to the end of the `CURLE_*` constants list.

[^1]: https://curl.se/libcurl/c/libcurl-errors.html
[^2]: https://curl.se/libcurl/c/symbols-in-versions.html

Related: php#12000, php#13259, php#13209, php#13282
@Ayesh
Copy link
Member

Ayesh commented Aug 11, 2024

Just wanted to gently bump this up, really looking forward to have this merged for PHP 8.4 before the upcoming feature-freeze.

@Girgias
Copy link
Member

Girgias commented Aug 11, 2024

This PR needs to be rebased on top of master.

Ayesh added a commit to Ayesh/php-src that referenced this pull request Aug 11, 2024
This intends to supersede the two following PRs:
 - php#12000 because it does not modify the stub file, but only update the
   arginfo file. It also proposes to merge to GA branches, and is
   currently marked as Requires RM Approval.
 - php#12543 Essentially the same as this PR and from the same author, as
   this, but its about a year old and requires rebasing anyway.

This adds the `CURL_HTTP_VERSION_3` and `CURL_HTTP_VERSION_3ONLY`
constants on relevant versions (7.66 and 7.88 respectively).

It is possible to use HTTP/3 without having these constants declared,
but having them declared in PHP makes things more approachable and
"official".
@Ayesh
Copy link
Member

Ayesh commented Aug 11, 2024

Thanks for the review @Girgias. This PR apparently does not modify the stub file, and looks like a manual arginfo change, so I opened #15350 to hopefully supersede this PR.

Ayesh added a commit to Ayesh/php-src that referenced this pull request Aug 12, 2024
This intends to supersede the two following PRs:
 - php#12000 because it does not modify the stub file, but only update the
   arginfo file. It also proposes to merge to GA branches, and is
   currently marked as Requires RM Approval.
 - php#12543 Essentially the same as this PR and from the same author, as
   this, but its about a year old and requires rebasing anyway.

This adds the `CURL_HTTP_VERSION_3` and `CURL_HTTP_VERSION_3ONLY`
constants on relevant versions (7.66 and 7.88 respectively).

It is possible to use HTTP/3 without having these constants declared,
but having them declared in PHP makes things more approachable and
"official".
Girgias pushed a commit that referenced this pull request Aug 12, 2024
This intends to supersede the two following PRs:
 - #12000 because it does not modify the stub file, but only update the
   arginfo file. It also proposes to merge to GA branches, and is
   currently marked as Requires RM Approval.
 - #12543 Essentially the same as this PR and from the same author, as
   this, but its about a year old and requires rebasing anyway.

This adds the `CURL_HTTP_VERSION_3` and `CURL_HTTP_VERSION_3ONLY`
constants on relevant versions (7.66 and 7.88 respectively).

It is possible to use HTTP/3 without having these constants declared,
but having them declared in PHP makes things more approachable and
"official".
@Ayesh
Copy link
Member

Ayesh commented Aug 12, 2024

Now that #15350 is merged, I think we can close this PR.

@Girgias
Copy link
Member

Girgias commented Aug 12, 2024

Closed in favour of #15350

@Girgias Girgias closed this Aug 12, 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.

6 participants