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

Use Only mb_convert_encoding in StringHelper sanitizeUTF8 #2994

Merged
merged 4 commits into from
Aug 13, 2022

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented Aug 7, 2022

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:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • 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.

Fix PHPOffice#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.
Make mb_convert_encoding use same substitution character as UConverter, ensuring consistent results whatever the user's environment.
Copy link
Member

@MarkBaker MarkBaker left a comment

Choose a reason for hiding this comment

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

I'd probably also be inclined to add php-intl in the composer.json as a suggestion.

For one of my existing PRs, I'd been looking at duplicating the test suite (one set with php-intl, the other without), but hadn't done so because I couldn't see how to merge the code coverage

Since mb_convert_encoding can now return the same output as UConverter, we don't need UConverter (or iconv) after all in sanitizeUTF8.
@oleibman oleibman changed the title Test if UConverter Exists Without Autoload Use Only mb_convert_encoding in StringHelper sanitizeUTF8 Aug 7, 2022
@oleibman
Copy link
Collaborator Author

oleibman commented Aug 7, 2022

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.

@oleibman oleibman merged commit 0492ea6 into PHPOffice:master Aug 13, 2022
@oleibman oleibman deleted the uconverter branch September 2, 2022 00:38
MarkBaker added a commit that referenced this pull request Sep 25, 2022
### Added

- Implementation of the new `TEXTBEFORE()`, `TEXTAFTER()` and `TEXTSPLIT()` Excel Functions
- Implementation of the `ARRAYTOTEXT()` and `VALUETOTEXT()` Excel Functions
- Support for [mitoteam/jpgraph](https://packagist.org/packages/mitoteam/jpgraph) implementation of
  JpGraph library to render charts added.
- Charts: Add Gradients, Transparency, Hidden Axes, Rounded Corners, Trendlines, Date Axes.

### Changed

- Allow variant behaviour when merging cells [Issue #3065](#3065)
  - Merge methods now allow an additional `$behaviour` argument. Permitted values are:
    - Worksheet::MERGE_CELL_CONTENT_EMPTY - Empty the content of the hidden cells (the default behaviour)
    - Worksheet::MERGE_CELL_CONTENT_HIDE - Keep the content of the hidden cells
    - Worksheet::MERGE_CELL_CONTENT_MERGE - Move the content of the hidden cells into the first cell

### Deprecated

- Axis getLineProperty deprecated in favor of getLineColorProperty.
- Moved majorGridlines and minorGridlines from Chart to Axis. Setting either in Chart constructor or through Chart methods, or getting either using Chart methods is deprecated.
- Chart::EXCEL_COLOR_TYPE_* copied from Properties to ChartColor; use in Properties is deprecated.
- ChartColor::EXCEL_COLOR_TYPE_ARGB deprecated in favor of EXCEL_COLOR_TYPE_RGB ("A" component was never allowed).
- Misspelled Properties::LINE_STYLE_DASH_SQUERE_DOT deprecated in favor of LINE_STYLE_DASH_SQUARE_DOT.
- Clone not permitted for Spreadsheet. Spreadsheet->copy() can be used instead.

### Removed

- Nothing

### Fixed

- Fix update to defined names when inserting/deleting rows/columns [Issue #3076](#3076) [PR #3077](#3077)
- Fix DataValidation sqRef when inserting/deleting rows/columns [Issue #3056](#3056) [PR #3074](#3074)
- Named ranges not usable as anchors in OFFSET function [Issue #3013](#3013)
- Fully flatten an array [Issue #2955](#2955) [PR #2956](#2956)
- cellExists() and getCell() methods should support UTF-8 named cells [Issue #2987](#2987) [PR #2988](#2988)
- Spreadsheet copy fixed, clone disabled. [PR #2951](#2951)
- Fix PDF problems with text rotation and paper size. [Issue #1747](#1747) [Issue #1713](#1713) [PR #2960](#2960)
- Limited support for chart titles as formulas [Issue #2965](#2965) [Issue #749](#749) [PR #2971](#2971)
- Add Gradients, Transparency, and Hidden Axes to Chart [Issue #2257](#2257) [Issue #2229](#2929) [Issue #2935](#2935) [PR #2950](#2950)
- Chart Support for Rounded Corners and Trendlines [Issue #2968](#2968) [Issue #2815](#2815) [PR #2976](#2976)
- Add setName Method for Chart [Issue #2991](#2991) [PR #3001](#3001)
- Eliminate partial dependency on php-intl in StringHelper [Issue #2982](#2982) [PR #2994](#2994)
- Minor changes for Pdf [Issue #2999](#2999) [PR #3002](#3002) [PR #3006](#3006)
- Html/Pdf Do net set background color for cells using (default) nofill [PR #3016](#3016)
- Add support for Date Axis to Chart [Issue #2967](#2967) [PR #3018](#3018)
- Reconcile Differences Between Css and Excel for Cell Alignment [PR #3048](#3048)
- R1C1 Format Internationalization and Better Support for Relative Offsets [Issue #1704](#1704) [PR #3052](#3052)
- Minor Fix for Percentage Formatting [Issue #1929](#1929) [PR #3053](#3053)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

UConverter
2 participants