-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fix UNKNOWN default values in ext/curl and remove the deprecated param of curl_version() #5734
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
Conversation
d2c3df9
to
925258c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
The removal of the curl_version()
may be somewhat controversial, though, since it is deprecated as of PHP 7.4.0 only.
|
||
?> | ||
--EXPECT-- | ||
curl_version() expects exactly 0 parameters, 1 given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we drop this one? Looks like a pure ZPP test for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good idea :)
UPGRADING
Outdated
@@ -760,6 +760,7 @@ PHP 8.0 UPGRADE NOTES | |||
checks for `false`. The curl_share_close() function no longer has an effect, | |||
instead the CurlShareHandle instance is automatically destroyed if it is no | |||
longer referenced. | |||
. The deprecated optional parameter of curl_version() has been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. The deprecated optional parameter of curl_version() has been removed. | |
. The deprecated parameter `$version` of curl_version() has been removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
The question is: is it worth the BC break or should we take a more conservative approach? @nikic ? I am ok with getting rid of the removal, too. |
@kocsismate No problem from my side. We removed plenty of things deprecated in 7.4. |
925258c
to
f92d70c
Compare
Hmm, the AppVeyor test failure may be due to a segfault. |
@cmb69 Due to JIT, maybe? :) |
Maybe. I can't reproduce that test failure, unfortunately. |
Ah, just saw this test failing on master; so the good news is that it's not related to this PR. |
No description provided.