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

Pass int to &$still_running of curl_multi_exec #2991

Merged
merged 2 commits into from Mar 7, 2022

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Mar 3, 2022

Fixes #2990

Passing null is not allowed as the type is int.

See https://www.php.net/manual/en/function.curl-multi-exec.php

@ruudk ruudk changed the title [CurlMultiHandler] Pass int to &$still_running of curl_multi_exec [PHP 8.1] [CurlMultiHandler] Pass int to &$still_running of curl_multi_exec Mar 3, 2022
@ruudk
Copy link
Contributor Author

ruudk commented Mar 3, 2022

Deployed this branch to production and it solved the deprecation warning.

Copy link
Contributor

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

This one is interesting. The change looks good to me, but I wanted to propose a test / enabling converting deprecations to exceptions in PHPUnit to catch this. However it appears I'm unable to reproduce this locally:

php > $a = null; http_build_query([], $a, '&');

Deprecated: http_build_query(): Passing null to parameter #2 ($numeric_prefix) of type string is deprecated in php shell code on line 1
php > $a = null; curl_multi_exec(curl_multi_init(), $a);

@TimWolla
Copy link
Contributor

TimWolla commented Mar 4, 2022

However it appears I'm unable to reproduce this locally:

Even CI does not appear to catch this: #2992.

@GrahamCampbell
Copy link
Member

Probably because deprecation errors are suppressed by default on PHP 8.1. Telling PHPUnit to convert them, when they happen, to errors, won't unsuppress them.

@TimWolla
Copy link
Contributor

TimWolla commented Mar 4, 2022

Probably because deprecation errors are suppressed by default on PHP 8.1.

As shown in my first comment: It worked for the http_build_query() case, but curl_multi_exec() is silent. Likewise I've specifically enabled E_ALL in #2992 which caught an incompatibility with guzzle/psr7 and PHP 8.1, but not this bug.

@ruudk
Copy link
Contributor Author

ruudk commented Mar 7, 2022

So can this be merged? :)

@GrahamCampbell GrahamCampbell changed the title [PHP 8.1] [CurlMultiHandler] Pass int to &$still_running of curl_multi_exec Pass int to &$still_running of curl_multi_exec Mar 7, 2022
@GrahamCampbell GrahamCampbell merged commit 74ca2cb into guzzle:master Mar 7, 2022
@GrahamCampbell
Copy link
Member

Thanks. 🍻

@ruudk ruudk deleted the patch-1 branch March 7, 2022 13:17
@ruudk
Copy link
Contributor Author

ruudk commented Mar 7, 2022

@GrahamCampbell Thanks, could you also create a new tag? 🙏

@GrahamCampbell
Copy link
Member

Not right now. I'd recommend turning off deprecation errors in production. @Nyholm and I need to get together at some point, and go through more PRs that need merging, before we're ready for our next release.

@BillyMorgan
Copy link

Probably because deprecation errors are suppressed by default on PHP 8.1.

As shown in my first comment: It worked for the http_build_query() case, but curl_multi_exec() is silent. Likewise I've specifically enabled E_ALL in #2992 which caught an incompatibility with guzzle/psr7 and PHP 8.1, but not this bug.

Can confirm seeing the same behavior. Somehow curl_multi_exec calls don't (always) trigger the deprecation warning. Only reason I started looking at all though is because one of our servers DID start returning the error. Currently looking into what difference there could possibly be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants