* Additional Unit Tests for XIRR and XNPV
See discussion in issue #3297. Excel result disagreed with PhpSpreadsheet, but it seems apparent that the error is Excel's. Although we have other test cases where Excel and PhpSpreadsheet disagree, those have so far been cases where Excel cannot give an answer because its algorithm does not converge, which is possible for any algorithm. In the issue in question, Excel seems to converge to a different answer; this is different than the other exceptions, and so deserves documentation with a formal test case. The discussion also mentions some cases where XNPV seems to give the wrong result in Excel, and those cases are added for XNPV.
* Scrutinizer
Whatever.
* Minor Fix for AND/OR/XOR
These 3 fall into the set of functions where Excel treats string literals differently depending on whether they are passed to the function directly or as a cell reference. PhpSpreadsheet is updated to try to duplicate that logic. New tests are added. Some existing test results had to change as a result of this code change.
* Adopt A Suggestion From Mark Baker
Reduce if statements by adding functions.
* Eliminate Shared\JAMA
The code under `Shared\JAMA` is called from exactly 3 places in Calculation, and exactly 1 other place (see next paragraph). These places are inadequately covered in the test suite, so it is not clear that the existing code works. In addition, see PR #2964 for commentary decrying the continued used of JAMA. I will also note that it has a pitiful 8.20% coverage in the test suite. There seems to already be a perfectly adequate equivalent in Calculation for those parts of JAMA which are used (e.g. we don't use any decompositions, so the fact that no decomposition code is present in Calculation is not a problem). This PR replaces the uses of JAMA within Calculation with `checkMatrixOperands`, and deletes Shared/JAMA entirely (which, among other thing, makes many Phpstan reported problems go away). The test suite is enhanced to ensure coverage of all the changed statements. A side benefit of deleting Shared/Jama is that the only function which PhpSpreadsheet has added to the global namespace (hypo in Maths.php) goes away.
There is one additional use in Shared/Trend/PolynomialBestFit. That use is best replaced with Matrix/Matrix, which is already a requirement for PhpSpreadsheet. PolynomialBestFit has zero test coverage, and I didn't add any for this change. There seem to be existing errors which both Phpstan and Scrutinizer complain about, and I am quite convinced that they are not false positives. Fixing those problems will be a project for another day.
Calculation has many calls to `Information\ErrorValue` and `Information\ExcelError`. It is inconsistent in how it does so - most invoke `Information\E...` but a small number take advantage of additional `use` statements to just invoke `E...`. I have made the usage consistent by changing the small number to act like the majority and eliminating the `use` statements.
Some of the test cases exposed the fact that MAX, MAXA, MIN, and MINA do not properly handle error strings in their input. For example, if cell A1 contains 2, and A2 contains `=5/0`, `=MAX(A1, A2)` should return `#DIV/0!`. They are changed to handle them properly.
`Shared\JAMA` was normally omitted from code coverage. Since it is being deleted, there is no longer a reason to explicitly exclude it. `Writer\PDF` was also on the exclude list, probably for historical reasons, but there is no reason to exclude it now (it is, in fact, 100% covered), so it will no longer be excluded.
* Scrutinizer
Eliminate some newly dead code.
Fix#689. XIRR is calculated by making guesses which are hopefully better with each iteration. It is not guaranteed to succeed for Excel, PhpSpreadsheet, or any other implementation. PhpSpreadsheet uses the Newton-Raphson method for its guesses. So does Python package xirr (https://github.com/tarioch/xirr/), but, if Newton-Raphson fails to converge, Python tries Brent's method as an alternative. Two sets of non-converging data are noted in 689. For both, a solution does converge in Excel. For the first of the problems, a solution converges in Python with Newton-Raphson; but, for the second, a solution converges which requires Brent. For the Java package https://github.com/RayDeCampo/java-xirr on which Python was based, and which uses only Newton-Raphson, a solution converges for the first, and does not converge for the second.
To try to match the good results of the others, I added an alternate algorithm if Newton-Raphson fails. Brent's algorithm seems difficult to implement to me. I might have gone there regardless, but I first tried a slightly simpler alternative, bisection. This solved the problem for both of the cases in 689. Perhaps someone will one day report a problem that doesn't converge for Newton-Raphson or bisection, but does for Brent. We can review this decision then.
The new code causes 3 changes in the unit test. In all 3 tests, Excel and PhpSpreadsheet had not converged, but Python and/or Java had. I now believe that Python/Java is correct in those cases, and Excel is not. The new code aligns PhpSpreadsheet with Python/Java for those tests. It is, of course, impossible to know when Excel's implementation doesn't converge, so we aren't guaranteed to match its results in those hopefully rare situations.
* Change Additional Statistical Tests to Use Spreadsheet Context
With an earlier change, I made all but 18 Statistical tests run in spreadsheet context. This PR changes 12 of those 18. The remaining 6 usually return array results, so it is a tougher task to handle them. I will continue to think on it.
AVERAGEIF, AVERAGEIFS, and COUNTBLANK are changed to throw an Exception when a range is specified as a literal. They previously accepted array (enclosed in braces) literals, and bumbled along till they threw an error for non-array literals. Throwing an exception appears to be analogous to how Excel operates, rather than something more friendly like a VALUE error. There may be other functions which require similar treatment.
There also remains a TODO for COUNTIFS, and possibly other functions. It appears that PhpSpreadsheet counts booleans for both integer and string compares and probably shouldn't. Again, this is a problem for another day.
* Scrutinizer
Fix one problem.
* Scrutinizer Ignores Its Own Suggested Remedy
Try another approach.
* Fix Unintential Deprecated Calls in Tests - STATISTICAL
I think it's best to install these before PR #3166. There are no changes to source code, only to doc-blocks and to test members which continue to inadvertently use calls to deprecated functions.
* Missed One Deprecation
Fix it now.
* Run Tests in Spreadsheet Context
This is quite a bit more difficult for Statistical than for the other Calculation categories. This is partly because of the use of multi-dimensional matrices, and also because some arguments are interpreted differently when they come from a cell rather than entered directly in a formula. This push leaves 18 out of 89 test members unchanged, except that they are marked with a TODO to show that the work isn't finished. I will not revisit them as part of this PR, but probably will take a look in a subsequent ticket.
* Fix Unintential Deprecated Calls in Tests - FINANCIAL
I think it's best to install these before PR #3166. There are no changes to source code, only to doc-blocks and to test members which continue to inadvertently use calls to deprecated functions.
* Change Tests to Run in Spreadsheet Context
Found and fixed some problems with how MIRR handles errors.
* Fix Unintential Deprecated Calls in Tests - ENGINEERING
I think it's best to install these before PR #3166. There are no changes to source code, only to test members which continue to inadvertently use calls to deprecated functions.
* Fix deprecation DocBlocks
Deprecated->deprecated, adjust see and comments
* Fix Deliberate Deprecated Tests
Add annotations.
* Run Unit Tests in Spreadsheet Context
This turned up only one error, in IMSUB. The only group that still needs this is Statistical. Not sure if I will get to that quickly.
* Fix Unintential Deprecated Calls in Tests - LOGICAL
I think it's best to install these before PR #3166. There are no changes to source code, only to doc-blocks and to test members which continue to inadvertently use calls to deprecated functions.
* Change Unit Tests to Run in Spreadsheet Context
They had been run as direct calls, which is not how most users would use them. Making this change exposed some minor coding errors - SWITCH needs to flatten its arguments, and IFERROR and IFNA were not handling a null testValue in the same manner as Excel.
* Fix Unintential Deprecated Calls in Tests - DATABASE
I think it's best to install these before PR #3166. There are no changes to source code, only to test members which continue to inadvertently use calls to deprecated functions.
* Fix deprecation Blocks
Deprecated->deprecated, adjust see and comments
* Fix Deliberate Deprecated Tests
Add annotations.
* Change Unit Tests to Run in Spreadsheet Context
... rather than as direct calls. The major difference is that specifying an invalid column should result in an Excel error, not null. Minor code changes were needed, including to Statistical/Conditional which sometimes calls Database.
* Correct Some DocBlocks
Null is no longer a possible output for most of these functions.
* 2 Overlooked Tests
Change to run in spreadsheet context.
* T Function Should Return Null-String, not Null
Fix it.
Allow default values for TEXTJOIN() delimiter and ignore_empty... even though the official MS documentation lists them as required arguments, they are actually optional
* MATCH Problems with Int/Float Compare and Wildcards
Fix#3141. Function matchSmallestValue did not recognize that an integer could match a float. Adding test cases, it seems that matchFirstValue had the same problem. However, matchLargestValue seemed to handle things correctly - but see below.
In addition, the wildcard logic in matchFirstValue is faulty. It ignored tilde as a wildcard character. Although it would have been easy to just add that, I think it was wrong to determine on its own if a wildcard was in use. Just using the already available wildcard functions whenever comparing two strings is sufficient.
I note that Excel doesn't seem to follow its own rules for MATCH (https://support.microsoft.com/en-us/office/match-function-e8dffd45-c762-47d6-bf89-533f4a37673a?ns=excel&version=90&syslcid=1033&uilcid=1033&appver=zxl900&helpid=xlmain11.chm60112&ui=en-us&rs=en-us&ad=us). PhpSpreadsheet's results match Excel's, so no problem. However, when match_type is not zero, the match array is supposed to be sorted, so I would expect `#N/A` when it isn't; but that's not how Excel operates. I have no idea what Excel is doing. If `MATCH(2,{2,0,4,3},1)` isn't `#N/A` because of the unsorted array, then surely it should be `1` (item 1 of the array is the largest number less than or equal to the lookup value); but Excel and PhpSpreadsheet (before and after changes) return `2`. I have moved this example to be the first of the test cases.
One would think strings would behave similarly. But, no - see the second test case. This time Excel does look for an exact match. But the existing logic doesn't get the matching result in PhpSpreadsheet. It requires a whole new block of code, one which doesn't work correctly for numeric lookup value. Ugh.
LibreOffice doesn't always agree with Excel. It seems that it will use wildcard matching even when the match type is not zero (Excel documentation says wildcards are only for type zero, which is just as well because I don't really know what greater/less mean when wildcards are involved). I have not attempted to duplicate this behavior. For the record, Gnumeric agrees with Excel here.
* More Changes - LibreOffice
Add support for LibreOffice matching wildcard strings when type is not zero. Add support for type to be specified as integer other than 0/1/-1, or as float, or as numeric string; non-numeric string should case `#VALUE!` error.
I have found an example of undefined behavior (unsorted array where type is non-zero) where PhpSpreadsheet does not produce the same result as Excel. It is present as a new `incomplete` test case. I can fix it, but not without breaking other tests where the proper behavior is undefined. IMO, this is not a problem we should be concerned about.
Many test cases are added. Chances are I will add some more before merging this change.
Scrutinizer had previously suggested annotations for 3 LookupRef tests, but it no longer accepts its own annotation for those cases. This PR cleans them up. ColumnsTest and RowsTest are extremely straightforward.
IndexTest is a bit more complicated, but only because, unlike the other two, it had no test which executed in the context of a spreadsheet. And, when I added those, I discovered a couple of bugs. INDEX always requires at least 2 parameters (row# is always required), but its entry in the function table specified 1-4 parameters, now changed to 2-4. And, omitting col# is not handled the same way as specifying 0 for col#, though the code had treated them identically. (The same would have been true for row# but, because it is now required, ...)
* R1C1 Format and Internationalization, plus Relative Offsets
Fix#1704, albeit imperfectly. Excel's implementation of this feature makes it impossible to fix perfectly. I don't know why it was necessary to internationalize R1C1 in the first place - the benefits are so minimal,and the result is worksheets that break when opened in different locales. Ugh. I can't even find complete documentation about the format in different languages; I am using https://answers.microsoft.com/en-us/officeinsider/forum/all/indirect-function-is-broken-at-least-for-excel-in/1fcbcf20-a103-4172-abf1-2c0dfe848e60 as my definitive reference.
This fix concentrates on the original report, using the INDIRECT function; there may be other areas similarly affected. As with ambiguous date formats, PhpSpreadsheet will do a little better than Excel itself when reading spreadsheets with internationalized R1C1 by trying all possibilities before giving up. When it does give up, it will now return `#REF!`, as Excel does, rather than throwing an exception, which is certainly friendlier. Although read now works better, when writing it will use whatever the user specified, so spreadsheets breaking in the wrong locale will still happen.
There were some bugs that turned up as I added test cases, all of them concerning relative addressing in R1C1 format, e.g. `R[+1]C[-1]`. The regexp for validating the format allowed for minus signs, but not plus signs. Also, the relevant functions did not allow for passing the current cell address, which made relative addressing impossible. The code now allows these, and suitable test cases are added.
* Use Locale for Formats, but Not for XML
Implementing a suggestion from @MarkBaker to use the system locale for determining R1C1 format rather than looping through a set of regexes and accepting any that work. This is closer to how Excel itself operates. The assumption we are making is to use the first character of the translated ROW and COLUMN functions. This will not work for Russian or Bulgarian, where each starts with the same letter, but it appears that Russian, at least, still uses R1C1. So our algorithm will not use non-ASCII characters, nor characters where ROW and COLUMN start with the same letter, falling back to R/C in those cases. Turkish falls into that category. Czech uses an accented character for one of the functions, and I'm guessing to use the unaccented character in that case. Polish COLUMN function is NR.KOLUMNY, and I'm guessing to use K in that case.
The function that converts R1C1 references is also used by the XML reader *where the format is always R1C1*, not locale-based (confirmed by successfully opening in Excel an XML spreadsheet when my language is set to French). The conversion code now handles that distinction through the use of an extra parameter. Xml Reader Load Test is duplicated to confirm that spreadsheet is loaded properly whether the locale is English or French. (No, I did not add an INDIRECT function to the Xml spreadsheet.)
Tests CsvIssue2232Test and TranslationTest both changed locale without resetting it when done. That omission was exposed by the new code, and both are now corrected.
* OpenOffice and Gnumeric
OpenOffice and Gnumeric make it much easier to test with other languages - they can be handled with an environment variable. Sensibly, they require R and C as the characters for R1C1 notation regardless of the language. Change code to recognize this difference from Excel.
* Handle Output of ADDRESS Function
One other function has to deal with R1C1 format as a string. Unlike INDIRECT, which receives the string on input, ADDRESS generates the string on output. Ensure that the ADDRESS output is consistent with the INDIRECT input.
ADDRESS expects its 4th arg to be bool, but it can also accept int, and many examples on the net supply it as an int. This had not been handled properly, but is now corrected.
* More Structured Test
I earlier introduced a new test for relative R1C1 addressing. Rewrite it to be clearer.
* Add Row for This to Locale Spreadsheet
It took a while for me to figure out how it all works. I have added a new row (with English value `*RC`) to Translations.xlsx, in the "Lookup and Reference" section of sheet "Excel Functions". By starting the "function name" with an asterisk, it will not be confused with a "real" function (confirmed by a new test). This approach also gives us the flexibility to do something similar if another surprise case occurs in future; in particular, I think this is more flexible than adding this as another option on the "Excel Localisation" sheet. It also means that any errors or omissions in the list below will be handled as with any other translation problem, by updating the spreadsheet without needing to touch any code.
The spreadsheet has the following entries in the *RC row:
- first letter of ROW/COLUMN functions for da, de, es, fi, fr, hu, nl, nb, pt, pt_br, sv
- no value for locales where ROW/COLUMN functions start with same letter - bg, ru, tr
- no value for locales with a multi-part name for ROW and/or COLUMN - it, pl (I had not previously noted Italian as an exception)
- no value for locales where ROW and/or COLUMN starts with a non-ASCII character - cs (this would also apply to bg and ru which are already included under "same letter")
- it does nothing for locales which are defined on the "Excel Localisation" sheet but have no entries yet on the "Excel Functions" sheet (e.g. eu)
Note that all but the first bullet item will continue to use R/C, which leaves them no worse off than they were before this change.
Fix#2934. Null is passed to StringHelper::strtolower which expects string. Same problem appears to be applicable to HLOOKUP.
I noted the following problem in the code, but will document it here as well. Excel's results are not consistent when a non-numeric string is passed as the third parameter. For example, if cell Z1 contains `xyz`, Excel will return a REF error for function `VLOOKUP(whatever,whatever,Z1)`, but it returns a VALUE error for function `VLOOKUP(whatever,whatever,"xyz")`. I don't think PhpSpreadsheet can match both behaviors. For now, it will return VALUE for both, with similar results for other errors.
While studying the returned errors, I realized there is something that needs to be deprecated. `ExcelError::$errorCodes` is a public static array. This means that a user can change its value, which should not be allowed. It is replaced by a constant. Since the original is public, I think it needs to stay, but with a deprecation notice; users can reference and change it, but it will be unused in the rest of the code. I suppose this might be considered a break in functionality (that should not have been allowed in the first place).
* Keep Calculated String Results Below 32K
This is the result of an investigation into issue #2884 (see also PR #2913). It is, unfortunately, not a fix for the original problem; see the discussion in that PR for why I don't think there is a practical fix for that specific problem at this time.
Excel limits strings to 32,767 characters. We already truncate strings to that length when added to the spreadsheet. However, we have been able to exceed that length as a result of the concatenation operator (Excel truncates); as a result of the CONCATENATE or TEXTJOIN functions (Excel returns #CALC!); or as a result of the REPLACE, REPT, SUBSTITUTE functions (Excel returns #VALUE!). This PR changes PhpSpreadsheet to return the same value as Excel in these cases. Note that Excel2003 truncates in all those cases; I don't think there is a way to differentiate that behavior in PhpSpreadsheet.
However, LibreOffice and Gnumeric do not have that limit; if they have a limit at all, it is much higher. It would be fairly easy to use existing settings to differentiate between Excel and LibreOffice/Gnumeric in this respect. I have not done so in this PR because I am not sure how useful that is, and I can easily see it leading to problems (read in a LibreOffice spreadsheet with a 33K cell and then output to an Excel spreadsheet). Perhaps it should be handled with an additional opt-in setting.
I changed the maximum size from a literal to a constant in the one place where it was already being enforced (Cell/DataType). I am not sure that is the best place for it to be defined; I am open to suggestions.
* Implement Some Suggestions
... from @MarkBaker.
These changes have already been implemented twice, and been regressed twice. I'll try once more (with a different approach), then give up ...
As configured, Phpstan running under Php7 reports no errors. However, running under Php8, it reports 100 (!) errors. The vast majority of these are due to two reasons:
- renaming parameters in Php builtin functions in preparation for named parameters.
- using the new class GdImage rather than type resource as the argument type for many image-based functions.
Regardless of the cause, this will be a problem sooner or later. This PR is an attempt to get ahead of that problem. For source members, it mostly adds annotations or updates doc-blocks. Only 2 members have changes to executable code, and these are very minor - BitWise and Writer/Xlsx. For test members, all baseline errors are deleted and the code is fixed. Php7 and Php8 both report no errors with this configuration.
Note that this method is used when translating Excel functions between en and other locale languages, as well as when converting formulae between different spreadsheet formats (e.g. Ods to Excel)
Nor is this a perfect solution, as there may still be issues when function calls have array arguments that themselves contain function calls; but it's still better than the current logic
Implement Array-enabled for ERROR.TYPE() function
Extract ERROR.TYPE() function tests into separate test file
Extract error function tests into separate test files
And thus complete the implemented Information functions
* First steps toward array-enabling the information functions
Also includes moving unit tests out from Functions and into a separate, dedicated Information folder
* Resolve issue with IF(), branch pruning and calculation cache (ensure that we don't convert the if condition to a bool before we've tested to see if it evaluates to an error)
More refactoring
* Start work on array-enabling the Lookup and Reference functions
Requires a new method (`evaluateArrayArgumentsSubsetFrom()`) in the `ArrayEnabled` Trait to handle functions where the arguments that need special array handling are trailing rather than leading arguments
* Split Information functions into a dedicated class and namespace and categorise as Value or Error
* Refactor all error functions into the new ExcelError class
* Add unit test for erroneous translations from Russian to English, and a quick/dirty fix
* Additional translation unit tests with accented characters from Spanish, Bulgarian, Czech and Turkish
* Update Change Log
* Implementation of the SEQUENCE() Excel365 function
Note that the Calculation Engine does not yet support the Spill operator, or spilling functions
* Handle the use-case of step = 0; and tests for exception handling for invalid arguments
* Update Change Log
* Refinement for XIRR
Fix#2469. The algorithm used for XIRR is known not to converge in some cases, some of which are because the value is legitimately unsolvable; for others, using a different guess might help.
The algorithm uses continual guesses at a rate to hopefully converge on the solution. The code in Python package xirr (https://github.com/tarioch/xirr/) suggests a refinement when this rate falls below -1. Adopting this refinement solves the problem for the data in issue 2469 without any adverse effect on the existing tests. My thanks to @tarioch for that refinement.
The data from 2469 is, of course, added to the test cases. The user also mentions that an initial guess equal to the actual result doesn't converge either. A test is also added to confirm that that case now works.
The test cases are changed to run in the context of a spreadsheet rather than by direct calls to XIRR calculation routine. This revealed some data validation errors which are also cleaned up with this PR. This suggests that other financial tests might benefit from the same change; I will look into that.
* More Unit Tests
From https://github.com/RayDeCampo/java-xirr/blob/master/src/test/java/org/decampo/xirr/XirrTest.javahttps://github.com/tarioch/xirr/blob/master/tests/test_math.py
Note that there are some cases where the PHP tests do not converge, but the non-PHP tests do. I have confirmed in each of those cases that Excel does not converge, so the PhpSpreadsheet results are good, at least for now. The discrepancies are noted in comments in the test member.
Replace mock tests with real ones when possible. The original tests are all still present; they just take place in a more representative scenario.
After this, there will be 4 remaining uses of mocking. Of these, 3 are needed for scenarios which are otherwise hard to test - WebServiceTest, CellsTest, and SampleCoverageTest. For the other one, AutoFilterTest, I just can't figure out what it's trying to accomplish, so have left it alone.
This change is almost entirely restricted to tests. There is a one-line change in src. When the first argument passed to OFFSET is null or nullstring, the returned value is currently 0. However, according to the documentation for Excel, it should be `#VALUE!`. The code is changed accordingly.
* Handle a wildcard match that contains a forward slash in the pattern by adding / to the delimiter list of preg_quote
* Fix SUMIF doing a wildcard match on empty cells (NULL)
* Fix compare logic to return false when value is an empty string or NULL (Verified against LibreOffice SUMIF and MATCH handling of empty cells)
See issue #2123. HLOOKUP needs to do some conversions between column numbers and letters which it had not been doing.
HLOOKUP tests were performed using direct calls to the function in question rather than in the context of a spreadsheet. This contributed to keeping this error obscured even though there were, in theory, sufficient test cases. The tests are changed to perform in spreadsheet context. For the most part, the test cases are unchanged. One of the expected results was wrong; it has been changed, and a new case added to cover the case it was supposed to be testing.
After getting the HLOOKUP tests in order, it turned out that a test using literal arrays which had been succeeding now failed. The array constructed by the literals are considerably different than those constructed using spreadsheet cells; additional code was added to handle this situation.
oleibman
Mark Baker
MarkBaker
lucasnetau
oleibman