Add new curl constants from curl > 7.80 #10459

nielsdos · 2023-01-26T22:42:24Z

Girgias · 2023-01-27T16:05:36Z

Please add an entry in UPGRADING

nielsdos · 2023-01-27T18:02:06Z

Added UPGRADING entry

UPGRADING

kocsismate · 2023-01-31T21:24:09Z

FYI (I'm not sure you are aware of this): you can use php ./ext/curl/sync-constants.php to search for missing constants :)

nielsdos · 2023-01-31T21:40:18Z

FYI (I'm not sure you are aware of this): you can use php ./ext/curl/sync-constants.php to search for missing constants :)

Thanks! I wasn't aware, I'm quite new to this part of PHP, I wanted to learn more about this part. I have some implementation work to do it seems, especially regarding that ssh host key function callback :)

devnexen

LGTM but waiting @kocsismate green light on this

UPGRADING

ext/curl/interface.c

ext/curl/curl.stub.php

ext/curl/interface.c

ext/curl/tests/curl_basic_026.phpt

ext/curl/interface.c

adoy · 2023-02-16T05:27:00Z

You'll also need to free ch->handlers.sshhostkey in _php_curl_reset_handlers, copy it in _php_setup_easy_copy_handlers, and also add handling in curl_get_gc.

ext/curl/curl_private.h

nielsdos · 2023-02-18T17:29:43Z

Thanks for the reviews. I pushed fixes (as separate commits to more easily make it reviewable), and I've also pushed a small cleanup commit because I was touching that code anyway.
One thing is uncertain for me: and that's the exception being thrown in the ssh hostkey function callback. I'm not sure if that's correct and I also don't know how to test that properly. Any pointers are appreciated, thanks :)

adoy · 2023-02-18T17:57:37Z

@nielsdos I'll have a look later this weekend.

ext/curl/tests/curl_basic_027.phpt

ext/curl/interface.c

adoy · 2023-02-19T15:46:12Z

ext/curl/interface.c

+		if (retval_long == CURLKHMATCH_OK || retval_long == CURLKHMATCH_MISMATCH) {
+			rval = retval_long;
+		} else {
+			zend_value_error("The CURLOPT_SSH_HOSTKEYFUNCTION callback must return either CURLKHMATCH_OK or CURLKHMATCH_MISMATCH");


I'm not sure a zend_value_error is allowed for a return value.

Yes I was wondering that too. My gut feeling says no. kocsismate talked about the requirements of the function and that is why I added the exception. Since the function only allows two possible return values I wonder what we should do if something is returned that isn't one of the allowed values...

I had a look at the curl code and it looks like this :

rc = data->set.ssh_hostkeyfunc(data->set.ssh_hostkeyfunc_userp, keytype, remotekey, keylen); if(rc!= CURLKHMATCH_OK) {

We could send a E_WARNING to the user saying that the return code was not one of the two allowed and that CURLKHMATCH_MISMATCH will be used. What do you think ?

Okay sounds good. Thanks for the help.
I'll do that when I have time :)

Yes, ValueError always refers to arguments, not return values. A simple TypeError or Error should be thrown instead. I don't like triggering a warning, because it's a programmatic error if an invalid value is returned, and we do not have to respect BC here. So let's be strict

OK got it! In case it's not a long, I'll use a TypeError. If it's a long but not one of the allowed values I'll use an Error (because the type is right this time).

OK, I did some quick investigation, and I found one example where ValueError refers to a property (zend_value_error("SNMP::$max_oids must be greater than 0 or null");). But otherwise I couldn't find any example where a ValueError doesn't refer to arguments. However, if we think through... If TypeErrors are used for return values, why ValueErrors shouldn't... So I'm uncertain at this point.

Using ValueError for a return value would be in violation of the documentation though:

A ValueError is thrown when the type of an argument is correct but the value of it is incorrect.

From: https://www.php.net/manual/en/class.valueerror.php

Yeah,I saw it, but in this case, the manual should be adjusted to the code :) btw I don't mind much what exception we end up with. cc @Girgias Do you have any suggestion?

ooops very late reply, I think a ValueError makes sense, and amend the documentation.
The other solution would be to make those cURL constants an Enum and enforce that return type but that sounds maybe like overengineering, especially if the enum values might get changed.

But possibly the one where a value of a different type is returned should use the more standard type error message, instead of the "must return X or Y"?

ext/curl/interface.c

nielsdos · 2023-02-19T21:03:00Z

Thanks for the review, I pushed the review fixes in a separate commit instead of force-pushing my changes for easier review. I can rebase when you want me to. :)

ext/curl/interface.c

nicolas-grekas · 2023-04-24T05:57:55Z

(rebase needed apparently)

nielsdos · 2023-04-30T21:22:17Z

(rebase needed apparently)

Rebased.

ext/curl/interface.c

nielsdos · 2023-07-15T23:32:11Z

With the feature freeze creeping closer, I think I should finish this.
This was reviewed a whole while back, but that was before there were many new changes done to the code in response to review comments.
There's also the discussion that ended here: #10459 (comment) about the error type. What do we do with that?

Girgias · 2023-07-16T17:47:37Z

With the feature freeze creeping closer, I think I should finish this. This was reviewed a whole while back, but that was before there were many new changes done to the code in response to review comments. There's also the discussion that ended here: #10459 (comment) about the error type. What do we do with that?

Let's switch the exception thrown to Error as this means we can always just change the type later on as it will be a subtype. Other than that I think good to ship.

Fixes phpGH-10454

* Fix callback in test * Use appropriate error types * Check for long type * Get rid of else branch

…hp/php-src#10459)

github-actions bot added the Extension: curl label Jan 26, 2023

devnexen reviewed Jan 31, 2023

View reviewed changes

UPGRADING Show resolved Hide resolved

kocsismate self-requested a review January 31, 2023 21:18

nielsdos force-pushed the new-curl-opt branch from d6ed080 to 25b8c1b Compare January 31, 2023 23:06

devnexen approved these changes Feb 13, 2023

View reviewed changes

kocsismate reviewed Feb 14, 2023

View reviewed changes

UPGRADING Outdated Show resolved Hide resolved

ext/curl/interface.c Show resolved Hide resolved

ext/curl/interface.c Outdated Show resolved Hide resolved

adoy reviewed Feb 16, 2023

View reviewed changes

ext/curl/curl_private.h Show resolved Hide resolved

adoy self-requested a review February 16, 2023 23:41

nielsdos force-pushed the new-curl-opt branch from 25b8c1b to 40ab8cc Compare February 18, 2023 17:28

adoy reviewed Feb 19, 2023

View reviewed changes

kocsismate reviewed Feb 19, 2023

View reviewed changes

ext/curl/interface.c Outdated Show resolved Hide resolved

ext/curl/interface.c Outdated Show resolved Hide resolved

nielsdos force-pushed the new-curl-opt branch from 3abdf5f to 3b8df32 Compare April 30, 2023 21:22

Girgias reviewed May 5, 2023

View reviewed changes

ext/curl/interface.c Show resolved Hide resolved

nicolas-grekas reviewed May 5, 2023

View reviewed changes

ext/curl/interface.c Show resolved Hide resolved

nielsdos added 3 commits July 17, 2023 16:51

Add new curl constants and functionality from curl > 7.80

09724c8

Fixes phpGH-10454

[ci skip] Update UPGRADING for new curl constants and functionality

9599311

Split versions and tests

d2b0c20

nielsdos added 8 commits July 17, 2023 16:51

Add missing memory management code for sshhostkey

871c8e4

Corrections for curl_ssh_hostkeyfunction

4b48d7e

Reduce some redundant code in curl

55aaca0

Address review comments

80f1572

* Fix callback in test * Use appropriate error types * Check for long type * Get rid of else branch

More review fixes

5cb8676

Change error type

69fa157

[ci skip] NEWS

23948f2

Use binary-safe STRINGL macro

7f131ce

nielsdos force-pushed the new-curl-opt branch from 3b8df32 to 7f131ce Compare July 17, 2023 15:42

Check CURLOPT_REDIR_PROTOCOLS_STR too

5fa027e

nielsdos merged commit de60872 into php:master Jul 17, 2023

jrfnl mentioned this pull request Sep 10, 2023

PHP 8.3 | NewConstants: add new Curl constants PHPCompatibility/PHPCompatibility#1638

Merged

llaville added a commit to llaville/php-compatinfo-db that referenced this pull request Oct 8, 2023

[PHP 8.3 support](#133) : add [new curl constants from curl > 7.80](p…

94cbd5c

…hp/php-src#10459)

llaville added a commit to llaville/php-compatinfo-db that referenced this pull request Nov 18, 2023

[PHP 8.3 support](#133) : add [new curl constants from curl > 7.80](p…

b0482ee

…hp/php-src#10459)

llaville added a commit to llaville/php-compatinfo-db that referenced this pull request Nov 23, 2023

[PHP 8.3 support](#133) : add [new curl constants from curl > 7.80](p…

ab898f4

…hp/php-src#10459)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new curl constants from curl > 7.80 #10459

Add new curl constants from curl > 7.80 #10459

nielsdos commented Jan 26, 2023

Girgias commented Jan 27, 2023

nielsdos commented Jan 27, 2023

kocsismate commented Jan 31, 2023

nielsdos commented Jan 31, 2023

devnexen left a comment

adoy commented Feb 16, 2023 •

edited

Loading

nielsdos commented Feb 18, 2023

adoy commented Feb 18, 2023

adoy Feb 19, 2023

nielsdos Feb 19, 2023

adoy Feb 19, 2023

nielsdos Feb 19, 2023

kocsismate Feb 19, 2023

nielsdos Feb 19, 2023

kocsismate Feb 19, 2023

nielsdos Feb 19, 2023

kocsismate Feb 19, 2023

Girgias Mar 30, 2023

nielsdos commented Feb 19, 2023

nicolas-grekas commented Apr 24, 2023

nielsdos commented Apr 30, 2023

nielsdos commented Jul 15, 2023

Girgias commented Jul 16, 2023

Add new curl constants from curl > 7.80 #10459

Add new curl constants from curl > 7.80 #10459

Conversation

nielsdos commented Jan 26, 2023

Girgias commented Jan 27, 2023

nielsdos commented Jan 27, 2023

kocsismate commented Jan 31, 2023

nielsdos commented Jan 31, 2023

devnexen left a comment

Choose a reason for hiding this comment

adoy commented Feb 16, 2023 • edited Loading

nielsdos commented Feb 18, 2023

adoy commented Feb 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos commented Feb 19, 2023

nicolas-grekas commented Apr 24, 2023

nielsdos commented Apr 30, 2023

nielsdos commented Jul 15, 2023

Girgias commented Jul 16, 2023

adoy commented Feb 16, 2023 •

edited

Loading