WordPress / WordPress/Requests
Review which characters should be `trim`med for cookie and header data
Nobody has claimed this yet.
- 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
$charactersparameter. - Made the
$charactersbeing 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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