WIP Minor Breaking Change to DefaultValueBinder #4527
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix PHPOffice#4522. Although this is technically a breaking change, it is expected that very few, if any, existing programs will be affected. Nevertheless, DefaultValueBinder is implicitly used by the vast majority of programs out there, and I am reluctant to install a breaking change for something so widespread. So I will save this change for the next breaking release, which should be PhpSpreadsheet 5.0.0. There is no current schedule for that release. For strings consisting entirely of digits, DefaultValueBinder treats the value as a string if greater than PHP_INT_MAX, or an int otherwise. This prevents the loss of precision in large integers. This treatment dates back to PHPExcel, and has been in place since 2014. There are several problems with this approach. Excel itself maintains [15 digits of precision](https://support.microsoft.com/en-us/office/excel-specifications-and-limits-1672b34d-7043-467e-8e27-269d656771c3). So, string-vs-int should be decided at 999_999_999_999_999. This is much lower than PHP_INT_MAX for 64-bit, and much higher than for 32-bit. DefaultValueBinder is changed to use that new limit. A second problem is that DefaultValueBinder is only making this adjustment for positive integers. It is changed to test absolute value. A third problem is that DefaultValueBinder is making this adjustment only for strings, so that if you pass the number in as an int, it will not be adjusted (and will lose precision). It is changed to apply the same test for int.
|
My patience with Scrutinizer is just about done. It does not recognize the underscore numeric separator which was introduced in Php 7.4 over 5.5 years ago. I could eliminate the underscores, but I do not believe that we should be forced to adopt less clear code just to satisfy Scrutinizer. For now, I will just suppress the errors. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #4522. Although this is technically a breaking change, it is expected that very few, if any, existing programs will be affected. Nevertheless, DefaultValueBinder is implicitly used by the vast majority of programs out there, and I am reluctant to install a breaking change for something so widespread. So I will save this change for the next breaking release, which should be PhpSpreadsheet 5.0.0. There is no current schedule for that release.
For strings consisting entirely of digits, DefaultValueBinder treats the value as a string if greater than PHP_INT_MAX, or an int otherwise. This prevents the loss of precision in large integers. This treatment dates back to PHPExcel, and has been in place since 2014.
There are several problems with this approach. Excel itself maintains 15 digits of precision. So, string-vs-int should be decided at 999_999_999_999_999. This is much lower than PHP_INT_MAX for 64-bit, and much higher than for 32-bit. DefaultValueBinder is changed to use that new limit.
A second problem is that DefaultValueBinder is only making this adjustment for positive integers. It is changed to test absolute value.
A third problem is that DefaultValueBinder is making this adjustment only for strings, so that if you pass the number in as an int, it will not be adjusted (and will lose precision). It is changed to apply the same test for int.
This is:
Checklist: