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

Fix PHP 8.4 deprecation. #170

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kagg-design
Copy link

Fixed: "Deprecated: str_getcsv(): the $escape parameter must be provided as its default value will change."

Copy link
Collaborator

@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.

Nice find!

My two pennies:

I think the @noinspection comment should probably be removed as that is an IDE specific thing.

I'd also like to see at least one test covering this change to safeguard it for the future.

@kagg-design
Copy link
Author

kagg-design commented Dec 2, 2024

Hi @jrfnl! Thank you for your comment.

I have removed // @noinspection ... line.

Concerning tests, I have the following thoughts.

  1. This is pure refactoring.
  2. As far as I can see, this method was not covered by tests, so should I write a new test for it?
  3. This line is called 19 times during tests of the Function Mocker; you can see my merged PR with 8.4 updates for it. Should it be considered as enough check?

Thank you.

@jrfnl
Copy link
Collaborator

jrfnl commented Dec 2, 2024

Concerning tests, I have the following thoughts.

  1. This is pure refactoring.
  2. As far as I can see, this method was not covered by tests, so should I write a new test for it?
  3. This line is called 19 times during tests of the Function Mocker; you can see my merged PR with 8.4 updates for it. Should it be considered as enough check?

@kagg-design Sorry, but no, tests in an external project should - IMO - definitely not be considered enough of a check.

The existing tests for this package are currently passing on PHP 8.4, so this means this code is currently not covered by tests. And even though I checked both the tests and did various scans on the code base with tooling when preparing for PHP 8.4 (see #157, #160, #161, #165, #166), this one wasn't found as it was "hidden" in a callback string.

Considering PHP intends to make further changes to this function in future PHP versions (based on the RFC and discussions I've had about this in the PHP src GH PR and the Internals mailing list), I think it is only prudent to make sure the code is covered by tests in this project to be sure that, when the next change lands in PHP, we can verify things continue to work as expected.

Aside from that, having a test in place will also safeguard that someone doesn't accidentally remove the fix at some point in the future.

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.

2 participants