Skip to content

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

Merged
samdark merged 15 commits into
masterfrom
mb-trim
Oct 3, 2026

Conversation

@vjik

@vjik vjik commented Sep 15, 2026

Copy link
Copy Markdown
Member
Q A
Is bugfix? ❌
New feature? ✔️
Breaks BC? ❌
Fix #130

@codecov

codecov Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (90a34d6) to head (9a27ebb).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 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.

@vjik
vjik requested a review from a team September 15, 2026 20:25
@vjik vjik added the status:code review The pull request needs review. label Sep 15, 2026
@samdark

samdark commented Sep 15, 2026

Copy link
Copy Markdown
Member

What's your reasoning for introducing new attributes instead of making existing one unicode-aware?

@vjik

vjik commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

What's your reasoning for introducing new attributes instead of making existing one unicode-aware?

trim() and mb_trim() process "characters" differently. trim supports ranges, but mb_trim() does not. Also mb_trim() supports from PHP 8.4 or requires polyfill.

@samdark

samdark commented Sep 16, 2026

Copy link
Copy Markdown
Member

Yes but is there a reason to use the one with no unicode support?

@vjik

vjik commented Sep 20, 2026

Copy link
Copy Markdown
Member Author

Yes but is there a reason to use the one with no unicode support?

Why not? I think such situations can happen currently.

For mb_tim() using user should use PHP 8.4 or install polyfill, it’s not always possible or meaningful.

I suggest to merge this PR, but in future, after drop PHP 8.3 support, use mb_* always (revert changes from this PR and just replace trim() to mb_trim())

}

return Result::success(
mb_ltrim($resolvedValue, $attribute->characters ?? $this->characters),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ability to specify encoding is missing. It should be UTF-8 by default, but users should be able to specify it as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

}

return Result::success(
mb_rtrim($resolvedValue, $attribute->characters ?? $this->characters),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ability to specify encoding is missing. It should be UTF-8 by default, but users should be able to specify it as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

}

return Result::success(
mb_trim($resolvedValue, $attribute->characters ?? $this->characters),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ability to specify encoding is missing. It should be UTF-8 by default, but users should be able to specify it as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@samdark

samdark commented Sep 20, 2026

Copy link
Copy Markdown
Member

@vjik

Having both Trim and MultibyteTrim is confusing. I'd use

#[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 multibyte false but change the default in next release.

@samdark

samdark commented Sep 20, 2026 •

Copy link
Copy Markdown
Member

We can introduce support for ranges for the multibyte version. At least for ASCII ones.

@vjik

vjik commented Sep 20, 2026

Copy link
Copy Markdown
Member Author
#[Trim(multibyte: true)]

$characters in trim() and mb_trim() are different parameter:

  • trim() supports ranges, e.g. a..z
  • mb_trim() doesn't support ranges

So, we don't can to use one parameter for multibyte and non-multibyte modes.

I add multibyte parameter to ToArrayOfStrings because it doesn't contain $characters parameter.

@vjik

vjik commented Sep 20, 2026

Copy link
Copy Markdown
Member Author

We can introduce support for ranges for the multibyte version. At least for ASCII ones.

I think is overhead in this case. Better drop support of ranges)

@vjik vjik added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Sep 20, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 Medium severity

Open (4)

Comment thread src/Attribute/Parameter/LeftTrimResolver.php Outdated
Comment thread src/Attribute/Parameter/RightTrimResolver.php Outdated
Comment thread src/Attribute/Parameter/ToArrayOfStringsResolver.php Outdated
Comment thread src/Attribute/Parameter/TrimResolver.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The implementation preserves existing defaults, handles optional dependencies, and provides comprehensive cross-version coverage.

Review effort: Balanced
Findings: None

Resolved since last review (4)

@vjik vjik added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Sep 20, 2026
@vjik
vjik requested a review from samdark September 20, 2026 15:58
for ($i = 0; $i < $count; $i++) {
$char = $chars[$i];

if ($i + 3 < $count && $chars[$i + 1] === '.' && $chars[$i + 2] === '.') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 ..

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#[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, '..')) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Won't work for encodings such as UTF-32.

@vjik
vjik requested a review from samdark September 30, 2026 07:21
@samdark

samdark commented Sep 30, 2026

Copy link
Copy Markdown
Member

@vjik

vjik commented Sep 30, 2026 •

Copy link
Copy Markdown
Member Author

@samdark samdark changed the title Multibyte trim support 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 Oct 3, 2026
@samdark
samdark merged commit e67a852 into master Oct 3, 2026
30 checks passed
@samdark

samdark commented Oct 3, 2026

Copy link
Copy Markdown
Member

👍

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

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TrimResolver doesn't respect UTF-8

3 participants