Skip to content

Review which characters should be trimmed for cookie and header data #1084

Description

@jrfnl

Setting the stage

By default, the PHP native [lr]trim() functions, trim ASCII whitespace and the NUL byte character.
PHP 8.6 changes the default value of the $characters parameter to also include the form feed - "\f" - character, which was previously not trimmed.

⚠️ Keep in mind that [lr]trim() only operates on the leading and/or trailing characters for a text string. It does not affect the characters in the "middle" !

Refs:

What has already been done

Requests uses [lr]trim() in various places throughout the codebase.
To make this code PHP cross-version compatible, PR #1083 made the following changes:

  • Introduced two class constants to represent the different default values for the $characters parameter.
  • Made the $characters being trimmed explicit in each of the [lr]trim() function calls (if it wasn't already).

To determine which characters should be trimmed, the following rule of thumb has been used for the initial changes made in PR #1083:
"Use the PHP 8.6 default (Trim::WHITESPACE_CHARS), except when the trimming may be subject to an RFC or other documented rules, in which case use the PHP < 8.6 default (Trim::WHITESPACE_CHARS_NO_FF)"

That way, we preserved existing behaviour in "important" places, while benefitting from the new default value everywhere else.

Task

For all calls to [lr]trim() which currently use the Trim::WHITESPACE_CHARS_NO_FF value for $characters, it should be researched whether that is correct by checking RFCs and other applicable rules. In some cases, it may also be prudent to evaluate how other projects similar to Requests, like for instance Guzzle, handle these text strings.

If the trimming is correct, it should be documented that this was verified via a comment in this issue.

If a change is necessary, please submit a PR and include tests safeguarding the change to the value passed for $characters.

After PR #1083, there will be 14 places in the codebase where this review task is necessary.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions