Skip to content

Ban strtok() from the codebase #57542

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.

Sign up for GitHub

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

Jump to bottom
Closed
nicolas-grekas opened this issue Jun 26, 2024 · 7 comments
Closed

Ban strtok() from the codebase #57542

nicolas-grekas opened this issue Jun 26, 2024 · 7 comments
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them.

Comments

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 26, 2024 •
edited
Loading

strtok() is considered for deprecation in https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_strtok_function
whether or not this it accepted, we shouldn't use this function, for the same reasons that it might be considered: this is a stateful function.

We should do this on branch 5.4, same as we do when fixing php-src deprecations.
@derrabus derrabus added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Jun 26, 2024
@derrabus
Copy link
Member

If the function is not deprecated, I'd prefer to do this change on 7.2 tbh. Do you know when this will be voted on?

@wouterj
Copy link
Member

wouterj commented Jun 26, 2024

Note that we only use this function in the stateless way.

@alexandre-daubois alexandre-daubois mentioned this issue Jun 27, 2024
Merged
@alexandre-daubois
Copy link
Member

Do you know when this will be voted on?

The debate looks particularly heated, I guess it could take some time before the vote being triggered

@nicolas-grekas
Copy link
Member Author

Note that we only use this function in the stateless way.

yes but no: the function still keeps the string in memory in case the stateful API is used, there is no way around (except maybe calling strtok again with the empty string after calling it...)

@nicolas-grekas
Copy link
Member Author

Do you know when this will be voted on?

In ~2 weeks: 8.4 freezes on August 13th

@that-guy-iain
Copy link
Contributor

Considering how often PHP deprecates a function and then undeprecates it in later releases may it would be worth while waiting until the next release?

@nicolas-grekas
Copy link
Member Author

We don't really need php-src's lead here. The statefulness of the function is enough to ban it.

nicolas-grekas added a commit that referenced this issue Jun 27, 2024
@nicolas-grekas
minor #57550 [Lock][Process] Replace strtok calls (alexandre-daubois)
03d2bff 
This PR was merged into the 7.2 branch.

Discussion
----------

[Lock][Process] Replace `strtok` calls

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | #57542
| License       | MIT

I targeted 5.4, I think this is a good thing in order to be future-proof and help potential fixes upmerges, strtok being deprecated or not. Rebasing on 7.2 is not a big deal though if needed, as I only found 1 more occurrence on the latest branch.

Commits
-------

c76ba0d [Lock][Process] Replace `strtok` calls
@derrabus derrabus mentioned this issue Jun 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nicolas-grekas @wouterj @derrabus @alexandre-daubois @that-guy-iain