Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Str::split() Incorrectly Trims Strings with Multiple-Character Separator #1753

Closed
neildaniels opened this issue May 6, 2019 · 4 comments

Comments

@neildaniels
Copy link
Contributor

commented May 6, 2019

Describe the bug
If you use Str::split() with a separator string that has multiple characters (e.g. ---), it will incorrectly trim the first and last elements.

To Reproduce

$str = <<<EOT
-abc-
---
-def-
EOT;

print_r(Str::split($str, '---'));

Outputs:

Array
(
    [0] => abc-
    [1] => -def
)

Expected behavior

Array
(
    [0] => -abc-
    [1] => -def-
)

Kirby Version
3.1.3

Additional context
The Str::split() function uses the php trim() function like so:

kirby/src/Toolkit/Str.php

Lines 802 to 803 in c9dcfb5

$string = trim((string)$string, $separator);
$parts = explode($separator, $string);

However, trim() uses the separator parameter as a pool of characters, not as a fixed string. This behavior differs from explode()'s behavior, which works on fixed strings.

In cases where $separator is a single character, the current behavior should always be fine.

In multi-character cases though, it should probably check if the input has a prefix/suffix of the separator and remove it based on string lengths?

@neildaniels neildaniels changed the title Str::split() Incorrectly Trims Strings with Multiple Character Separator Str::split() Incorrectly Trims Strings with Multiple-Character Separator May 6, 2019

@distantnative distantnative added this to the 3.2.1 milestone May 10, 2019

@bastianallgeier bastianallgeier modified the milestones: 3.2.1, 3.2.2 Jul 2, 2019

@bastianallgeier bastianallgeier modified the milestones: 3.2.2, 3.2.3 Jul 10, 2019

@distantnative

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

The trim line could be replaced with a preg_replace:

$string = preg_replace('/^' . $separator . '\s?|\s?'. $separator . '$/', '', (string)$string); 

@distantnative distantnative self-assigned this Jul 18, 2019

@lukasbestle

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Do we even need to trim the string at all? There's an empty value detection right below the explode() call anyway. I'd say the RegEx uses more processor time than the foreach loop.

@distantnative

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

You are right, I didn't check the code that follows. Removing the trim line should have no effect at all (or only the one that the issue is solved but no other side effects).

distantnative added a commit that referenced this issue Jul 18, 2019

@distantnative

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.