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

Update Curl is_resource() to work with PHP 8 CurlHandle and CurlMultiHandle objects #2715

Merged
merged 2 commits into from Oct 1, 2020
Merged

Update Curl is_resource() to work with PHP 8 CurlHandle and CurlMultiHandle objects #2715

merged 2 commits into from Oct 1, 2020

Conversation

Ayesh
Copy link
Contributor

@Ayesh Ayesh commented Jun 28, 2020

Updates is_resource() calls for curl handlers to properly work with PHP 8's \CurlHandle and \CurlMultiHandle objects.
See https://php.watch/versions/8.0/resource-CurlHandle

PHPDoc comments are updated to indicate that the variable/property can be either a resource or a Curl object.
In PHP 8, curl_close() calls are no-op, and and explicit unset() call is added to ensure the \CurlHandle object is destroyed.


This PR depends on #2714 (to enable tests).
Related: #2702


On manual/forced PHP 8 tests, there are two failures related to this repo (and 10 errors related to promises).

There were 2 failures:

1) GuzzleHttp\Test\Handler\CurlFactoryTest::testCreatesCurlHandle
Failed asserting that CurlHandle Object &00000000184dcf9b000000006cc08298 () is of type "resource".

##[error]/home/runner/work/guzzle/guzzle/tests/Handler/CurlFactoryTest.php:49

2) GuzzleHttp\Test\Handler\CurlFactoryTest::testEmitsProgressToFunction
Failed asserting that actual size 5 matches expected size 4.

##[error]/home/runner/work/guzzle/guzzle/tests/Handler/CurlFactoryTest.php:289

ERRORS!
Tests: 486, Assertions: 2092, Errors: 10, Failures: 2, Skipped: 5.

This PR fixes the two failures related to this repo:

Tests: 486, Assertions: 2120, Errors: 10, Skipped: 5.

src/Handler/CurlFactory.php Outdated Show resolved Hide resolved
src/Handler/CurlFactory.php Outdated Show resolved Hide resolved
@Nyholm
Copy link
Member

Nyholm commented Sep 22, 2020

Could you make sure we test this on php8 too?

@GrahamCampbell
Copy link
Member

PHP 8 is pending #2712 first, to avoid conflicts and to ensure the dependencies work on PHP 8.

@GrahamCampbell
Copy link
Member

This needs rebasing. :)

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I'll be happy to merge this once you rebase it and the CI is green

Updates `is_resource()` calls for curl handlers to properly work with PHP 8's `\CurlHandle` and `\CurlMultiHandle` objects.
See https://php.watch/versions/8.0/resource-CurlHandle

PHPDoc comments are updated to indicate that the variable/property can be either a resource or a Curl object.
@Ayesh
Copy link
Contributor Author

Ayesh commented Oct 1, 2020

Thank you @Nyholm and @GrahamCampbell - I just force-pushed after rebasing from current master branch.

@Nyholm
Copy link
Member

Nyholm commented Oct 1, 2020

Could you also make sure the CI is green?

@Ayesh
Copy link
Contributor Author

Ayesh commented Oct 1, 2020

Both test failures are due to the PHP 8's changes in parameter types. I suppose both static analyzers infer the parameter types via Reflection.

It wouldn't be ideal if we were to make psalm/phpstan suppress these errors, but that's the only approach I can think of for now. Even if were to run the static analyzers on PHP 8, it would now complain about the other type of parameters too.

@Ayesh
Copy link
Contributor Author

Ayesh commented Oct 1, 2020

I will make changes in psalm/phpstan config files so it ignores the type mismatch false positives.

@Nyholm
Copy link
Member

Nyholm commented Oct 1, 2020

I agree with you. Let's update the baselines for now.

@Nyholm Nyholm added this to the 7.2.0 milestone Oct 1, 2020
@GrahamCampbell GrahamCampbell mentioned this pull request Oct 1, 2020
@Ayesh
Copy link
Contributor Author

Ayesh commented Oct 1, 2020

Thanks for updating the baselines, it wasn't something I had a lot of experience.

@Nyholm
Copy link
Member

Nyholm commented Oct 1, 2020

I ran the following commands:

phpstan analyze  --generate-baseline
psalm --set-baseline=psalm.baseline.xml

@Nyholm
Copy link
Member

Nyholm commented Oct 1, 2020

Thank you for your work and for the reviews

@Nyholm Nyholm merged commit 33e12c0 into guzzle:master Oct 1, 2020
@mfn
Copy link

mfn commented Oct 1, 2020

I ran the following commands:

phpstan analyze  --generate-baseline
psalm --set-baseline=psalm.baseline.xml

I was about to suggest to add this to composer.json scripts but then realized neither phpstan/psalm is actually a dev dependency, so that would be odd.

This could be unnecessary friction for newcomers, can't expect everyone to know the ins/outs of all the tools I guess, at least when it comes to handling things like updating the baseline.

Any ideas how to make this easier? 🤔

@Nyholm
Copy link
Member

Nyholm commented Oct 1, 2020

Running these commands are rare. Newcomers will never run this.

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.

None yet

5 participants