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

Bug in "Trunc" class #4113

Closed
1 of 8 tasks
Alan-FSantana opened this issue Jul 26, 2024 · 13 comments · Fixed by #4115
Closed
1 of 8 tasks

Bug in "Trunc" class #4113

Alan-FSantana opened this issue Jul 26, 2024 · 13 comments · Fixed by #4115

Comments

@Alan-FSantana
Copy link

This is:

- [X] a bug report
- [ ] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

What is the expected behavior?

PhpOffice\PhpSpreadsheet\Calculation\MathTrig\Trunc::evaluate(1.01, 1); // expected: 1.0

What is the current behavior?

PhpOffice\PhpSpreadsheet\Calculation\MathTrig\Trunc::evaluate(1.01, 1); // current: 1.01

What are the steps to reproduce?

<?php

require __DIR__ . '/vendor/autoload.php';

echo PhpOffice\PhpSpreadsheet\Calculation\MathTrig\Trunc::evaluate(1.01, 1);

What features do you think are causing the issue

  • Reader
  • Writer
  • Styles
  • Data Validations
  • Formula Calculations
  • Charts
  • AutoFilter
  • Form Elements

Does an issue affect all spreadsheet file formats? If not, which formats are affected?

Formats that use formulas.

Which versions of PhpSpreadsheet and PHP are affected?

All?

@oleibman
Copy link
Collaborator

Confirmed. This appears to have been a bug for a long time. Researching.

@oleibman
Copy link
Collaborator

The following code in TRUNC is causing this problem:

        if (($digits > 0) && (rtrim((string) (int) ((abs($value) - abs((int) $value)) * $adjust), '0') < $adjust / 10)) {
            return $value;
        }

If I remove it, all of our unit tests continue to succeed, and this case now returns the expected result. I do not understand the purpose of this code, which dates back to PHPExcel, and, if possible, I would like a better understanding before I remove it. @MarkBaker @PowerKiKi any ideas on what it is supposed to do?

@Alan-FSantana
Copy link
Author

Alan-FSantana commented Jul 26, 2024

Without those lines, it fails with the case: $value = 10.04, $digits = 2
Maybe something like this should work:

    $str = json_encode($value);
    if (($p = strpos($str, '.')) !== false) {
        $value = (float) (substr($str, 0, $p + 1 + $digits));
    }

    return $value;

Stack Overflow link

@oleibman
Copy link
Collaborator

Thank you for the link and the additional example.

@oleibman
Copy link
Collaborator

It's hard to imagine that we need Json to solve this problem. I like the solution from Juan. I think it clearly needs more testing, but it works for both test cases you've submitted and all our existing tests.

        if ($value == 0) {
            return $value;
        }
        if ($value >= 0) {
            $originalNegative = false;
        } else {
            $originalNegative = true;
            $value = -$value;
        }
        $result = round($value - 5 * pow(10, -($digits + 1)), $digits);

        return $originalNegative ? -$result : $result;

@oleibman
Copy link
Collaborator

Sigh, that code breaks down in Php 8.4.

@oleibman
Copy link
Collaborator

The suggestion from savageman needs work (doesn't work for negative precision, and needs to use both sprintf and round). But it does seem to work in Php8.4, as well, of course, as 8.1-8.3.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Jul 27, 2024
Fix PHPOffice#4113. TRUNC isn't always producing the expected result. There was a promising algorithm at https://stackoverflow.com/questions/4668628/truncate-float-numbers-with-php from user Juan. It works through Php8.3, but failed in Php8.4 (more on this later). User Savageman on the same page has a solution that needs work, but, once the work had taken place, it works on Php8.1-8.4.

The ROUNDUP and ROUNDDOWN functions were adversely affected by Php8.4, probably for the same reasons as Juan's TRUNC suggestion. I put a kludge in place for them some time ago, but I wasn't happy with it. The solution used for TRUNC here suggested a change to the ROUNDUP and ROUNDDOWN code that would no longer require the kludge. The change to those functions now works more cleanly on Php8.1-8.4.
@Alan-FSantana
Copy link
Author

Hi, thanks for your work on this issue!
I did some tests using Php 3.9 and didn't find any failure with the Juan's solution, but some cases failed with the new function in Trunc class:

$value = -43747.99122596; $digits = 0; // float(-43748)
$value = -9.1196419; $digits = 2; // float(-9.12)
$value = -42300.65099338; $digits = 3; // float(-42300.651)

Obs: an alternative for json_enconde is var_export($value, true).

@oleibman
Copy link
Collaborator

Thanks for keeping me honest :-) Please try again with the latest push.

@Alan-FSantana
Copy link
Author

You're welcome! =)

There is a problem with numbers that are represented in the exponential form after converting to string.
Example: $value = 0.0000123; // (string) = '1.23e-5'

Also, prior to PHP 8.0, float to string conversions depended on the current locale set with the setlocale function (except for the functions var_export, json_encode and serialize).

https://php.watch/versions/8.0/float-to-string-locale-independent

@oleibman
Copy link
Collaborator

oleibman commented Jul 30, 2024

Well, we're certainly going to have the world's greatest truncater when we're finished. Please try the latest push. We do not support Php releases prior to 8.1, so there is no concern about locale ... oh, rats, it appears I should have used F rather than f as my argument to sprintf. Another push coming soon.

@Alan-FSantana
Copy link
Author

Excellent! No problems found this time ;)

@oleibman
Copy link
Collaborator

Changes to use non-locale-aware formats for sprintf are now pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants