WordPress / WordPress/Requests

Review which characters should be `trim`med for cookie and header data

Open
#1,084 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Status: help wanted Type: testing/chores/QA
Dominant language
PHP
Stars
3.6k
Forks
500
Avg merge
2d 21h
Merged PRs (30d)
5

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Review the 14 [lr]trim() calls using Trim::WHITESPACE_CHARS_NO_FF, starting with the changes in PR #1083. Check the trim form-feed RFC, PHP documentation, and comparable handling in projects such as Guzzle. Document verified choices in this issue; where a value should change, add tests covering the $characters value and submit a PR.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
62/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.