Skip to content

GH-20783: support \f in trim, rtrim and ltrim#20788

Open
LamentXU123 wants to merge 22 commits intophp:masterfrom
LamentXU123:master
Open

GH-20783: support \f in trim, rtrim and ltrim#20788
LamentXU123 wants to merge 22 commits intophp:masterfrom
LamentXU123:master

Conversation

@LamentXU123
Copy link
Contributor

@LamentXU123 LamentXU123 commented Dec 27, 2025

I searched keyword trim in the codebase and fix every case I think related to the three functions. I didn't touch other features with trim implemented and missing \f. (such as trim in php filters or other code)
This PR changes:

  • The default parameters characters of trim (also ltrim and rtrim) in the php interface, adding \f, source
  • The behavior in php_trim_int (which effects php_trim), source
  • Test case concerning trim, adding case of \f.

I am sending mails to internals now. After that I will write the NEWS file (and docs) if this feature is accepted.
doc of trim: https://www.php.net/manual/en/function.trim.php
This is my first time contributing to php so I am not so sure about this PR. Thanks.

ps: also a little detail. I change the default parameter to \f\n\t\r\v\0 using alphabetic orders. (abcdefghijklmn...)

@LamentXU123 LamentXU123 requested a review from bukka as a code owner December 27, 2025 10:16
@LamentXU123 LamentXU123 marked this pull request as draft December 27, 2025 12:25
@LamentXU123 LamentXU123 marked this pull request as ready for review March 7, 2026 15:40
@LamentXU123
Copy link
Contributor Author

RFC (https://wiki.php.net/rfc/trim_form_feed) implemented, with specific test case.
I am convinced it is ready to be reviewed.

@LamentXU123 LamentXU123 requested a review from devnexen March 7, 2026 15:44
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

The default value specified in ext/standard/basic_functions.stub.php also needs to be adjusted.

@LamentXU123 LamentXU123 requested a review from TimWolla March 8, 2026 04:21
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

The implementation itself LGTM now.

@TimWolla TimWolla added the RFC label Mar 8, 2026
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.

7 participants