Handle REF Error as Part of Range #3467
Merged
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#3453. User sets a valid formula (e.g. `=SUM(Sheet2!B1:Sheet2!B3)`), and then does something to invalidate the formula (e.g. delete Sheet2). Excel changes the formula to `SUM(#REF!:#REF!)` when the spreadsheet is saved; apparently someone thought this was a good idea. But PhpSpreadsheet (a) used to throw an Exception when it evaluated the formula, and (b) now gives a result of `0` when evaluating the formula. Neither is ideal. It would be better to propagate the `#REF!` error. It is likely that more tests are needed, which is why I will keep this in draft status for a bit.
8 tasks
|
Nice work! This seems to correctly propagate the ref error back to the caller. I wonder if this counts as a BC break since it is different behavior to both pre-1.26.0 and post-1.26.0? What sort of other tests do you think are necessary? Your current tests seem to cover everything way of getting a |
|
I don't think any other Excel errors need to be tested. The only reason this change is needed is because Excel, for whatever reason, changes the formulas. I am not aware of any cases where, for example, it replaces the function argument with I don't really consider this a BC break. It was returning the wrong result before; it seems like any other bug fix in that respect. |
oleibman
changed the title
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Jun 30, 2024
Fix PHPOffice#2581 (not obvious - see next paragraph for explanation). This continues the work of PR PHPOffice#2902 (and also PR PHPOffice#3467) to have errors propagated through function calculations rather than treating them as strings. All text functions, and the concatenation operator, are addressed in this PR. In the original issue, the spreadsheet being loaded uses the result of an unimplemented function as an argument to another function. When `getCalculatedValue` is used on the cell in question, the result is returned as `#VALUE!`. If the cell had just contained a function call to the unimplemented function, getCalculatedValue would have recognized the situation and returned oldCalculatedValue as the result. Not perfect, but good enough most of the time. User would like oldCalculatedValue returned here as well, which seems like a reasonable request. PhpSpreadsheet always returns `#Not Yet Implemented` as the result for a function which it knows about but which is not yet implemented. That is the key to the `Cell` class being able to substitute oldCalculatedValue in the first place. However, in order to do that for the issue in question, that result has to be propagated to any functions for which the result is an argument. I don't want to add unimplemented to the list of known error codes, but I am willing to add a parameter to `ErrorValue::isError` to indicate whether that value should be considered an error (default is "no"). The first use of that new parameter would be by the text functions. They go through a common Helper routine, so it is pretty easily implemented. And, as it turns out, most of the text functions do not currently propagate errors, e.g. if A1 results in a value error, `=LEFT(A1,2)` will result in `#V` rather than `#VALUE!`. With this PR, they will now be handled correctly.
11 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 #3453. User sets a valid formula (e.g.
=SUM(Sheet2!B1:Sheet2!B3)), and then does something to invalidate the formula (e.g. delete Sheet2). Excel changes the formula toSUM(#REF!:#REF!)when the spreadsheet is saved; apparently someone thought this was a good idea. But PhpSpreadsheet (a) used to throw an Exception when it evaluated the formula, and (b) now gives a result of0when evaluating the formula. Neither is ideal. It would be better to propagate the#REF!error.It is likely that more tests are needed, which is why I will keep this in draft status for a bit.
This is:
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.