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.
* Correct Xlsx Parsing of quotePrefix="0"
Fix#3435. Mis-parsed attribute is not normally generated by Excel or PhpSpreadsheet, but some 3rd-party software (correctly) generates it.
* Update Issue3435Test.php
* WIP Conditional Formatting Improvements for Xlsx
Fix#3370. Conditional styles are always generated with 5 borders (right, left, top, bottom, diagonal) even though the border style is none in each case. For the spreadsheet in question, top and bottom were inappropriate and interfered with the desired formatting. A new border style, BORDER_OMIT is added which will cause the Xlsx Writer to not generate that style. All conditional borders will be initialized with that value. Any border included in the Xml will, of course, change it to the specified type.
Fix#3202. User wants a condition to use "No format set" as you can in Excel. A new boolean property `$noFormatSet`, along with setter and getter, is added to Style/Conditional. It is initialized to false. User can call setter to change it. More importantly for the issue in question, if the Xlsx Reader encounters a `cfRule` tag which does not have a `dxfId` attribute (i.e. no style is associated with the rule), it will set noFormatSet to true. Similarly, the Xlsx writer will not generate a `dfxId` tag when noFormatSet is true.
This change is applicable only to Xlsx. Html, Csv, and Ods do not have support for Conditional Formatting. Limited support was added to Xls with PR #2696 in April 2022 and PR #2702 about a month later. However, with the current release code, Xls equivalents of the two new test spreadsheets in this PR are too complicated to be handled correctly by PhpSpreadsheet - loading and then saving them as Xls results in Excel complaining of corruption, and the results don't meet expectations. Since I have no idea how BIFF works, and since the problems with those spreadsheets are not caused by this PR, I am not planning to address those problems at this time.
* Update Documentation, Write Alignment and Font Less Often
It doesn't cause any particular harm except for small increases in file size and run time, but Alignment tags are written even when (a) all its attributes are null for Conditional Formatting, and (b) when the xml specifically indicates that Alignment should not be applied. Similarly, Font is written even when all its attributes are null for Conditional Formatting.
There are some errors in the Conditional Formatting documentation. Specifying a solid fill color in a Conditional Style requires the use of endColor, not StartColor. The discussion of Order of Evaluating is not entirely accurate. I have changed it to what I believe is an accurate explanation of how Excel works; and also added a mention that other spreadsheet programs might not work the same way, adding a couple of illustrations of the difference. The description of the multiple conditions did not quite match the diagram. 'Stop if true' was a blank paragraph; it is now described, and the new 'No format set' option is described in that paragraph since (I think) it would be used most often in conjunction with 'Stop if true'.
* Xlsx Writer Allow StartColor for Conditional Solid Fill
To set a solid fill in a non-conditional style, you set StartColor (xml will use that value as fgColor and a default value as bgColor). If you instead set EndColor (xml will use that value as bgColor and a default value as fgColor), the styling will not work as expected.
However, for conditional styles, if you set StartColor (xml will use that value as fgColor and not specify bgColor), the styling will not work as expected. If you instead set EndColor (xml will use that value as bgColor and not specify fgColor), the styling will work as expected.
Together, this means that you need to use different methods for non-conditional style fill than for conditional style fill. This isn't a big problem, but it is a bit weird. This PR changes Xlsx Writer so that if (a) fill is olid and (b) startColor is specified and (c) endColor is null, the xml will be written as bgColor without specifying fgColor. This means that you can set StartColor for both conditional and non-conditional and get the expected styling. You may, of course, continue to specify EndColor instead for conditional.
* Fix Some (Not Many) Xls Problems
I will open an issue for the (pre-existing) remainder.
I hadn't noticed some minor corruption in a file. It is fixed. Xls now supports BAHTTEXT as _xlfn.BAHTTEXT, not as an index to a list of built-in functions, so it is no longer suitable for this test and is removed from the spreadsheet. TRANSPOSE changes, e.g. A1:B2 to {1,2;3,4}, and the Xls Writer Parser has trouble with that, so its test is also removed from the test spreadsheet.
PR #3340 increased coverage in Reader Xls for most functions. Some of the omissions from that PR (see below) were intended from the start. However, the set of (BINOMDIST, EXPONDIST, GAMMADIST, NORMDIST, POISSON, WEIBULL, and FIXED) were not intended to be omitted; they just did not seem to work. Having had time to research, it appears that the problem is not those functions themselves, but rather the use of Boolean constants as function arguments (see issue #3369). Knowing that, it is possible to add those missing functions back in, just taking care to use `0` or `1` or `TRUE()` or `FALSE()` rather than boolean constants as arguments. No update to source or test code; just adding a few new cells to an Xls spreadsheet.
Functions still omitted because they return array results:
- GROWTH
- LINEST
- LOGEST
- MINVERSE
- MMULT
- TRANSPOSE
- TREND
Functions still omitted because they are not implemented in PhpSpreadsheet:
- BAHTTEXT
- CELL
- FREQUENCY
- GETPIVOTDATA
* Permit Max Column for Row Breaks
Fix#3143. Page break was dropped. Difference between good and bad was the use of attribute `max` in `brk` tag in the good spreadsheet. However, `max` was *not* required in a similar spreadsheet. So the reason for the problem isn't completely explained. Nevertheless, it can't really hurt to capture the `max` value on read (if present) and generate it on write. This resolves the issue. User is also permitted to specify max column when setting a row break programatically. I am not yet in position to document when that might be a good idea.
* Case-sensitive Directory Name
Not a problem on my Windows system.
* Update Documentation and Add Tests
Change is necessitated by probable Excel bug.
* Unhappy With Initial Implementation
I kind of shoe-horned it in. Better to create a new PageBreak class, which will make it easier to accomodate any future surprises about page break handling. The only difficulty with the new approach is making sure getBreaks maintains backwards compatibility. New tests will ensure that.
* Allow More Fonts/Fontnames for Exact Width Calculation
Fix#3190. A limited set of explicitly-named font files can be used when an exact width calculation is required. User noted that font files are named differently on Mac than on Windows (if the fonts are installed on Linux, the font names probably match Windows). Since the algorithm for generating the Mac file name from the font name seems easy, that algorithm is invoked when the Windows-named file is not found.
Moving on from there, it seems odd that only a small set of fonts are supported. It is, of course, impossible to support all possible fonts out of the box. However, it is possible to allow the user to supply additional mappings from font name to file name (or override existing mappings if neither the Windows nor Mac name matches the user's system), and doing so is permitted with this change:
```php
\PhpOffice\PhpSpreadsheet\Shared\Font::setExtraFontArray([
'fontname' => [ /* More than 1 can be specified */
'x' => 'fontfilenamefornormal.ttf',
'xb' => 'fontfilenameforbold.ttf',
'xi' => 'fontfilenameforitalic.ttf',
'xbi' => 'fontfilenameforbolditalic.ttf',
],
]);
```
* Unexpected Test Difference
... between Windows and Linux.
* Additional Coverage for Reader Xls
More than 750 lines are newly covered, increasing overall coverage on my machine from 91.14% to 93.09%.
* Update FormulasTest.php
* 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.
* Namespace-Aware Code for SheetViewOptions, SheetProtection (minor break)
This is fairly straightforward for SheetViewOptions.
SheetProtection is a bit less straightforward. All the attributes should allow for null as well as boolean; this is especially important because Excel defaults to true for some of them and false for others (existing code always treated attributes missing from the XML as false). DocBlocks are updated to indicate the defaults. The test for isProtectionEnabled is changed to reflect this reality. Also, PhpSpreadsheet has been using true/false when writing these, but Excel uses 1/0. Because of these differences, this is a breaking change, but the consequences of the breaks should be minor. Two unit test results needed to change.
Remaining areas in Reader/Xlsx which still use the namespace-unaware code include conditional formatting (internal or external), auto filters, unparsed loaded data, data validation (internal or external), alternate content, and header/footer images.
* Corrections to Xls Reader and Writer
New test and changed sample to go with code changes. Also doc change.
* 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
* Problems Formatting Very Small and Very Large Numbers
Fix#3128 (author found a workaround but the problem remains). For some complex masks, when a cast of the cell value from float to string results in the use of scientific notation, the result of the formatting is unusable. I believe this PR solves the problem for numbers close to zero (Php cast from float to string uses scientific notation starting with 1E-5), and for a range of large numbers which are not handled correctly now.
However, I have not found a way to ensure that the results match Excel for very large numbers (1E18 or larger); this change at least ensures that the resulting string is an accurate rendition of the number (which is not the case now) even if doesn't match Excel. As an example, if you use the mask reported in the original issue `0 000.0` and enter a value of 1E90 into the cell, Excel will show it as 1 followed by 87 zeros, a space, 3 more zeros, decimal point and zero. I have not figured out how to get PhpSpreadsheet to do that; for now, it will just return the formatted value as 1 followed by 90 zeroes instead (I might have chosen to go with scientific notation instead). I will continue to think about those, but do not feel it is worth delaying the improvements in this ticket while I do so.
The affected section of code also truncated to the appropriate precision. It now rounds, as Excel does.
This seemed to be an area of code where problems might arise on 32-bit systems, and, indeed, I found something in the formatting code which had to be changed for 32-bit to work correctly. As long as I was doing that anyhow, I ran the full test suite, and found that Php8.1 had introduced some new stringencies which caused problems in a handful of places. All were found in Xls Reader, and all are corrected now.
* Remove Dead Assignment
Scrutinizer will be happy now.
* 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.
* Generation3 Copy With Image in Footer
Fix#3126. A worksheet contained an image in its footer. It could be loaded and saved as another spreadsheet. However, if you tried to load and save that spreadsheet, PhpSpreadsheet would be unable to find the footer image and would therefore throw an exception.
This error was introduced a long time ago, in PhpSpreadsheet 1.3.0. The apparent cause of the problem was PR #435, sometime around June 2018. That change was very useful, but it had problems which exposed themselves only with a third generation copy. An additional contributor to the issue at hand was PR #1690 (December 2020), which again exposed itself with a third generation copy.
The issue from 1690 is easier to explain and deal with. It added a 'ps' suffix to printer settings resources in Xlsx Reader (to avoid name conflicts), but did not limit itself to a single addition (so subseqent generations would have multiple ps's). It also neglected to add the suffix in Reader/Xlsx/PageSetup.
As for 435, it loops through all the worksheet relationships, and uses the last that it finds as the base for header/footer drawings. It has been changed to use only the relationship whose `rId` matches the worksheet's `legacyDrawingHF` `rId`. It also needs a bit extra validation to make sure a drawing exists before adding it to its array of header/footer images. It also meant that Xlsx/Writer/Rels might write an entry with the same rId twice. I have also changed the header/footer image processing to be namespace aware (see PR #3137).
* Minor Change
I didn't like the way I performed one operation.
* Fix Test
Array index should not have been constant.
* Xlsx Reader Namespace Aware Handling of Drawings, RowAndColumnAttributes, MergeCells
Fix#1482 (actually fix a problem recently attached to that ticket long after it closed). There were problems processing a spreadsheet generated by third party software. That spreadsheet used unexpected namespacing, and absolute paths within the zip file where relative paths were expected.
Xlsx Reader handles most, but not all, of its processing in a namespace-aware manner. Two versions of a worksheet's xml are available - `$xmlSheet` is not namespace aware and `$xmlSheetNS` is aware. This was necessary in order to add namespace support in an incremental manner. The primary reason to continue to use the unaware version is the absence of test cases. In particular, drawings, row and column attributes, and merge cells continue to use the unaware version; this PR changes those to use the aware version.
As noted in the summary above, a couple of new places in the handling of the those items were expecting file locations to be specified as relative paths in the zip file, but the file used absolute paths instead. Those unexpected usages are now addressed.
The user reporting the new problem tried a change which effectively made all uses of `$xmlSheet` namespace aware, and that seemed helpful. It may be time eliminate its usage altogether, whether or not we have appropriate examples of unexpected namespaces to test with. I will not do that with this change, but I may add a new PR to do so after this one is merged. Remaining areas which still use the unaware version include conditional formatting (internal or external), sheet view options, sheet protection, auto filters, unparsed loaded data, data validation (internal or external), alternate content, and header/footer images.
There is an interesting anomaly with the new test file. When I load it and save it, the appearance of the output file does not quite match the input. Oddly, the output file seems much better than the input - the picture no longer covers any data, for example. This is because, in particular, the output file row heights and column widths seem to match the xml, but the input file does not. For example, the xml in both files seems to indicate that row 5 should have a height of 234, which it does in the output file, but the height of that row when the input file is opened is 156. It appears that all row heights and column widths when the input file is opened are very close to 2/3 of what is expected. I will continue to research that anomaly for a few days, but I will not let it prevent me from moving forward with this PR if I don't find the explanation. Whatever that problem is, it seems distinct from the namespacing/pathing problems which the PR addresses.
* Scrutinizer New False Positives
Eliminate them with annotations.
* Using htmlspecialchars to fix issue #3145: this prevents the SimpleXMLElement error "unterminated entity reference" when HTML entities are present in a worksheet
* replace htmlspecialchars with proposal of oleibman
* eliminate string cast, because PhpStan has a problem with that
* ignore string cast for phpstan + enhance unit test
MarkBaker
oleibman
Kevin Bond
Jokūbas
Robin