-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 PHP8.2 str_split function returns empty arrays for empty strings #3341
base: master
Are you sure you want to change the base?
Fix PHP8.2 str_split function returns empty arrays for empty strings #3341
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we are actually subject to any failures as a result of the Php change, but I think mb_substr_split is probably what should be using anyhow (in addition to other mb_ functions in lieu of the straight string equivalents). Can you please add some null string tests to tests\data\Calculation\Engineering\Hex2Dec
([0, '""'],
), tests\data\Calculation\Engineering\Oct2Dec
(['0', '""'],
) and tests\data\Calculation\MathTrig\Arabic
([0, ''],
), just to demonstrate that they work as in Excel.
The Mbstring extension is not bundled with every distribution of PHP by default. If the issue is splitting on an empty string, would it not be better to check for that case prior to calling
Would there ever be a false-y |
It's tagged as a required dependency in I'd also like to see some tests to identify any input $value where the change to |
I don't think it would be practical to use PhpSpreadsheet if mbstring is not enabled. Mbstring functions are directly called 45 times in 38 different modules. In addition, StringHelper methods, many of which use mbstring functions, are invoked in 186 different modules. |
It would be a very limited subset of functionality, but not impossible: for character conversion, we try to use iconv where possible, with mb_ functions as a fallback We also have our own |
0b350d5
to
4a55e40
Compare
I see that you've added some unit tests; but I don't see any tests that demonstrate there is a bug unless this change to use |
This is:
Checklist:
Why this change is needed?
In PHP 8.2, the str_split function will returns empty arrays for empty strings.
See: https://php.watch/versions/8.2/str_split-empty-string-empty-array
We can use mb_str_split() instead