PHP 8: Add support for CurlHandle resource objects #70
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In PHP 8.0, [Curl resources are changed to CurlHandle class objects](https://php.watch/versions/8.0/resource-CurlHandle#is-resource). This changes DocBlock parameters and other areas where provided handles are checked with `is_resource()`, which now needs to account for the replaced `CurlHandle` class objects.
dbu
reviewed
Nov 22, 2020
| @@ -80,7 +80,7 @@ public function __construct( | |||
| $handle, | |||
| ResponseBuilder $responseBuilder | |||
| ) { | |||
| if (!is_resource($handle)) { | |||
| if (!is_resource($handle) && !(is_object($handle) && $handle instanceof \GdImage)) { | |||
There was a problem hiding this comment.
i am confused by the second part, i assume you meant CurlHandle rather than GdImage?
but i think we should make an explicit check for the php version. something like
if (
PHP_MAJOR_VERSION === 7 && !is_resource($handle)
|| PHP_MAJOR_VERSION > 7 && !$handle instanceof CurlHandle
) {
...
| * @param resource $handle cURL handle. | ||
| * @param ResponseBuilder $responseBuilder Response builder. | ||
| * @param RequestInterface $request HTTP request. | ||
| * @param resource|CurlHandle $handle cURL handle. |
There was a problem hiding this comment.
Suggested change
| * @param resource|CurlHandle $handle cURL handle. | |
| * @param resource|\CurlHandle $handle cURL handle. |
or add CurlHandle to the use classes
| @@ -89,7 +89,7 @@ public function __construct( | |||
| ); | |||
| } | |||
|
|
|||
| if (get_resource_type($handle) !== 'curl') { | |||
| if (is_resource($handle) && get_resource_type($handle) !== 'curl') { | |||
There was a problem hiding this comment.
check for php version.
maybe we should split all checks into something like this?
if (PHP_MAJOR_VERSION === 7) {
both checks
} else {
class check
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's in this PR?
In PHP 8.0, Curl resources are changed to CurlHandle class objects. This changes DocBlock parameters and other areas where provided handles are checked with
is_resource(), which now needs to account for the replacedCurlHandleclass objects.Why?
Because in PHP 8.0, Curl handles are no longer resources, and this libraries
is_resourceno longer return true for valid Curl handlers.Example Usage
Checklist