Fix: SUBTOTAL to handle #REF! errors #2870
Conversation
|
I am not duplicating your result in PhpSpreadsheet 1.23; in particular, I do not see a warning message. Please tell Php and PhpSpreadsheet release where you see a warning message (and the contents of the message). There is at least one problem with your test case, so let's start with that. We currently cannot always distinguish the error situation |
More detailsThanks for the comment @oleibman. I can confirm that I'm using PhpSpreadsheet 1.23.0 and PHP 8.1.6.
New testsI've added in 2 more tests as you requested, both of which fail with the But that leads to a RE: Unable to reproduce the issue:I'm not sure why you can't reproduce it, but commenting out my changes to And you can see the PHP 7.4 build is now failing with |
|
Small changes are required in Calculation and SubTotal to get everything to work properly. Ironically, once these changes are made, your change to Subtotal is no longer necessary, as your tests will prove. For now, I am placing your change in draft status. If you are willing to expand your scope to include the change in Calculation (plus a few extra test cases that such a change will require), I can discuss that with you. Otherwise, I will open a PR to make the change. Let me know how you wish to proceed. |
|
Ok no problem. I'm not entirely sure which changes are required. If you're able to do up a PR im happy to test it 🙂 |
This PR derives from, and supersedes, PR PHPOffice#2870, submitted by @ndench. The problem reported in the original is that SUBTOTAL does not handle #REF! errors in its arguments properly; however, my investigation has enlarged the scope. The main problem is in Calculation, and it has a simple fix. When the calculation engine finds a reference to an uninitialized cell, it uses `null` as the value. This is appropriate when the cell belongs to a defined sheet; however, for an undefined sheet, #REF! is more appropriate. With that fix in place, SUBTOTAL still needs a small fix of its own. It tries to parse its cell reference arguments into an array, but, if the reference does not match the expected format (as #REF! will not), this results in referencing undefined array indexes, with attendant messages. That assignment is changed to be more flexible, eliminating the problem and the messages. Those 2 fixes are sufficient to ensure that the original problem is resolved. It also resolves a similar problem with some other functions (e.g. SUM). However, it does not resolve it for all functions. Or, to be more particular, many functions will return #VALUE! rather than #REF! if this arises, and the same is true for other errors in the function arguments, e.g. #DIV/0!. This PR does not attempt to address all functions; I need to think of a systematic way to pursue that. However, at least for most MathTrig functions, which validate their arguments using a common method, it is relatively easy to get the function to propagate the proper error result.
* Handling of #REF! Errors in Subtotal, and More This PR derives from, and supersedes, PR #2870, submitted by @ndench. The problem reported in the original is that SUBTOTAL does not handle #REF! errors in its arguments properly; however, my investigation has enlarged the scope. The main problem is in Calculation, and it has a simple fix. When the calculation engine finds a reference to an uninitialized cell, it uses `null` as the value. This is appropriate when the cell belongs to a defined sheet; however, for an undefined sheet, #REF! is more appropriate. With that fix in place, SUBTOTAL still needs a small fix of its own. It tries to parse its cell reference arguments into an array, but, if the reference does not match the expected format (as #REF! will not), this results in referencing undefined array indexes, with attendant messages. That assignment is changed to be more flexible, eliminating the problem and the messages. Those 2 fixes are sufficient to ensure that the original problem is resolved. It also resolves a similar problem with some other functions (e.g. SUM). However, it does not resolve it for all functions. Or, to be more particular, many functions will return #VALUE! rather than #REF! if this arises, and the same is true for other errors in the function arguments, e.g. #DIV/0!. This PR does not attempt to address all functions; I need to think of a systematic way to pursue that. However, at least for most MathTrig functions, which validate their arguments using a common method, it is relatively easy to get the function to propagate the proper error result. * Arrange Array The Way call_user_func_array Wants Problem with Php8.0+ - array passed to call_user_func_array must have int keys before string keys, otherwise Php thinks we are passing positional parameters after keyword parameters. 7 other functions use flattenArrayIndexed, but Subtotal is the only one which uses that result to subsequently pass arguments to call_user_func_array. So the others should not require a change. A specific test is added for SUM to validate that conclusion. * Change Needed for Hidden Row Filter Same as change made to Formula Args filter.
This is:
Checklist:
Why this change is needed?
When Excel calculates a
SUBTOTALthat contains#REF!, such as=SUBTOTAL(9, #REF!)it returns#REF!. PhpSpreadsheet causes a PHP warning to be raised due to an array unpacking in Subtotal.The root cause is the call to Functions::flattenArrayIndexed which returns something that looks like
[0 => '#REF!']as opposed to[0.W.29 => '#REF!']. TheSubtotal.phpfunction then does an explode on the key to get the column and row values to check if they are hidden or contain other aggregate functions that theSUBTOTALshould ignore.I'm unsure of the best way to handle this.
FlattenArrayIndexedbe updated?#REF!calculation errors that should also be tracked down?explodeand then unpack the result immediately into more than property. So I think it's unlikely.Subtotal.phphandle other error type values?#VALUE!error. Excel won't let me write other errors into a column to see how it handles them with the SUBTOTAL function.Submitting this PR to provide some failing test cases and my suggested solution (which has the fewest flow on effects possible).
Let me know if there's another way this should be handled.