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

Fix split filter for php >=8.0 in case of empty separator #213

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

oleksii-pavlikovskii-eleks-com

For php >8.0 split filter throws ValueError when separator parameter is an empty string.

  • I've run the tests with vendor/bin/phpunit (php 8.2)
  • None of the tests were found failing
  • I've seen the coverage report at build/coverage/index.html
  • Not a single line left uncovered by tests
  • Any coding standards issues were fixed with vendor/bin/php-cs-fixer fix

tests/Liquid/StandardFiltersTest.php Outdated Show resolved Hide resolved
tests/Liquid/StandardFiltersTest.php Outdated Show resolved Hide resolved
tests/Liquid/StandardFiltersTest.php Outdated Show resolved Hide resolved
tests/Liquid/StandardFiltersTest.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@sanmai sanmai left a comment

Choose a reason for hiding this comment

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

There's a failing test. Please fix and request re-review.

return mb_str_split($input);
}

if (empty($pattern)) {
Copy link
Collaborator

@sanmai sanmai Mar 19, 2024

Choose a reason for hiding this comment

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

There's a problem in this condition. Can you see it?

@@ -621,6 +621,10 @@ public static function split($input, $pattern)
return [];
}

if ($pattern === '') {
return mb_str_split($input);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Matches the behavior of the upstream project 👍

Copy link
Collaborator

@sanmai sanmai left a comment

Choose a reason for hiding this comment

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

Thanks!

@sanmai sanmai merged commit de705d5 into kalimatas:master Mar 19, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants