Scrutinizer balked at quotes around key 'Unit Name'; no reason not to use 'UnitName' instead.
Having done that, several ConvertUOM tests seemed a little unconvincing. Replaced them with improved versions.
Fix#3909. SUMPRODUCT is mishandling multi-row ranges. In Calculation/Calculation, `checkMatrixOperands` will often resize its operands. When it does so, it needs to recalculate the dimensions of each. This fixes the reported problem.
Likely cause was PR #3260. That ticket noted the poor coverage of the code being replaced. Tests of the problem in this ticket were absent and are now added. Despite this, I note that `resizeMatricesShrink` is virtually uncovered, and `resizeMatricesExpand` has substantial gaps in its coverage. I have covered some, but not all, of the Expand gaps. I am struggling to come up with examples to fill its remaining gaps and those for Shrink. However, I will merge this fix in about a week even if I don't succeed.
I check from time to time. There are a number of problems now, mostly due to the elimination of Php 7.4 and replacement of doc-block typing with explicit Php typing.
- Bitwise functions were particularly affected by PR #3718 and PR #3793.
- Chart/Axis and Writer/Xlsx were amusingly affected by PR #3836, which added a scaling option which included an array indexed by the known allowable factors, one of which is 1 trillion, which cannot be represented as an integer on a 32-bit system. Issue3833Test, introduced by the same PR (and not suffering any errors) was expanded to test this value.
- Some minor changes to Reader/Xls and Shared/OLE/PPS to accommodate hex values which are negative in 32-bit but which Php-32 may wind up casting to large floating point numbers; it is not clear to me why these hadn't shown up as problems previously. Possibly this is the result of changes in the most recent Php versions.
- BitAndTest, BitOrTest, BitXorTest and Shared/DateTest were adversely affected by PR #3859 when arguments and/or expected results too large for a 32-bit integer were supplied.
- ImExpTest required a slightly reduced precision for 32-bit. No idea why this hadn't shown up earlier.
Change "mixed" declarations to more accurate types in test members; in particular, change those that would be flagged if we were to run Phpstan at level 9 (we currently run level 8). I may or may not follow up with source code (over 700 level-9 problems remain for src), but, as with strict typing, there is no reason to avoid the effort for test members.
It was necessary to update some doc blocks in src to accommodate this change. However, no executable code is touched.
This was suggested by the investigation of issue #3811. No fix is necessary for the issue. However, two possible code solutions (Php setlocale, which comes with certain design flaws, and StringHelper set(Decimal/Thousands)Separator were suggested, and neither is adequately tested. This PR adds such tests.
Unusually, getting StringHelper Decimal Separator, Thousands Separator, and Currency Code can result in a change to those properties. So, the existing design in several tests where those properties are captured in Setup and restored in Teardown do not work quite as designed. Instead, the ability to set those properties to their default value (null) is added, and the tests re-done to restore the default in Teardown.
The two methods yield the same results when parsing input. However, they diverge when examining output fields through `getFormattedValue`. Such output is currently correct (usually) when using setlocale, but not when using StringHelper. The former works through the 'trick' of using `sprintf(%f)`, which generates a locale-aware string. However, using non-locale-aware `sprintf(%F)` followed by `str_replace` will produce the correct result for both setlocale and StringHelper. One place in the code uses a cast to string, which is incorrect for both methods. Following that up with the same str_replace makes it correct for both. These changes permit, but do not require, the user to avoid setlocale altogether.
It remains an open question whether Settings/Calculation::setLocale should set DecimalSeparator, CurrencySeparator, and CurrencyCode. That makes logical sense, but it would be a breaking change, and having to explicitly set those values when using setLocale does not seem especially burdensome. For now, such a change will not be made.
Fix#3782. A signature of the ReflectionMethod constructor will be deprecated, and PhpSpreadsheet runs afoul of that change in one place. Php8.4 is not yet available in any form, and I am reluctant to make this change until we see that the issue is real (and that this PR fixes it), so am leaving this PR in draft status till then.
I note that one Excel function `PI` is implemented not as a class method in PhpSpreadsheet, but rather as a call to the native Php function `pi`. The ReflectionMethod call is subject to a TypeError in the changed code, but that is already the case. We haven't seen a TypeError because (a) it will arise only if the caller supplies an argument to the function (which must be called with zero arguments), and (b) there are no test cases for that function. The code is slightly cleaned up, and test cases are now added. This is not an important enough problem to rush this PR - the existing code (and the changed code), rather than failing with TypeError, will fail with a CalculationException (wrong number of arguments) before it gets to the TypeError; that is the correct behavior.
Because they tend to clutter our code a lot and unfortunately, there are
lots of false positives. Instead, it would probably be better to deal
with false positives out of band, via the Scrutinizer web UI.
While we might never be able to have 100% of our code strict, we can at
the very least do it for all of our tests. This ensures that our tests
are using our API with the types as intended by the test author, and not
silently be cast to what our API requires.
And quite a bit more manual changes. The idea is that typing of our
tests can be a bit more loose, so we assume PHPDoc is mostly correct. If
that happens to be wrong, it should be caught by the tests themselves.
* Correct Re-computation of Relative Addresses in Defined Names
Fix#3661. Insertion or deletion of rows or columns can cause changes to the ranges for Defined Names. In fact, only the absolute parts of such ranges should be adjusted, while the relative parts should be left alone. Otherwise, as the original issue documents, the adjustment to the relative portion winds up being double-counted when the Defined Name is referenced in a formula. The major part of this change is to ReferenceHelper and CellReferenceHelper to not adjust relative addresses for Defined Names. An additional small change is needed in the Calculation engine to `recursiveCalculationCell` when a Defined Formula is being calculated.
In a sense, this is a breaking change, but for an obscure use case which (a) was wrong, and (b) is unlikely to be of importance. Some of the tests in ReferenceHelperTest were wrong and are now corrected, with the results being cross-checked against Excel.
When a Defined Name using relative addressing is defined in Excel, the result is treated as relative to the active cell on the sheet in which the name is defined. PhpSpreadsheet treats it as relative to cell A1. I think that is a reasonable treatment, and will not change its behavior to match Excel's - that would definitely be a breaking change of some consequence.
An interesting use of relative address in a defined name is demonstrated at https://excelguru.ca/always-refer-to-the-cell-above/. Note that the steps there involve setting the selected cell to A2 before defining the name. When that spreadsheet is stored, the actual definition of the range is `A1048576`. Likewise, adding a defined name for the cell to the left would be stored as `XFD1`. This seems a little fragile, but Ods, which I believe does not have the same row and column limits as Excel, certainly treats these values the same as Excel. This particular construction is formally unit-tested. Note, however, that although using these Defined Names as a formula on their own works just fine, a construction like `=SUM(A1:CellAbove)`, as suggested in the article, seems to put PhpSpreadsheet calculation engine in a loop. In the likely event that I can't solve that before I merge this change, I will open a new issue to that effect when I do merge it. Note that this can be handled without defined names as `=SUM(A$1:INDIRECT(ADDRESS(ROW()-1,COLUMN())))`. PhpSpreadsheet will handle this as a cell formula, but not yet as a Named Formula.
The tests show a breakdown evaluating `=ProductTotal` (product of 2 formulas using defined names with relative addresses) on the sheet on which it is defined, but it works from a different sheet. The usual debugging techniques show me why this is happening, but I can't see how to overcome it. As above, if I can't solve it before I merge, I will open a new issue.
For those situations where I intend to open a new issue, tests are added but are marked Incomplete. Because of those, I will leave this PR in draft status for 2 weeks before moving forward with it.
* Fix productTotal Problem
Need to restore current cell after evaluating defined name.
* Check For and Eliminate Octal Literals
Php8.1 introduced a new form of octal literal (0o123); the old form (0123) continues to be supported, but can certainly be misleading. There are exactly 6 uses of the old form in the code base (5 in tests, 1 in infra). 4 of these are clearly unintended (02 or 03), and the leading 0 should just be dropped for those. The other 2 are for file permissions and are easily replaced.
* Minor Changes to LocaleGenerator
Testing the permissions change exposed some problems when the locale files don't exist. This never shows up as an issue when the files do exist, which is pretty much all the time, but correct the theoretical exposures anyhow.
* Populate bg and en_uk Locale Files from Spreadsheet
Also rename locale generator so that it is executed first. This is when it should happen because other tests depend on its results.
* Some Bulgarian Function Omissions
Figured out a reasonable way to compare old Bulgarian function file to new; this identified a handful of now-corrected omissions and errors. Note that JIS function, in old list, is not on spreadsheet, but the Bulgarian translation is the same as English (so the translation isn't needed), and the function has been replaced by DBCS, which is also not on the spreadsheet.
Fix#3624. Use the same logic as elsewhere in the same module to invoke `imagesavealpha` when appropriate. (I confess that I do not understand the use case where you would not use imagesavealpha.) The fix was easy; writing a test was not. Google to the rescue.
* Cleanup for Phpstan, PHP-CS-Fixer Upgrades
Dependabot wanted to upgrade, but changes resulted in new "errors". Fix them.
* Scrutinizer
It says value must be numeric; Phpstan says it might not be. Try to reconcile them.
* Changes to NUMBERVALUE, VALUE, DATEVALUE, TIMEVALUE
Fix#3574. Reporter received deprecation notice for NUMBERVALUE function with invalid arguments. In fact, the arguments turn out to be valid after all; NUMBERVALUE treats a null-string or an all-blank-string in the first argument as if it were 0. Fixed this, and added several test cases suggested by it.
VALUE had been parsing its argument the same way as NUMBERVALUE. However, VALUE does not substitute 0 for null-string or all-blank-string. Coded up the difference between the two, and added the same tests for VALUE as for NUMBERVALUE.
VALUE can also pass its argument to DATEVALUE or TIMEVALUE. It is currently over-permissive about that, because Php is over-permissive, e.g. `new DateTime('q')` will return a DateTime object with the current date and time with a timezone of 'q'. Excel will, naturally, return `#VALUE!` for `DATEVALUE('q')`. I don't know that we can ever match Excel's (AFAIK not formally documented) decisions here 100%, but we can get a lot closer by parsing the date string if and only if it contains at least one digit. Code to enforce that is added to DATEVALUE and TIMEVALUE, and appropriate tests are added.
* Failure for Php 7.4 Linux
After not seeing any such problem for many months, this is the second day in a row where result is different on Windows than Linux with no apparent reason to think why that should be the case. At any rate, easily solved.
Fix#3561. PhpSpreadsheet VLOOKUP/HLOOKUP allow the specification of index_number (third parameter) as an array when lookup_value (first parameter) is supplied as an array. Change to permit index_number as an array even when lookup_value is not an array, as Excel does.
Successor to PR #3523. There are 494 single-line changes (`public function provider` to `public static function provider`) in this PR. None of these were made manually; they were all created with the following script (adapted from
https://stackoverflow.com/questions/25909820/how-to-recursively-iterate-through-files-in-php):
```php
$dir = 'C:/git/unit10prep2/tests/PhpSpreadsheetTests';
$it = new RecursiveDirectoryIterator($dir);
// Loop through files
foreach(new RecursiveIteratorIterator($it) as $file) {
if ($file->getExtension() === 'php') {
$contents = file_get_contents($file);
$new = preg_replace('/public function (\\w*)([Pp])rovider/', 'public static function $1$2rovider', $contents);
if ($new !== $contents) {
echo "changing $file\n";
file_put_contents($file, $new);
}
}
}
```
After this PR, there will be one more, with a small number of test changes, and enabling PhpUnit 10 for Php 8.1+.
* PhpUnit 10 Compatibility Part 1
This is not a change to move to PhpUnit 10. There is no compelling reason to do so at this time, although it is bound to happen eventually. There are a staggering number of problems (somewhere around 3,000) with the current test suite under PhpUnit 10; this is an attempt to get ahead of the curve by addressing them now.
Method `setOutputCallback` has gone away. This affects only Helper/SampleTest. It appears that `ob_start` and its allies provide an effective equivalent. FWIW, the absence of `setOutputCallback` is a good indication of whether or not PhpUnit 10 is in use, and I will use that fact in a few tests.
Class `ComplexAssert` with no constructor, and always used with `new ComplexAssert()`, extends `TestCase`. Apparently, the constructor for TestCase requires an argument, and PhpUnit 10 complains about not supplying one. Adding an empty constructor to ComplexAssert avoids this problem.
There are two very minor source changes, to Calculation/Calculation and Reader/Xlsx, where problems were exposed with PhpUnit 10 that had not been previously been exposed. AFAIK, these are the only source changes required; the rest of the changes are to test members.
The bulk of the problems are because PhpUnit 10 insists that provider methods be static. Most of those can be changed by a script without any further action; those changes will constitute the Part 2 counterpart of this PR. In this PR you will find the exceptional cases that can't be automated for one reason or another. The tests for Database functions have mild complications that are easily handled. Most of the other provider changes in this PR are because the method names didn't follow an established pattern ('provider' isn't part of the method name); those are also easily handled manually. Modifying the following tests provided significant challenges:
- Writer/Xls/WorkbookTest testAddColor
- Worksheet/Table/TableTest testSetRangeValidRange
The handling of warning messages issued by the code differs in PhpUnit 10. According to the change log, "This means that using PHP functionality which triggers E_DEPRECATED, E_NOTICE, E_STRICT, or E_WARNING or calling code which triggers E_USER_DEPRECATED, E_USER_NOTICE, or E_USER_WARNING can no longer hide a bug in your code." To me, the effect of that change seems to be exactly the opposite - such messages were available to the test with PhpUnit 9 (so we could test for them), and are no longer available (so we can't). I haven't even succeeded with a custom error message handler as part of the script. I will continue to investigate, but, for now, will skip some tests under PhpUnit 10 for the following:
- Shared/OleTest testChainedWriteMode and testChainedBadPath
- Reader/Html/HtmlLoadStringTest testLoadInvalidString
- Reader/Html/HtmlTest testBadHtml
* Scrutinize, and Parent Construct
Parent construct suggested by @MarkBaker.
* Redo Tests Dependent on Warning Messages
Warning (and other) messages are handled differently in PhpUnit 10 than in earlier versions.
Stop a few of scrutinizers complaints about 100% valid use of PHP variadics.
Hopefully, this issue will cease to be an issue when we can specify mixed datatype for variadic arguments in the tests
Also enforce more rigorous Excel Function implementation by testing the underlying implementation, call via the Calc Engine, and execution from in a worksheet.
Separate out unhappy path (exception) checks into a separate test, so that a single test isn't made overcomplex checking for every potentiality.
Scrutinizer may dislike variadics, for variable number of arguments of mixed type; but tough. It's 100% valid PHP, accepted by phpstan, and makes life a lot easier.
Initial work here covers all the database and datetime unit tests for Excel function implementations.
* Coerce Bool to Int for Unary Operation on Arrays
Fix#3389. It seems some functionality was left behind when JAMA was eliminated (PR #3260). In particular, it is apparently a known trick to use double negation on boolean values as arguments to functions like SUMPRODUCT.
* Fix 3396
Treat booleans in arrays as int for mathematical operators as well.
* Edge Case
When array operand was neither numeric nor boolean, PhpSpreadsheet had always been evaluating the operand as #NUM!. It will now propagate an error string like #DIV/0!, and treat non-error strings as #VALUE!, consistent with Excel.
* 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.
* Resolve Phpstan Messages in Calculation Statistical
Reduce number of Phpstan messages by addressing their issues.
I did not eliminate the messages for Trends, because I did not understand why it is working as it does, so felt it was better to leave it alone.
* Scrutinizer
A new false positive.
* 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.
* Clean Up Documentation for Worksheet
This PR was intended to clean up Phpstan/Scrutinizer messages regarding Worksheet. It is, for the most part, straightforward, but there is one problem which complicates things. The frequently-called public method `getParent` returns `Worksheet` or `null` but is documented to return only `Worksheet`. This can be addressed in either of two ways - change the code to match the documentation (smaller number of changes but a backwards compatibility break), or change the documentation to match the code (larger number of changes but no compatibility break). I have prepared a PR for each approach, but avoiding a compatibility break seems better, so I am pushing the latter. I can switch to the other if preferred. Most existing internal calls to `getParent` are changed to use a new method `getParentOrThrow`, which will throw an exception if parent is null. These calls would all have thrown a null pointer exception anyhow in that situation, so this should not cause any new breaks.
* Scrutinizer
One false positive, and one message leading to a minor code improvement.
* 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.
Test both the function implementation (directly), and when the function is called in a formula from a spreadsheet
Replace error strings in expected result for providers with the value returned from the ExcelErrors class
* 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 - Everything Else
I think it's best to install these before PR #3166. This one, which I hope to be the last in this series, does have some minor changes to source code, as well as to doc-blocks and to test members which continue to inadvertently use calls to deprecated functions.
* Some Remaining Deprecations in Tests
Fix them now.
* Minor Docblock Updates
Worksheet::unprotectCellsByColumnAndRow was incorrect. Other changes are cosmetic, leading to slightly better documentation.
* Update Worksheet.php
* 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.
oleibman
Adrien Crivelli
MarkBaker