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.
* 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.
* Fix Unintential Deprecated Calls in Tests - INFORMATION
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 Some Deprecated Calls
Fix them now.
* Fix Unintential Deprecated Calls in Tests - LOOKUPREF
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
* One Intentional Deprecation
Annotate it.
* 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, ...)
When I cloned this morning, composer gave me a message that the lock file was not up to date with the latest changes in composer.json. I do not understand why, but it suggested to run `composer update`, which I did. This led to a handful of problems with php-cs-fixer, all fixed with changes to doc-blocks, and phpstan (only Writer/Xls/Worksheet required a change to code). We would presumably have had these problems at the start of next month when dependabot did its thing, so fix them now.
* 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.
* Floating-point Equality in Two Tests
Merging a change today, Git reported failures that did not occur during "normal" unit testing. The merge still succeeded, but ... The problem was an error comparing float values for equal, and the inequality occurred beyond the 14th decimal digit. Change the tests in question, which incidentally were not part of the merged changed, to use assertEqualsWithDelta.
* Egad - 112 More Precision-related Problems
Spread across 9 test members.
oleibman
Adrien Crivelli
MarkBaker