Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

kocsismate
Copy link
Member

No description provided.

@kocsismate kocsismate requested a review from cmb69 June 18, 2020 07:28
@kocsismate kocsismate force-pushed the unknown-default-curl branch from d2c3df9 to 925258c Compare June 18, 2020 07:34
Copy link
Member

@cmb69 cmb69 left a 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
Copy link
Contributor

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.

Copy link
Member Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. The deprecated optional parameter of curl_version() has been removed.
. The deprecated parameter `$version` of curl_version() has been removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@kocsismate
Copy link
Member Author

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.

@nikic
Copy link
Member

nikic commented Jun 18, 2020

@kocsismate No problem from my side. We removed plenty of things deprecated in 7.4.

@kocsismate kocsismate force-pushed the unknown-default-curl branch from 925258c to f92d70c Compare June 18, 2020 08:14
@cmb69
Copy link
Member

cmb69 commented Jun 18, 2020

Hmm, the AppVeyor test failure may be due to a segfault.

@kocsismate
Copy link
Member Author

@cmb69 Due to JIT, maybe? :)

@cmb69
Copy link
Member

cmb69 commented Jun 18, 2020

Maybe. I can't reproduce that test failure, unfortunately.

@cmb69
Copy link
Member

cmb69 commented Jun 18, 2020

Ah, just saw this test failing on master; so the good news is that it's not related to this PR.

@php-pulls php-pulls closed this in ed6fbf9 Jun 18, 2020
@kocsismate kocsismate deleted the unknown-default-curl branch June 18, 2020 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants