Skip to content

Conversation

@LamentXU123
Copy link

@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

if (c <= ' ' &&
(c == ' ' || c == '\n' || c == '\r' || c == '\t' || c == '\v' || c == '\0')) {
(c == ' ' || c == '\f' || c == '\n' || c == '\r' || c == '\t' || c == '\v' || c == '\0')) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: personal opinion would be nice to share a helper for both cases e.g.

static zend_always_inline bool php_is_whitespace(char c)
{
    ...
}

but no need to bother now until the topic is discussed at least.

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