Fix #131: Deprecate the .. range syntax in the characters parameter of Trim, LeftTrim and RightTrim attributes, add multibyte and encoding parameters to Trim, LeftTrim, RightTrim and ToArrayOfStrings attributes and their resolvers - #131
Conversation
vjik
commented
Sep 15, 2026
| Q | A |
|---|---|
| Is bugfix? | ❌ |
| New feature? | ✔️ |
| Breaks BC? | ❌ |
| Fix #130 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #131 +/- ##
============================================
Coverage 100.00% 100.00%
- Complexity 337 388 +51
============================================
Files 43 44 +1
Lines 796 913 +117
============================================
+ Hits 796 913 +117 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
The opt-in implementation preserves existing behavior and is covered across resolver, hydration, configuration, and Unicode cases.
Pull request overview
Adds opt-in Unicode-aware trimming while preserving existing byte-oriented trim behavior.
Changes:
- Adds multibyte trim attributes and resolvers.
- Adds multibyte mode to array-string conversion.
- Adds tests, documentation, dependency metadata, and changelog entries.
File summaries
| File | Description |
|---|---|
src/Attribute/Parameter/MultibyteTrim.php |
Adds two-sided multibyte trim attribute. |
src/Attribute/Parameter/MultibyteTrimResolver.php |
Implements two-sided multibyte trimming. |
src/Attribute/Parameter/MultibyteLeftTrim.php |
Adds left-side multibyte trim attribute. |
src/Attribute/Parameter/MultibyteLeftTrimResolver.php |
Implements left-side multibyte trimming. |
src/Attribute/Parameter/MultibyteRightTrim.php |
Adds right-side multibyte trim attribute. |
src/Attribute/Parameter/MultibyteRightTrimResolver.php |
Implements right-side multibyte trimming. |
src/Attribute/Parameter/Trim.php |
Documents the multibyte alternative. |
src/Attribute/Parameter/LeftTrim.php |
Documents the multibyte alternative. |
src/Attribute/Parameter/RightTrim.php |
Documents the multibyte alternative. |
src/Attribute/Parameter/ToArrayOfStrings.php |
Documents resolver-level multibyte mode. |
src/Attribute/Parameter/ToArrayOfStringsResolver.php |
Adds optional multibyte element trimming. |
tests/Attribute/Parameter/MultibyteTrimTest.php |
Tests two-sided multibyte trimming. |
tests/Attribute/Parameter/MultibyteLeftTrimTest.php |
Tests left-side multibyte trimming. |
tests/Attribute/Parameter/MultibyteRightTrimTest.php |
Tests right-side multibyte trimming. |
tests/Attribute/Parameter/TrimTest.php |
Confirms existing byte-oriented behavior. |
tests/Attribute/Parameter/LeftTrimTest.php |
Confirms existing left-trim behavior. |
tests/Attribute/Parameter/RightTrimTest.php |
Confirms existing right-trim behavior. |
tests/Attribute/Parameter/ToArrayOfStringsTest.php |
Tests default and multibyte array trimming. |
docs/guide/en/typecasting.md |
Documents multibyte trimming usage. |
composer.json |
Adds the development polyfill and runtime suggestions. |
composer-dependency-analyser.php |
Accounts for the optional polyfill dependency. |
CHANGELOG.md |
Records the new functionality. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
What's your reasoning for introducing new attributes instead of making existing one unicode-aware? |
|
|
Yes but is there a reason to use the one with no unicode support? |
Why not? I think such situations can happen currently. For I suggest to merge this PR, but in future, after drop PHP 8.3 support, use |
| } | ||
|
|
||
| return Result::success( | ||
| mb_ltrim($resolvedValue, $attribute->characters ?? $this->characters), |
There was a problem hiding this comment.
The ability to specify encoding is missing. It should be UTF-8 by default, but users should be able to specify it as well.
| } | ||
|
|
||
| return Result::success( | ||
| mb_rtrim($resolvedValue, $attribute->characters ?? $this->characters), |
There was a problem hiding this comment.
The ability to specify encoding is missing. It should be UTF-8 by default, but users should be able to specify it as well.
| } | ||
|
|
||
| return Result::success( | ||
| mb_trim($resolvedValue, $attribute->characters ?? $this->characters), |
There was a problem hiding this comment.
The ability to specify encoding is missing. It should be UTF-8 by default, but users should be able to specify it as well.
|
Having both #[Trim(multibyte: true)]While the current PR solution makes sense from the backwards compatibility standpoint, I think in the modern web the majority of input will be UTF-8. So for current release I'd keep |
|
We can introduce support for ranges for the multibyte version. At least for ASCII ones. |
So, we don't can to use one parameter for multibyte and non-multibyte modes. I add |
I think is overhead in this case. Better drop support of ranges) |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Automatic multibyte selection changes existing default behavior despite the stated backward-compatibility guarantee.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
| for ($i = 0; $i < $count; $i++) { | ||
| $char = $chars[$i]; | ||
|
|
||
| if ($i + 3 < $count && $chars[$i + 1] === '.' && $chars[$i + 2] === '.') { |
There was a problem hiding this comment.
Comparison with . won't work for $encodings such as UTF-16LE, UTF-16BE, or UTF-32.
$dot = mb_chr(0x2E, $encoding);Then use it where you currently compare with ..
There was a problem hiding this comment.
#[TestWith(['UTF-16LE'])]
#[TestWith(['UTF-16BE'])]
#[TestWith(['UTF-32LE'])]
#[TestWith(['UTF-32BE'])]
public function testExpandRangesWithEncoding(string $encoding): void
{
$characters = mb_convert_encoding('a..z', $encoding, 'UTF-8');
$expected = mb_convert_encoding(
'abcdefghijklmnopqrstuvwxyz',
$encoding,
'UTF-8',
);
$this->assertSame(
$expected,
TrimCharacters::expandRanges($characters, $encoding),
);
}Also, the malformed range test can have an extra case:
#[TestWith([
"a\0.\0.\0",
'UTF-16LE',
"Invalid '..'-range, no character to the right of '..'",
"a\0.\0",
])]| return; | ||
| } | ||
|
|
||
| if (str_contains($characters, '..')) { |
There was a problem hiding this comment.
Won't work for encodings such as UTF-32.
.. range syntax in the characters parameter of Trim, LeftTrim and RightTrim attributes, add multibyte and encoding parameters to Trim, LeftTrim, RightTrim and ToArrayOfStrings attributes and their resolvers
|
👍 |
