Skip to content

Conversation

TobiasBg
Copy link

@TobiasBg TobiasBg commented Sep 1, 2025

curl_close is deprecated in PHP 8.5+, and hasn't been doing anything since PHP 8.0.

To prevent deprecation warnings it should therefore be called on older versions of PHP only.

See https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_no-op_functions_from_the_resource_to_object_conversion.

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • Bug fix
  • New feature
  • Documentation improvement
  • Code quality improvement

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

@TobiasBg Thanks for this PR.

The change on line 326 looks valid, but the other two changes are redundant. The conditions already check if $this->handle is a resource and that condition would return false as of PHP 8.0 (as it would be an object, not a resource), so adding an additional version comparison will not make a difference...

@jrfnl
Copy link
Member

jrfnl commented Sep 1, 2025

P.s.: probably better to change the version comparison on line 326 to a check for is_resource() as well for stability (if that is not checked earlier in the logic, in which case, the change on line 326 would be redundant too....)

`curl_close` is deprecated in PHP 8.5+, and hasn't been doing anything since PHP 8.0, when handles were switched from `resource` to `object`.

To prevent deprecation warnings it should therefore be called on older versions of PHP only, where handles are `resource`s.

See https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_no-op_functions_from_the_resource_to_object_conversion.
@TobiasBg
Copy link
Author

TobiasBg commented Sep 2, 2025

Oh, thanks @jrfnl! Good to know about the resource/object switch.

I have updated the PR, to only leave that one line, with an is_resource check.

@jrfnl
Copy link
Member

jrfnl commented Sep 2, 2025

Good to know about the resource/object switch.

It helps to understand a deprecation before addressing it... Not sure if you pulled similar PRs elsewhere, but if so, you may want to update those too...

@TobiasBg
Copy link
Author

TobiasBg commented Sep 2, 2025

Yes, checked those already. Thanks.

I think that including the PHP version check can be helpful in the future, when support for older versions of PHP is dropped in projects.

@jrfnl
Copy link
Member

jrfnl commented Sep 2, 2025

I think that including the PHP version check can be helpful in the future, when support for older versions of PHP is dropped in projects.

I don't agree. That's what comments are for, or one could open an issue with a tasklist as a reminder of things which would need to be done in the future, but the code itself should be based on what makes it most stable. Considering that people can compile custom versions of PHP with different version of extensions (no matter how unlikely it is for anyone to do that in this case), the is_resource() check creates the most stable code.

@TobiasBg
Copy link
Author

TobiasBg commented Sep 3, 2025

Great points and arguments! Changed my mind :-) Thanks!

@TobiasBg TobiasBg requested a review from jrfnl September 12, 2025 10:23
@jrfnl jrfnl removed their request for review September 12, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants