Skip to content

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

Merged
Nyholm merged 2 commits intoguzzle:masterfrom
Ayesh:php8-curlhandle-fix
Oct 1, 2020
Merged

Update Curl is_resource() to work with PHP 8 CurlHandle and CurlMultiHandle objects#2715
Nyholm merged 2 commits intoguzzle:masterfrom
Ayesh:php8-curlhandle-fix

Conversation

@Ayesh
Copy link
Copy Markdown
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.

nikic
nikic reviewed Jun 29, 2020
View reviewed changes
@nikic nikic mentioned this pull request Jun 29, 2020
Closed
@Nyholm
Copy link
Copy Markdown
Member

Nyholm commented Sep 22, 2020

Could you make sure we test this on php8 too?

@GrahamCampbell
Copy link
Copy Markdown
Member

GrahamCampbell commented Sep 22, 2020

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

@GrahamCampbell
Copy link
Copy Markdown
Member

GrahamCampbell commented Sep 30, 2020

This needs rebasing. :)

Nyholm
Nyholm approved these changes Oct 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Nyholm Nyholm left a comment •
edited
Loading

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

@Ayesh
Update Curl is_resource() to work with PHP 8 Curl(Multi)Handle objects
b92f904 
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
Copy Markdown
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
Copy Markdown
Member

Nyholm commented Oct 1, 2020

Could you also make sure the CI is green?

@Ayesh
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Merged
@Ayesh
Copy link
Copy Markdown
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
Copy Markdown
Member

Nyholm commented Oct 1, 2020

I ran the following commands:

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

@Nyholm
Copy link
Copy Markdown
Member

Nyholm commented Oct 1, 2020 •
edited
Loading

Thank you for your work and for the reviews

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

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
Copy Markdown
Member

Nyholm commented Oct 1, 2020

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

@dependabot dependabot bot mentioned this pull request Mar 11, 2021
Merged
@ChristophWurst ChristophWurst mentioned this pull request Apr 22, 2021
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nyholm Nyholm Nyholm approved these changes

+1 more reviewer

@nikic nikic nikic left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

5 participants

@Ayesh @Nyholm @GrahamCampbell @mfn @nikic