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

Calculation suppressFormulaErrors - Minor Breaks #3081

Closed
wants to merge 2 commits into from

Conversation

oleibman
Copy link
Collaborator

Fix #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.

This is:

- [x] a bugfix
- [ ] a new feature
- [ ] refactoring
- [ ] additional unit tests

Checklist:

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#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 Author

No interest in resolving Scrutinizer failure. One method needed to add some tests for null in order to satisfy Phpstan; Scrutinizer says these extra checks changed complexity from B to C, which is a failure condition. C'est la vie.

@oleibman
Copy link
Collaborator Author

Phpstan seems to be timing out, for reasons which I do not understand. I may have to cancel this PR and submit another in its place.

@oleibman
Copy link
Collaborator Author

Phpstan failure does not seem to be the result of a botched merge. I will resubmit a less ambitious PR.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request 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 Author

Superseded by 3092. Closing.

@oleibman oleibman closed this Sep 28, 2022
oleibman added a commit that referenced this pull request 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.
@oleibman oleibman deleted the nocalcthrow branch October 20, 2022 14:41
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.

suppressFormulaErrors is not configurable
1 participant