PhpSpreadsheet
PhpSpreadsheet copied to clipboard
Use Only mb_convert_encoding in StringHelper sanitizeUTF8
This started out as a slightly different fix, but experimenting showed it could be simplified. SanitizeUTF8 uses all of UConverter, iconv, and mb_convert_encoding, but the last of those 3 is sufficient on its own. So eliminate the other 2, incidentally eliminating the need for php-intl in this function. The history of this change follows:
Fix #2982. That issue is actually closed, but it did expose a problem. Our test environments all enable php-intl, but that extension isn't a formal requirement for PhpSpreadsheet. Perhaps it ought to be. Nevertheless ...
Using UConverter for string translation solved some problems for us. However, it is only available when php-intl is enabled. The code tests if it exists before using it, so no big deal ... except it seems likely that the people reporting the issue not only did not have php-intl, but they do have their own autoloader which issues an exception when the class isn't found. The test for existence of UConverter defaulted to attempting to autoload it if not found. So, on a system without php-intl but with a custom autoloader, there is a problem. Code is changed to suppress autoload when testing UConverter existence.
Pending this fix, the workaround for this issue is to enable php-intl.
This is:
- [x] a bugfix
- [ ] a new feature
- [ ] refactoring
- [ ] additional unit tests
Checklist:
- [x] Changes are covered by unit tests
- [x] Changes are covered by existing unit tests
- [x] New unit tests have been added
- [ ] Code style is respected
- [ ] Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
- [ ] CHANGELOG.md contains a short summary of the change
- [ ] Documentation is updated as necessary
Why this change is needed?
Provide an explanation of why this change is needed, with links to any Issues (if appropriate). If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.
As you can see, I eliminated php-intl in this module after all. I agree that suggesting it is still a good move; I know that you've been on the verge of it with some of the translation and locale and other work that you've been up to. I won't be including that with this change. However, now that the mitoteam changes are merged, I'm about to start work on replacing jpgraph with it in the test suite; that will require changes to composer.json, and I will add the php-intl suggestion at the same time.