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

Inconsistency Between Actual and Declared Type - Minor Break #3715

Merged
merged 5 commits into from
Sep 8, 2023

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented Sep 6, 2023

Fix #3711. User set a cell value to float (implicitly by default value binder), then used setDataType to change its type to string. This caused a problem for Xlsx Writer, which uses the string cell values as an index into a Shared String array. However, as the cell actually contained a floating point value, Php treated it as an integer index; such a treatment is both deprecated, and leads to invalid values in the spreadsheet.

The use case for setDataType is not strong. The user always has the option to use setValueExplicit if the type is important. Setting a type afterwards, i.e. irrespective of the value, seems like a peculiar action. Indeed, there are no tests whatever for such use in the unit test suite.

There are two possible approaches to fixing this problem. The first is to add casts to the 3 or 4 places in Writer Xlsx which might be affected by this problem (hoping that you've found them all and realizing that similar changes might be needed for other Writers). The second is to change setDataType to call setValueExplicit using the current value of the cell, thereby possibly changing the cell value. I have gone with the second option - it seems like a much more logical approach, and guarantees that the content of the cell will always be consistent with its declared type. It is, however, a breaking change; if, for example, you have a cell with a string or numeric value and specify boolean to setDataType, the cell's value will change to true or false with no way to get back to the original.

This is:

  • 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 and a link to the pull request if applicable
  • 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#3711. User set a cell value to float (implicitly by default value binder), then used `setDataType` to change its type to string. This caused a problem for Xlsx Writer, which uses the string cell values as an index into a Shared String array. However, as the cell actually contained a floating point value, Php treated it as an integer index; such a treatment is both deprecated, and leads to invalid values in the spreadsheet.

The use case for `setDataType` is not strong. The user always has the option to use `setValueExplicit` if the type is important. Setting a type afterwards, i.e. irrespective of the value, seems like a peculiar action. Indeed, there are no tests whatever for such use in the unit test suite.

There are two possible approaches to fixing this problem. The first is to add casts to the 3 or 4 places in Writer Xlsx which might be affected by this problem (hoping that you've found them all and realizing that similar changes might be needed for other Writers). The second is to change `setDataType` to call `setValueExplicit` using the current value of the cell, thereby possibly changing the cell value. I have gone with the second option - it seems like a much more logical approach, and guarantees that the content of the cell will always be consistent with its declared type. It is, however, a breaking change; if, for example, you have a cell with a string or numeric value and specify `boolean` to `setDataType`, the cell's value will change to `true` or `false` with no way to get back to the original.
Consistent with work being done in PR PHPOffice#3718.
Better match to original issue.
@oleibman oleibman merged commit 52f5b24 into PHPOffice:master Sep 8, 2023
10 checks passed
@oleibman oleibman deleted the issue3711b branch November 13, 2023 15:38
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.

$spreadsheet->getCell()->setValue($val) unexpected result sometimes (no idea why)
1 participant