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

suppressFormulaErrors is not configurable #1531

Closed
lekoala opened this issue Jun 16, 2020 · 13 comments · Fixed by #3092
Closed

suppressFormulaErrors is not configurable #1531

lekoala opened this issue Jun 16, 2020 · 13 comments · Fixed by #3092

Comments

@lekoala
Copy link

lekoala commented Jun 16, 2020

This is:

- [ ] a bug report
- [x] 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?

If a formula is invalid, the behaviour is controlled by Calculation::raiseFormulaError that either throws an exception or triggers an error

This is controlled by the suppressFormulaErrors public variable, but there is no easy way to configure that variable at the spreadsheet level. More over, having a public variable is a bit strange (why not a config option on the spreadsheet with proper getters/setters?). Also the doc blocks of raiseFormulaError are not in line with regular docblocks

// trigger an error, but nicely, if need be
protected function raiseFormulaError($errorMessage)

It would be nice to actually allow suppressing errors to allow excel generation and not block the process because of one error in a formula (currently, both errors and exception will block the generation of the file)

As an added bonus, it would be nice if the error message could include the actual formula, in large excel file in can be a real mess to find back which formula is causing the error and why it's invalid. having the cell reference is nice, but the actual formula would be super helpful in my opinion.

What is the current behavior?

If a formula contains an error, it will trigger an error. I don't see how to throw an exception, although it wouldn't suppress anything (contrary to what the variable name would let me believe)

If you want to skip errors, you have to comment out everything in the raiseFormulaError method

// trigger an error, but nicely, if need be
protected function raiseFormulaError($errorMessage)
{
    $this->formulaError = $errorMessage;
    $this->cyclicReferenceStack->clear();
    // if (!$this->suppressFormulaErrors) {
        // throw new Exception($errorMessage);
    // }
    // trigger_error($errorMessage, E_USER_ERROR);

    return false;
}

What are the steps to reproduce?

Just make any sheet with a formula error in it, it will show a Formula Error

<?php

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

// Create new Spreadsheet object
$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
// add code that show the issue here...
$sheet->setCellValueByColumnAndRow(1, 1, '=SOMEERRORFUNCTION(A1:A10)');

$writer = IOFactory::createWriter($spreadsheet, 'Xlsx');
$writer->save('php://output');

Which versions of PhpSpreadsheet and PHP are affected?

1.8.2 to 1.13

@oleibman
Copy link
Collaborator

When I correct the statement "$sheet = $excel->getActiveSheet()" to
"$sheet = $spreadsheet->getActiveSheet()", the sheet saves correctly.
Do you still see a problem if you make that change?

@lekoala
Copy link
Author

lekoala commented Jun 16, 2020

I haven't tried the code I posted to be honest :) I'll try to see if I can replicate the issue I have on my project which is much more complex

@skillbox-koutja
Copy link

I have same trouble. Fix please

@lekoala
Copy link
Author

lekoala commented Aug 11, 2020

yes, although my code with steps to reproduce is very bad, I believe it's quite obvious from the code that throwing exception that are not catched or triggering errors basically do the same thing : the spreadsheet will not be generated

@stale
Copy link

stale bot commented Oct 12, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added the stale label Oct 12, 2020
@lekoala
Copy link
Author

lekoala commented Oct 12, 2020

This deserves some attention so I'll send a message again so that stalebot doesn't close this :)

@stale stale bot removed the stale label Oct 12, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added the stale label Dec 25, 2020
@Aerendir
Copy link

Aerendir commented Aug 30, 2022

Did you find a solution to set the property?

@oleibman
Copy link
Collaborator

oleibman commented Sep 7, 2022

When I used a different formula =SOMEERORRORFUNCTION(, omitting the ending parenthesis, I do get an Exception. When I suppress the throw/trigger, the spreadsheet is generated. However, Excel complains when it tries to open it ("We found a problem with some content"); I am not convinced that this is a better result than not generating the spreadsheet in the first place, especially considering the information that the Exception message contains:

PhpOffice\PhpSpreadsheet\Calculation\Exception: Worksheet!A1 -> Formula Error: Expecting ')' ...

That message identifies the worksheet, the cell, and the problem; this certainly gives the developer a good start in correcting the problem. Excel's message, on the other hand, identifies absolutely nothing - good luck fixing the problem now. Another reason I am not sure continuing to generate is a worthwhile action is that the (corrupt) file could be generated in most circumstances anyhow using:

$writer->setPreCalculateFormulas(false);

At any rate, now that we have a reproducible problem, I can entertain your thoughts on why having the option to continue to generate the spreadsheet is worthwhile.

@stale stale bot removed the stale label Sep 7, 2022
@lekoala
Copy link
Author

lekoala commented Sep 10, 2022

@oleibman yes i think the exception is fine in itself (i don't remember it was giving the cell reference which is great!). What I find odd is the suppressFormulaErrors property that is basically inaccessible for all practical purposes and that doesn't really "suppress" anything since the excel file is still invalid.
I would expect that:

  • It's configurable somewhere from the main spreadsheet object (or at the very least as a static property)
  • That the cell containing the wrong formula has an empty result (and why not a comment with a detailed error message) so that you would still get a valid excel file at the end of the process. In some cases it's not worth it to prevent the entire file generation for one single error which can happen for dynamically generated documents

or if the goal is not to suppress the error, but trigger an error instead of an exception, maybe the property should be called something else like like errorMode : silent, exception, warning, a bit like what PDO is doing.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Sep 24, 2022
Fix PHPOffice#1531. Calculation has a property `suppressFormulaErrors`, which really doesn't work as one might expect. If a calculation throws an exception, the setting of this property might prevent the Exception from being thrown, but it will still trigger an Error. I do not think this makes sense, and will change it so the calculation will return `false`, which is part of the original design but which would essentially never happen. This allows the user to save a corrupt spreadsheet, but this was already possible through the use of `setPreCalculateFormulas(false)` on the Writer, so this doesn't really open any new exposures. It nevertheless might be considered a breaking change because of the difference in behavior.

Another break - the visibility of the property is changed from public to private, and a public setter/getter is added.

Although I am enabling this ability, I don't necessarily think it's a good idea to make use of it. See the original issue for a discussion of why. It is not mentioned in the official documentation, and I will not be adding documentation for it. The originator discovered it by reading the code, and I think that is sufficient for what will often be an ill-advised choice.

Many of the large number (unfortunately not quite all) of problems with Calculation.php in phpstan baseline are addressed. Of note are that private arrays `phpSpreadsheetFunctions`, `controlFunctions`, `comparisonOperators`, and `operatorPrecedence` are changed from static to const.
@oleibman
Copy link
Collaborator

See PR #3081. I have met you at least part way. It is now configurable, and allows continuation when an exception is thrown in Calculation. It will not attempt to do anything about the cell in error; the result will be false, but any attempt by PhpSpreadsheet to alter the user's data (e.g. your suggestion of adding a comment) is way out of scope. Therefore Excel will still pop up "we found a problem" when the file is opened.

@lekoala
Copy link
Author

lekoala commented Sep 26, 2022

@oleibman this is looking great

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Sep 28, 2022
Fix PHPOffice#1531. This is a replacement for PR PHPOffice#3081 (see last paragraph below), which I will close.

Calculation has a property `suppressFormulaErrors`, which really doesn't work as one might expect. If a calculation throws an exception, the setting of this property might prevent the Exception from being thrown, but it will still trigger an Error. I do not think this makes sense, and will change it so the calculation will return `false`, which is part of the original design but which would essentially never happen. This allows the user to save a corrupt spreadsheet, but this was already possible through the use of `setPreCalculateFormulas(false)` on the Writer, so this doesn't really open any new exposures. It nevertheless might be considered a breaking change because of the difference in behavior.

Deprecation - the visibility of the existing property is public, which means it can be changed directly. A new private property is added with a public setter/getter. The new property will be used when the existing property is null (default), which will allow the existing property to be deprecated.

Function getFunctions is changed to static - the array which it returns is static. Existing callers using it as non-static will still function correctly.

Although I am enabling this ability, I don't necessarily think it's a good idea to make use of it. See the original issue for a discussion of why. It is not mentioned in the official documentation, and I will not be adding documentation for it. The originator discovered it by reading the code, and I think that is sufficient for what will often be an ill-advised choice.

Many of the large number of problems with Calculation.php in phpstan baseline are addressed. PR 3081 ran afoul of something in phpstan. The changes in this ticket are more limited, adding a number of doc blocks but leaving executable code unchanged.
@oleibman
Copy link
Collaborator

Had to replace 3082 with 3091 for some reason. Essentially the same change.

oleibman added a commit that referenced this issue Oct 1, 2022
Fix #1531. This is a replacement for PR #3081 (see last paragraph below), which I will close.

Calculation has a property `suppressFormulaErrors`, which really doesn't work as one might expect. If a calculation throws an exception, the setting of this property might prevent the Exception from being thrown, but it will still trigger an Error. I do not think this makes sense, and will change it so the calculation will return `false`, which is part of the original design but which would essentially never happen. This allows the user to save a corrupt spreadsheet, but this was already possible through the use of `setPreCalculateFormulas(false)` on the Writer, so this doesn't really open any new exposures. It nevertheless might be considered a breaking change because of the difference in behavior.

Deprecation - the visibility of the existing property is public, which means it can be changed directly. A new private property is added with a public setter/getter. The new property will be used when the existing property is null (default), which will allow the existing property to be deprecated.

Function getFunctions is changed to static - the array which it returns is static. Existing callers using it as non-static will still function correctly.

Although I am enabling this ability, I don't necessarily think it's a good idea to make use of it. See the original issue for a discussion of why. It is not mentioned in the official documentation, and I will not be adding documentation for it. The originator discovered it by reading the code, and I think that is sufficient for what will often be an ill-advised choice.

Many of the large number of problems with Calculation.php in phpstan baseline are addressed. PR 3081 ran afoul of something in phpstan. The changes in this ticket are more limited, adding a number of doc blocks but leaving executable code unchanged.
MarkBaker added a commit that referenced this issue Dec 21, 2022
### Added

- Extended flag options for the Reader `load()` and Writer `save()` methods
- Apply Row/Column limits (1048576 and XFD) in ReferenceHelper [PR #3213](#3213)
- Allow the creation of In-Memory Drawings from a string of binary image data, or from a stream. [PR #3157](#3157)
- Xlsx Reader support for Pivot Tables [PR #2829](#2829)
- Permit Date/Time Entered on Spreadsheet to be calculated as Float [Issue #1416](#1416) [PR #3121](#3121)

### Changed

- Nothing

### Deprecated

- Direct update of Calculation::suppressFormulaErrors is replaced with setter.
- Font public static variable defaultColumnWidths replaced with constant DEFAULT_COLUMN_WIDTHS.
- ExcelError public static variable errorCodes replaced with constant ERROR_CODES.
- NumberFormat constant FORMAT_DATE_YYYYMMDD2 replaced with existing identical FORMAT_DATE_YYYYMMDD.

### Removed

- Nothing

### Fixed

- Fixed handling for `_xlws` prefixed functions from Office365 [Issue #3245](#3245) [PR #3247](#3247)
- Conditionals formatting rules applied to rows/columns are removed [Issue #3184](#3184) [PR #3213](#3213)
- Treat strings containing currency or accounting values as floats in Calculation Engine operations [Issue #3165](#3165) [PR #3189](#3189)
- Treat strings containing percentage values as floats in Calculation Engine operations [Issue #3155](#3155) [PR #3156](#3156) and [PR #3164](#3164)
- Xlsx Reader Accept Palette of Fewer than 64 Colors [Issue #3093](#3093) [PR #3096](#3096)
- Use Locale-Independent Float Conversion for Xlsx Writer Custom Property [Issue #3095](#3095) [PR #3099](#3099)
- Allow setting AutoFilter range on a single cell or row [Issue #3102](#3102) [PR #3111](#3111)
- Xlsx Reader External Data Validations Flag Missing [Issue #2677](#2677) [PR #3078](#3078)
- Reduces extra memory usage on `__destruct()` calls [PR #3092](#3092)
- Additional properties for Trendlines [Issue #3011](#3011) [PR #3028](#3028)
- Calculation suppressFormulaErrors fix [Issue #1531](#1531) [PR #3092](#3092)
- Permit Date/Time Entered on Spreadsheet to be Calculated as Float [Issue #1416](#1416) [PR #3121](#3121)
- Incorrect Handling of Data Validation Formula Containing Ampersand [Issue #3145](#3145) [PR #3146](#3146)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3136](#3137)
- Generation3 Copy With Image in Footer [Issue #3126](#3126) [PR #3140](#3140)
- MATCH Function Problems with Int/Float Compare and Wildcards [Issue #3141](#3141) [PR #3142](#3142)
- Fix ODS Read Filter on number-columns-repeated cell [Issue #3148](#3148) [PR #3149](#3149)
- Problems Formatting Very Small and Very Large Numbers [Issue #3128](#3128) [PR #3152](#3152)
- XlsxWrite preserve line styles for y-axis, not just x-axis [PR #3163](#3163)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3137](#3137)
- More Detail for Cyclic Error Messages [Issue #3169](#3169) [PR #3170](#3170)
- Improved Documentation for Deprecations - many PRs [Issue #3162](#3162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment