Skip to content

More Precision for Float to String Casts #4479

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

Merged
merged 3 commits into from
May 26, 2025
Merged

Conversation

oleibman
Copy link
Collaborator

Fix #3899. Supersedes PR #4476, which will be changed to draft status and closed if this PR is merged.

A standard cast from float to string in PHP can drop trailing decimal positions. This can lead to problems above and beyond the usual problems associated with floating point. See the superseded PR for a more complete explanation.

StringHelper::convertToString is changed for how it handles floats. It will now do separate casts for the whole and decimal parts, and then combine the results. This affects Cell::getValueString and Cell::getCalculatedValueString. Xlsx Writer will now invoke convertToString before writing a float to Xml. Ods Writer already uses getValueString, so no change is needed there. Xls Writer writes its float values in binary, so no change is needed there. Tests are added for all 3 writers.

Aside from fixing some problems, it might appear that this change introduces some new problems. For instance, setting a cell to 12345.6789 will now result in 12345.67890000000079 in the Xml. This difference is an illusion, merely a consequence of floating point rounding. If you run the following check under PhpUnit, it will pass:

self::assertSame(12345.6789, 12345.67890000000079);

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

Fix PHPOffice#3899. Supersedes PR PHPOffice#4476, which will be changed to draft status and closed if this PR is merged.

A standard cast from float to string in PHP can drop trailing decimal positions. This can lead to problems above and beyond the usual problems associated with floating point. See the superseded PR for a more complete explanation.

`StringHelper::convertToString` is changed for how it handles floats. It will now do separate casts for the whole and decimal parts, and then combine the results. This affects `Cell::getValueString` and `Cell::getCalculatedValueString`. Xlsx Writer will now invoke `convertToString` before writing  a float to Xml. Ods Writer already uses `getValueString`, so no change is needed there. Xls Writer writes its float values in binary, so no change is needed there. Tests are added for all 3 writers.

Aside from fixing some problems, it might appear that this change introduces some new problems. For instance, setting a cell to `12345.6789` will now result in `12345.67890000000079` in the Xml. This difference is an illusion, merely a consequence of floating point rounding. If you run the following check under PhpUnit, it will pass:
```php
self::assertSame(12345.6789, 12345.67890000000079);
```
@oleibman
Copy link
Collaborator Author

Scrutinizer "error" is, once again, a false positive caused by its inability to properly interpret @return string[]. It has been suppressed.

@oleibman oleibman added this pull request to the merge queue May 26, 2025
Merged via the queue into PHPOffice:master with commit 68fd711 May 26, 2025
14 checks passed
@oleibman oleibman deleted the issue4476 branch May 26, 2025 03:23
@hackel
Copy link

hackel commented Jun 4, 2025

@oleibman This should probably be documented somewhere, it was a breaking change for me. I was using Cell::getCalculatedValueString and all of a sudden started getting those long floating point tails in my imported data after upgrading. I switched to getCalculatedValue and updated my code to accept numbers, and all seems to be well, but the release notes don't mention this change at all.

@oleibman
Copy link
Collaborator Author

oleibman commented Jun 5, 2025

@hackel The release notes for 4.3.0 include the following:

More Precision for Float to String Casts. Issue #3899 PR #4479

So your statement that it wasn't mentioned at all is not so. If you think there is a better way to have mentioned it, please suggest how and I will try to apply your suggestion to future release notes.

I am sorry that it was a breaking change for you. I do not understand why Php truncates its float to string casts, but its doing so was responsible for several problems which this change fixed. And, as I stated above, differences due to this change are a bit of an illusion (although apparently not so in your case). My apologies for the inconvenience.

@hackel
Copy link

hackel commented Jun 5, 2025

@oleibman You're right, I'm sorry I was looking at the release notes for 4.3.1. I was thinking that it might necessitate a major version bump, but I can see now how it's considered just a bugfix. Luckily the solution wasn't too difficult for me. Thanks for your work on this, it is appreciated very much!

@blizzz
Copy link

blizzz commented Jun 11, 2025

@oleibman thank you for the initiative; it does not seem to fix the issues I tackled in #4476 though, the time 13:37 is still interpreted as 13:36 for example.

@oleibman
Copy link
Collaborator Author

@blizzz Sorry this is still a problem for you. If you have a specific code/spreadsheet combination that is not working for you, I will take another look.

@blizzz blizzz mentioned this pull request Jun 20, 2025
11 tasks
@blizzz
Copy link

blizzz commented Jun 20, 2025

@blizzz Sorry this is still a problem for you. If you have a specific code/spreadsheet combination that is not working for you, I will take another look.

Thanks, I opened a PR with a failing test case containing at #4519

@oleibman
Copy link
Collaborator Author

@blizzz I will carry on this discussion in your PR rather than continuing to add to this closed one.

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.

Non-backwards compatible rounding issue in 2.0.0 - DateTime
3 participants