* Improve Identification of Samples in Coverage Report
The Phpunit coverage report currently contains bullet items like `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "49"`. This extremely simple change takes advantage of Phpunit's ability to accept an array with keys which are either strings or integers, by using the sample filenames as the array keys rather than sequential but otherwise meaningless integers (e.g. `49` in the earlier cited item). The bullet item will now read `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "Basic/38_Clone_worksheet.php"`.
* Fix for Issue 2158 (AverageIf Calculation Problem)
Issue #2158 reports an error calculating AverageIf because a function returns null rather than a string. There turn out to be several components to this problem:
- The nominal fix to the problem is to add some null-to-nullstring coercion in DatabaseAbstract.
- This fixes the error, but does not necessarily lead to the correct result because buildQuery treats values of null and null-string identically, whereas Excel does not. So change that to treat null-string as any other string.
- But that doesn't lead to the correct result either. That's because Functions/ifCondition recognizes a null string, but then continues to (over-)process it until it returns the wrong result. Fix this problem in conjunction with the other two, and we finally get the correct result.
A new unit test is added for AVERAGEIF, and new test cases are added for SUMIF. In each case, there are complementary tests for conditions of null and null-string, and the results agree with Excel. There may or may not be value in adding new tests to other functions, and I will be glad to do so for any functions which you care to identify, but no existing tests broke as a result of these changes.
* PHP8.1 Deprecation Passing Null to String Function
For each of the files in this PR, one or more statements can pass a null to string functions like strlower. This is deprecated in PHP8.1, and, when deprecated messages are enabled, causes many tests to error out. In every case, use coercion to pass null string rather than null.
* TextData - Minor Changes, Test Coverage
Per agreement on a previous push, I looked into standardizing the initialization of the TextData functions (like Engineering and MathTrig), with particular regard for avoiding multiple later null coercions. This simplifies the code quite a bit. This PR also increases coverage to 100% for all TextData modules. All entries in Phpstan baseline for non-deprecated TEXTDATA functions are removed. There were some minor bugfixes.
Whereas Excel (and Gnumeric) treat booleans when supplied as strings as 'TRUE' or 'FALSE', ODS treats them as '1' or '0'. Unlike Excel, ODS generally does not allow bool for int arguments; it does, however, allow them for FIND and SEARCH. ODS allows boolean for into for SUBSTITUTE even though Excel doesn't. ODS allows bool for string for NUMBERVALUE and VALUE even though Excel doesn't. ODS accepts 0 as an argument for CHAR; Excel doesn't. Most of this seems like random decisions on the part of the developers; I've done my best to follow the products in each case. There is a new test member devoted to ODS tests.
Gnumeric has an anomaly vis-a-vis the others - if length is supplied to LEFT/MID/RIGHT as null, Gnumeric treats it as 0 rather than 1.
All tests now take place in the context of a spreadsheet ...
Except for RETURNSTRING, which is not the implementation of an Excel function, and is referred to in the rest of PhpSpreadsheet only in the unit tests for itself. It should probably be deprecated, but that is not part of this PR, just in case there is some reason for it that I couldn't discern.
I have tried to make the first line of each doc block identify the Excel function name rather than its name in PhpSpreadsheet. I think it makes things more comprehensible.
Some tests call Settings::setLocale, but there was no Settings::getLocale. At the end of the tests which do it, they invoke setLocale('EN-US'), which, in a practical sense, is sufficient. However, in theory it would be better for them to get the current locale before changing it, then changing it back to the original when the time came. I have added getLocale and made the appropriate testing change.
The CHAR function took an interesting turn. One can set the value of a cell to, say, CHAR(2), the ASCII/UTF-8 representation of a control character, which is not legal in certain contexts. The only Reader/Writer that could handle this without problems is Xls, which deals with binary data all the time. However, if you tried to write it to Xlsx, Excel would not be able to open the resulting file because of what it considers an illegal character. I changed the Xlsx writer to escape such characters when writing the value of a string function. I did not make any other changes to the Xlsx writer - it seems to me that setting a cell to CHAR(2) is legitimate, but setting it to say `"\x02"` seems less likely to be legitimate, so the latter will still fail (although `="\x02"` should work). The Xlsx reader already supports the escape mechanism that I added to the writer.
CHAR control character and Ods - not supported by either Reader or Writer. I did not attempt to add this now. There is lots still missing from ODS, and this item just can't be a high priority amongst all of those.
CHAR control character and Csv - it is supported by reader and writer if the file has a csv extension. However, trying to guess the mime type without an extension - the control character makes mime_get_type guess application/octet-stream, and PhpSpreadsheet therefore thinks that Csv can't read it.
CHAR control character and Html. Actual use of the control character in the file is subject to the same problems as Xml (i.e. Xlsx and Ods). It wasn't terribly difficult to get the Html Writer to change `"\x02"` to "``". I believe that this is technically legal; however, DOMDocument.loadHTML rejects it as an illegal entity, and I am not convinced that it is wrong to do so, so I haven't changed the Html writer.
* Scrutinizer
Correct 3 minor errors.
* Fix for the BIFF-8 Xls colour mappings in the Reader
* Unit test for reading colours, writing hen rereading and ensuring that the RGB values have not changed
The Phpunit coverage report currently contains bullet items like `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "49"`. This extremely simple change takes advantage of Phpunit's ability to accept an array with keys which are either strings or integers, by using the sample filenames as the array keys rather than sequential but otherwise meaningless integers (e.g. `49` in the earlier cited item). The bullet item will now read `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "Basic/38_Clone_worksheet.php"`.
Allows basic column width conversion when importing from Html that includes UoM... while not overly-sophisticated in converting units to MS Excel's column width units, it should allow import without errors
Also provides a general conversion helper class, and allows column width getters/setters to specify a UoM for easier usage
For each of the files in this PR, one or more statements can pass a null to string functions like strlower. This is deprecated in PHP8.1, and, when deprecated messages are enabled, causes many tests to error out. In every case, use coercion to pass null string rather than null.
* Use of passing flags with Readers to identify whether speacial features such as loading charts should be enabled; no need to instantiate a reader and manually enable it before loading any more.
This is in preparation for supporting new "boolean" Reaer/Writer features, such as pivot tables
* Use of passing flags with Writers to identify whether speacial features such as loading charts should be enabled; no need to instantiate a writer and manually enable it before loading any more.
* Update documentation with details of changes to the StringValueBinder
Most of the remaining 32-bit-unsafe date handling that remains in PhpSpreadsheet is in AutoFilter. Cleaning this up demonstrated that there are a lot of problems with AutoFilter, and I will do it in (probably two) pieces.
In this PR:
- Dynamic date processing was really wrong. There were no tests nor samples to exercise this code. (If you need details, you can try running the new sample against old code.) It is completely re-written.
- ThisYear/Month/Week/Quarter had been omitted.
- Rules such as AUTOFILTER_RULETYPE_DYNAMIC_MONTH_2 were almost correct, but showed some off-by-1 errors. I suspect these were timezone-related, and therefore more obvious to those of us far away from Greenwich.
- All Autofilter tests are moved to a single directory.
- The documentation suggested using null with the Dynamic Date setup, but Phpstan did not like that in my new tests/samples. Rather than change the doc block, I changed the documentation to suggest null string.
- I created a new sample to generate sheets using all the dynamic filters.
- I have added some new unit tests for each of the dynamic filters. I would love to be able to add some "time travel" tests because the dynamic nature of the filter makes most of the results change from day to day, which presents significant challenges in writing comprehensive unit tests (the same is true for code coverage). I was not able to find a good way to simulate time within PhpUnit, but the Linux 'faketime' package was extraordinarily easy and helpful in allowing me to confirm some edge cases. I had less satisfactory results with some Windows equivalents, but was still able to run some tests.
- Code coverage increases from below 60% to above 80%.
To be done:
- Some 32-bit unsafe dates remain in filterTestInDateGroupSet.
- Also in some of the existing AutoFilter samples.
- Study existing unit tests for AutoFilter which use mocking to see if they can/should be replaced with 'real' tests.
- Improve code coverage in AutoFilter, AutoFilter/Column, and AutoFilter/Common/Rule.
* Document Properties - Coverage and 32-bit-safe Timestamps
While researching an issue, I noticed that coverage of Document/Properties was poor. Further, the use of int timestamps will eventually lead to problems for 32-bit PHP (see issue #1826).
Coverage Changes:
- Many property types with no special handling are enumerated but not tested. These are removed, but will continue to function as before.
- Existing code theoretically allows property to be set to an object, but there is no means to read or write such a property, and, even if there were, I don't believe Excel supports it. Setting a property to an object will now be changed to a no-op (can throw an exception if preferred).
- Since the Properties object now has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Large switch statements are replaced with associative arrays. Scrutinizer will like that.
- Coverage is now 100%.
<!-- end of coverage changes list -->
Timestamp Changes:
- Timestamps will be stored as int if possible, or float if not. This is, or will soon be, needed for 32-bit systems. Tests have been added for beyond-epoch dates, and run successfully with 32-bit.
- LibreOffice doesn't quite get the Created/Modified properties correct. These are written to the file as a string which includes offset from UTC, but LibreOffice ignores the offset portion when displaying them. Code had been generating these in UTC, but now generates them in default timezone, which should meet user's expectations.
<!-- end of timestamp changes list -->
Other Changes:
- Custom properties added to ODS Writer.
- Samples had not been generating any ODS files. One is now generated.
- Ods uses a single 'keywords' property rather than multiple 'keyword' properties.
- Breaking change - default company is changed to null string from Microsoft Corporation.
- Breaking change of sorts - PropertiesTest incorrectly tested a custom date property against a string, Reader/XlsxTest correctly tested against a timestamp converted to a string. PropertiesTest was defective, and will no longer work as coded; anyone using it as a model will likewise have a problem.
- PHP8.1 has been complaining for weeks about a time zone conversion test. I have now downloaded a version, and changed the code so that it will work in 8.1 as well as prior releases. (It is still likely that the existing code should work in 8.1, but I haven't yet figured out how to file a bug report.) In the course of testing, 3 additional 8.1 problems were reported (all along the lines of "can't pass null to strpos"), and are fixed with null coercion.
- Two Calculation tests failed because of large results on 32-bit system. These are corrected by allowing the functions involved to return float|int rather than int. I suspect that there are other functions with this problem, and will investigate as a follow-up activity.
- See issue #2090. I believe that changes between 17.1 and master will merely cause the problematic spreadsheet to fail in a different way. I believe that enclosing in quotes some variables passed to Document/Properties by Reader/Xlsx will eliminate the problem, but, in the absence of an example file, cannot say for sure.
- Properties tests are now separated out from Reader/XlsxTest and Reader/OdsTest, and now test both Read and Write (via reload).
<!-- end of other changes list -->
Miscellaneous Notes:
- There remains no support for Custom Properties in Xls Reader or Writer.
- We now have default timezones for all of PHP itself, Shared/Date, and Shared/Timezone. That is least one too many. I was unable to disentangle the latter two for this change, but will look into deprecating one or the other in future.
* Phpstan
6 baseline deletions, 2 docblock changes
* Scrutinizer's Turn
3 minor errors that hadn't blocked the request.
* Reader XML Properties - Eliminate strtotime
Piggyback on top of prior changes to eliminate 32-bit-unsafe call.
Add explicit tests for created, modified, and custom date properties.
* Document Properties - Coverage and 32-bit-safe Timestamps
While researching an issue, I noticed that coverage of Document/Properties was poor. Further, the use of int timestamps will eventually lead to problems for 32-bit PHP (see issue #1826).
Coverage Changes:
- Many property types with no special handling are enumerated but not tested. These are removed, but will continue to function as before.
- Existing code theoretically allows property to be set to an object, but there is no means to read or write such a property, and, even if there were, I don't believe Excel supports it. Setting a property to an object will now be changed to a no-op (can throw an exception if preferred).
- Since the Properties object now has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Large switch statements are replaced with associative arrays. Scrutinizer will like that.
- Coverage is now 100%.
<!-- end of coverage changes list -->
Timestamp Changes:
- Timestamps will be stored as int if possible, or float if not. This is, or will soon be, needed for 32-bit systems. Tests have been added for beyond-epoch dates, and run successfully with 32-bit.
- LibreOffice doesn't quite get the Created/Modified properties correct. These are written to the file as a string which includes offset from UTC, but LibreOffice ignores the offset portion when displaying them. Code had been generating these in UTC, but now generates them in default timezone, which should meet user's expectations.
<!-- end of timestamp changes list -->
Other Changes:
- Custom properties added to ODS Writer.
- Samples had not been generating any ODS files. One is now generated.
- Ods uses a single 'keywords' property rather than multiple 'keyword' properties.
- Breaking change - default company is changed to null string from Microsoft Corporation.
- Breaking change of sorts - PropertiesTest incorrectly tested a custom date property against a string, Reader/XlsxTest correctly tested against a timestamp converted to a string. PropertiesTest was defective, and will no longer work as coded; anyone using it as a model will likewise have a problem.
- PHP8.1 has been complaining for weeks about a time zone conversion test. I have now downloaded a version, and changed the code so that it will work in 8.1 as well as prior releases. (It is still likely that the existing code should work in 8.1, but I haven't yet figured out how to file a bug report.) In the course of testing, 3 additional 8.1 problems were reported (all along the lines of "can't pass null to strpos"), and are fixed with null coercion.
- Two Calculation tests failed because of large results on 32-bit system. These are corrected by allowing the functions involved to return float|int rather than int. I suspect that there are other functions with this problem, and will investigate as a follow-up activity.
- See issue #2090. I believe that changes between 17.1 and master will merely cause the problematic spreadsheet to fail in a different way. I believe that enclosing in quotes some variables passed to Document/Properties by Reader/Xlsx will eliminate the problem, but, in the absence of an example file, cannot say for sure.
- Properties tests are now separated out from Reader/XlsxTest and Reader/OdsTest, and now test both Read and Write (via reload).
<!-- end of other changes list -->
Miscellaneous Notes:
- There remains no support for Custom Properties in Xls Reader or Writer.
- We now have default timezones for all of PHP itself, Shared/Date, and Shared/Timezone. That is least one too many. I was unable to disentangle the latter two for this change, but will look into deprecating one or the other in future.
* Phpstan
6 baseline deletions, 2 docblock changes
* Scrutinizer's Turn
3 minor errors that hadn't blocked the request.
* Gnumeric Reader - Distinguish Created and Modified Timestamps
Both are being used to set both fields; change to set the appropriate one in each case.
Also replace use of 32-bit-unsafe strtotime.
* Document Properties - Coverage and 32-bit-safe Timestamps
While researching an issue, I noticed that coverage of Document/Properties was poor. Further, the use of int timestamps will eventually lead to problems for 32-bit PHP (see issue #1826).
Coverage Changes:
- Many property types with no special handling are enumerated but not tested. These are removed, but will continue to function as before.
- Existing code theoretically allows property to be set to an object, but there is no means to read or write such a property, and, even if there were, I don't believe Excel supports it. Setting a property to an object will now be changed to a no-op (can throw an exception if preferred).
- Since the Properties object now has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Large switch statements are replaced with associative arrays. Scrutinizer will like that.
- Coverage is now 100%.
<!-- end of coverage changes list -->
Timestamp Changes:
- Timestamps will be stored as int if possible, or float if not. This is, or will soon be, needed for 32-bit systems. Tests have been added for beyond-epoch dates, and run successfully with 32-bit.
- LibreOffice doesn't quite get the Created/Modified properties correct. These are written to the file as a string which includes offset from UTC, but LibreOffice ignores the offset portion when displaying them. Code had been generating these in UTC, but now generates them in default timezone, which should meet user's expectations.
<!-- end of timestamp changes list -->
Other Changes:
- Custom properties added to ODS Writer.
- Samples had not been generating any ODS files. One is now generated.
- Ods uses a single 'keywords' property rather than multiple 'keyword' properties.
- Breaking change - default company is changed to null string from Microsoft Corporation.
- Breaking change of sorts - PropertiesTest incorrectly tested a custom date property against a string, Reader/XlsxTest correctly tested against a timestamp converted to a string. PropertiesTest was defective, and will no longer work as coded; anyone using it as a model will likewise have a problem.
- PHP8.1 has been complaining for weeks about a time zone conversion test. I have now downloaded a version, and changed the code so that it will work in 8.1 as well as prior releases. (It is still likely that the existing code should work in 8.1, but I haven't yet figured out how to file a bug report.) In the course of testing, 3 additional 8.1 problems were reported (all along the lines of "can't pass null to strpos"), and are fixed with null coercion.
- Two Calculation tests failed because of large results on 32-bit system. These are corrected by allowing the functions involved to return float|int rather than int. I suspect that there are other functions with this problem, and will investigate as a follow-up activity.
- See issue #2090. I believe that changes between 17.1 and master will merely cause the problematic spreadsheet to fail in a different way. I believe that enclosing in quotes some variables passed to Document/Properties by Reader/Xlsx will eliminate the problem, but, in the absence of an example file, cannot say for sure.
- Properties tests are now separated out from Reader/XlsxTest and Reader/OdsTest, and now test both Read and Write (via reload).
<!-- end of other changes list -->
Miscellaneous Notes:
- There remains no support for Custom Properties in Xls Reader or Writer.
- We now have default timezones for all of PHP itself, Shared/Date, and Shared/Timezone. That is least one too many. I was unable to disentangle the latter two for this change, but will look into deprecating one or the other in future.
* Phpstan
6 baseline deletions, 2 docblock changes
* Scrutinizer's Turn
3 minor errors that hadn't blocked the request.
Add unit tests to cover all of HashTable. I was hoping to do this without source changes, but this class does require a deep clone, and, as the new unit tests revealed, the existing code did not fill the bill - it cloned objects, but not arrays which contained objects, and all the object variables in this class are arrays which can contain objects.
Having a parallel project to complete cover Document Properties, I turned my attention to to Document Security. As happens, this particular change grew a bit over time.
Coverage and Testing Changes:
- Since the Security object has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Almost all of the coverage for the Security Object came about through samples 11 and 41, not through formal tests with assertions. Formal tests have been added.
- All methods now use type-hinting via the function signature rather than doc block.
- Coverage is now 100%.
<!-- end of coverage and testing changes list -->
Bug:
- Xlsx Reader was not evaluating the Lock values correctly. This revelation came as a result of the new tests ...
- Which showed that Xlsx Reader was testing SimpleXmlElement as a boolean rather than the stringified version of that ...
- Which didn't matter all that much because Xlsx Writer was writing the values as 'true' or 'false' rather than '1' or '0', and (bool) 'false' is true.
- Xlsx Reader clearly needed a change. I was trying to avoid that while awaiting the namespacing change. At least this is restricted to a very small self-contained piece of the code.
- It is less clear whether Xlsx Writer should be changed. It is true that Excel itself uses 1/0 when writing; however it is equally true that it recognizes true/false as well as 1/0 when reading. For now, I have left Xlsx Writer alone to limit the change to what is absolutely needed.
<!-- end of bug list -->
Other Changes:
- I was at a complete loss as to what "lock revisions" was supposed to do, and it took a while to find anything on the web that explained it. Thank you, openpyxl, for coming through. I have documented it for PhpSpreadsheet now.
<!-- end of other changes list -->
Miscellaneous Note:
- There remains no support for Document Security in Xls Reader or Writer (nor in any of the other readers/writers except Xlsx).
- No Phpstan baseline changes, possibly for the first time in any of my PRs since Phpstan was introduced.
Co-authored-by: Mark Baker <mark@lange.demon.co.uk>
PR #2110 added some documentation for an unexpected observation when formula pre-calculation was set to false. I had suggested adding a unit test to demonstrate the observation, but I couldn't find any existing tests for PreCalc. This PR rectifies that omission.
This change restored behavior from PHP7 in PHP8. In PHP7 calling
setSize(0) resulted in font size being set to 10. The fix addresses
change to equal comparisons in PHP8. Extra comparison is added to keep
result from PHP7 in PHP8 for the setSize(0) case.
While researching an issue, I noticed that coverage of Document/Properties was poor. Further, the use of int timestamps will eventually lead to problems for 32-bit PHP (see issue #1826).
Coverage Changes:
- Many property types with no special handling are enumerated but not tested. These are removed, but will continue to function as before.
- Existing code theoretically allows property to be set to an object, but there is no means to read or write such a property, and, even if there were, I don't believe Excel supports it. Setting a property to an object will now be changed to a no-op (can throw an exception if preferred).
- Since the Properties object now has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed.
- Large switch statements are replaced with associative arrays. Scrutinizer will like that.
- Coverage is now 100%.
<!-- end of coverage changes list -->
Timestamp Changes:
- Timestamps will be stored as int if possible, or float if not. This is, or will soon be, needed for 32-bit systems. Tests have been added for beyond-epoch dates, and run successfully with 32-bit.
- LibreOffice doesn't quite get the Created/Modified properties correct. These are written to the file as a string which includes offset from UTC, but LibreOffice ignores the offset portion when displaying them. Code had been generating these in UTC, but now generates them in default timezone, which should meet user's expectations.
<!-- end of timestamp changes list -->
Other Changes:
- Custom properties added to ODS Writer.
- Samples had not been generating any ODS files. One is now generated.
- Ods uses a single 'keywords' property rather than multiple 'keyword' properties.
- Breaking change - default company is changed to null string from Microsoft Corporation.
- Breaking change of sorts - PropertiesTest incorrectly tested a custom date property against a string, Reader/XlsxTest correctly tested against a timestamp converted to a string. PropertiesTest was defective, and will no longer work as coded; anyone using it as a model will likewise have a problem.
- PHP8.1 has been complaining for weeks about a time zone conversion test. I have now downloaded a version, and changed the code so that it will work in 8.1 as well as prior releases. (It is still likely that the existing code should work in 8.1, but I haven't yet figured out how to file a bug report.) In the course of testing, 3 additional 8.1 problems were reported (all along the lines of "can't pass null to strpos"), and are fixed with null coercion.
- Two Calculation tests failed because of large results on 32-bit system. These are corrected by allowing the functions involved to return float|int rather than int. I suspect that there are other functions with this problem, and will investigate as a follow-up activity.
- See issue #2090. I believe that changes between 17.1 and master will merely cause the problematic spreadsheet to fail in a different way. I believe that enclosing in quotes some variables passed to Document/Properties by Reader/Xlsx will eliminate the problem, but, in the absence of an example file, cannot say for sure.
- Properties tests are now separated out from Reader/XlsxTest and Reader/OdsTest, and now test both Read and Write (via reload).
<!-- end of other changes list -->
Miscellaneous Notes:
- There remains no support for Custom Properties in Xls Reader or Writer.
- We now have default timezones for all of PHP itself, Shared/Date, and Shared/Timezone. That is least one too many. I was unable to disentangle the latter two for this change, but will look into deprecating one or the other in future.
This PR came about as I pondered how feasible it was to change the default escape character from backslash to null string, since the latter emulates Excel's own actions. Also, surveying issues relating to CSV, it seems that people are often in a situation where the current defaults aren't optimal for them (e.g. they are in a region where semicolon rather than comma is a better default delimiter). My case and that case can both be handled by methods after a reader is constructed. However, the issues also show that many use `IOFactory::load` rather than `new Csv()`, and the methods to affect the defaults are not available in that case.
Adding a static callback that can be invoked by the constructor addresses all these problems. This can be set as part of the user application's normal initialization, and no special attention needs to be paid to CSV loads thereafter, no matter how they are invoked.
This also makes it feasible to use 'guess' as inputEncoding, by providing a new setFallbackEncoding (default CP1252) method to use if none of the heuristic tests pass. There was already the ability to guess the encoding before `$reader->load()`, but not before `IOFactory::load`.
Almost all typehints in Reader/Csv and Reader/Csv/Delimiter are now part of the function signature rather than in the DocBlock. The exceptions are one method in Delimiter which uses a `resource` parameter, and the `canRead` and `load` methods, which must match the signature in IOFactory. I will look into changing those later.
The Csv Reader tests are moved into their own directory. All Phpstan baseline entries involving Csv Reader are eliminated.
Fix for #2082. Xlsx Writer was writing a cell which is a formula which evaluates to boolean false as an empty XML tag. This is okay for Excel 365, but not for Excel 2016-. Change to write the tag as a value of 0 instead, which works for all Excel releases. Add test.
19_NamedRange.php was not changed to use absolute addressing when that was introduced to Named Ranges. Consequently, the output from this sample has been wrong ever since, for both Xls and Xlsx.
There was an additional problem with Xls. It appears that the Xls Writer Parser does not parse multiple concatenations using the ampersand operator correctly. So, `=B1+" "+B2` was parsed as `=B1+" "`. I believe that this is due to ampersand being treated as a condition rather than an operator; `A1>A2>A3` isn't valid, but `A1&A2&A3` is. My original PR (#1992, which I will now close) only partially resolved this, but I think moving ampersand handling from `condition` to `expression` is fully successful.
There are already more than ample tests for Named Ranges, so I did not add a new one for that purpose. However, I did add a new test for the Xls parser problem.
* Additional unit tests for VLOOKUP() and HLOOKUP()
* Additional unit tests for CHOOSE()
* Unit tests for HYPERLINK() function
* Fix CHOOSE() test for spillage
* Let's see if the tests now pass against PHP8; output file looks to be good
* Font can't be both superscript and subscript at the same time, so we use if/else rather than if/if
* Gnumeric Better Namespace Handling
There have been a number of issues concerning the handling of legitimate but unexpected namespace prefixes in Xlsx spreadsheets created by software other than Excel and PhpSpreadsheet/PhpExcel.I have studied them, but, till now, have not had a good idea on how to act on them. A recent comment https://github.com/PHPOffice/PhpSpreadsheet/issues/860#issuecomment-824926224 in issue #860 by @IMSoP has triggered an idea about how to proceed.
Although the issues exclusively concern Xlsx format, I am starting out by dealing with Gnumeric. It is simpler and smaller than Xlsx, and, more important, already has a test for an unexpected prefix, since, at some point, it changed its generic prefix from gmr to gnm. I added support and a test for that some time ago, but almost certainly not in the best possible manner. The code as changed for this PR seems simpler and less kludgey, both for that exceptional case as well as for normal handling.
My hope is that this change can be a template for similar Reader changes for Xml, Ods, and, especially, Xlsx.
All grandfathered Phpstan issues with Gnumeric are fixed and eliminated from baseline as part of this change.
* Namespace Handling using XMLReader
Adopt a suggestion from @IMSoP affecting listWorkSheetInfo, which uses XMLReader rather than SimpleXML for its processing.
* Update GnumericLoadTest.php
PR #2024 was pushed last night, causing a Phpstan problem with this member.
* Update Gnumeric.php
Suggestions from Mark Baker - strict equality test, more descriptive variable names.
As issue #2042 documents, SUM behaves differently with invalid strings depending on whether they come from a cell or are used as literals in the formula. SUM is not alone in this regard; COUNTA is another function within this behavior, and the solution to this one is modeled on COUNTA. New tests are added for SUM, and the resulting tests are duplicated to confirm correct behavior for both cells and literals.
Samples 16 (CSV), 17 (Html), and 21 (PDF) were adversely affected by this problem. 17 and 21 were immediately fixed, but 16 had another problem - Excel was not interpreting the UTF8 currency symbols correctly, even though the file was saved with a BOM. After some experimenting, it appears that the `sep=;` line generated by setExcelCompatibility(true) causes Excel to mis-handle the file. This seems like a bug - there is apparently no way to save a UTF-8 CSV with non-ASCII characters which specifies a non-standard separator which Excel will open correctly. I don't know if this is a recent change or if it is just the case that nobody noticed this problem till now. So, I changed Sample 16 to use setUseBom rather than setExcelCompatibility, which solved its problem. I then added new tests for setExcelCompatibility, with documentation of this problem.
* Defined names/formulae in ODS are prefixed by $$ when used in a formula; so we need to strip this out to fully convert them to an Excel formula
* Test for ODS Writer for DefinedNames
* First steps in the implementation of AutoFilters for ODS Reader and Writer, starting with reading a basic AutoFilter range (ignoring row visibility, filter types and active filters for the moment).
And also some additional refactoring to extract the DefinedNames Reader into its own dedicated class as a part of overall code improvement... on the principle of "when working on a class, always try to leave the library codebase in a better state than you found it"
* Provide a basic Ods Writer implementation for AutoFilters
* AutoFilter Reader Test
* AutoFilter Writer Test
* Update Change Log
* Pattern Fill style should default to 'solid' if there is a pattern fill style for a conditional; though may need to check if there are defined fg/bg colours as well; and only set a fill style if there are defined colurs
* Fix for Issue 2029 (Invalid Cell Coordinate A-1)
Fix for #2021. When Html Reader encounters an embedded table, it tries to shift it up a row. It obviously should not attempt to shift it above row 1. @danmodini reported the problem, and suggests the correct solution. This PR implements that and adds a test case.
Performing some additional testing, I found that Html Reader cannot handle inline column width or row height set in points rather than pixels (and HTML writer with useInlineCss generates these values in points). It also doesn't handle border style when the border width (which it ignores) is omitted. Fixed and added tests.
* Completion of refactoring for Excel Lookup and Reference functions
* Fix LookupRef tests checking for cell existence
* Fix a couple of now invalid callable references in the Calculation Engine lookup table
Both methods used to optionally return null if passed a
second argument. This second argument was removed entirely and the
method always returns a RowDimension or ColumnDimension respectively
(possibly creating it if needed).
This make the API more predictable and easier to do static analysis
with tools such as PHPStan.
If you relied on that second parameter, you should instead use the
`Worksheet::getRowDimensions()` or `Worksheet::getColumnDimensions()` and
check for existence yourself before calling the getters.
* DateTimeExcel - Change Names of funcWhatever to evaluate
Per discussions while MathTrig was being broken up, this would help standardize the code. This PR applies that standardization to the DateTimeExcel family of functions.
The deprecation messages in DateTime.php are changed to match the style used in PR #2005.
All Phpstan grandfathered errors (about 25) in DateTimeExcel are fixed and removed from baseline. A small number (about 5) of phpstan annotations in the source members in that directory are also fixed and eliminated.
* MathTrig - Fix Phpstan Accomodations
This should be the last of my mass changes to MathTrig. All he Phpstan violations found in baseline which are part of MathTrig are now fixed and removed from baseline. There were about 20 of these.
* MathTrig - Change Names of funcWhatever to evaluate
Per discussions while MathTrig was being broken up, this would help standardize the code. That idea was adopted partway through the breakup. This PR applies that standardization to the earlier efforts. A similar effort is required for DateTime; that will come later. This PR replaces #2006.
The only 2 remaining funcWhatevers in MathTrig are both in SUM, which required two different methods depending on whether or not string parameters were to be ignored. It seems appropriate to leave those method names non-standardized in order to require a decision about which is to be used if they are invoked internally.
3 Phpstan grandfathered errors were eliminated as part of this change, and its baseline has changed accordingly.
Co-authored-by: Mark Baker <mark@lange.demon.co.uk>
* Improved Support for INDIRECT, ROW, and COLUMN Functions
This should address issues #1913 and #1993. INDIRECT had heretofore not supported an optional parameter intended to support addresses in R1C1 format which was introduced with Excel 2010. It also had not supported the use of defined names as an argument. This PR is a replacement for #1995, which is currently in draft status and which I will close in a day or two.
The ROW and COLUMN functions also should support defined names. I have added that, and test cases, with the latest push. ROWS and COLUMNS already supported it correctly, but there had been no test cases. Because ROW and COLUMN can return arrays, and PhpSpreadsheet does not support dynamic arrays, I left the existing direct-call tests unchanged to demonstrate those capabilities.
The unit tests for INDIRECT had used mocking, and were sorely lacking (tested only error conditions). They have been replaced with normal, and hopefully adequate, tests. This includes testing globally defined names, as well as locally defined names, both in and out of scope.
The test case in 1913 was too complicated for me to add as a unit test. The main impediments to it are now removed, and its complex situation will, I hope, be corrected with this fix.
INDIRECT can also support a reference of the form Sheetname!localName when localName on its own would be out of scope. That functionality is added. It is also added, in theory, for ROW and COLUMN, however such a construction is rejected by the Calculation engine before passing control to ROW or COLUMN. It might be possible to change the engine to allow this, and I may want to look into that later, but it seems much too risky, and not nearly useful enough, to attempt to address that as part of this change.
Several unusual test cases (leading equals sign, not-quite-as-expected name definition in file, complex indirection involving concatenation and a dropdown list) were suggested by @MarkBaker and are included in this request.
Openpyxl can generate the xml tag `<patternFill/>`, possibly even as a default style. Excel has no problem with this, treating it as "fill none", but PhpSpreadsheet has a glitch because it treats it as "fill solid white". So, when PhpSpreadsheet loads and saves such a file, the result at first appears as if gridlines are disabled; in fact, the gridlines are merely invisible behind the cells with their solid white fill. This PR makes PhpSpreadsheet behave the same as Excel in this circumstance.
Co-authored-by: Mark Baker <mark@lange.demon.co.uk>
`Worksheet::getCell()` used to optionnaly return null if passed a
second argument. This second argument was removed entirely and the
method always returns a Cell (possibly creating it if needed).
This make the API more predictable and easier to do static analysis
with tools such as PHPStan.
If you relied on that second parameter, you should instead use the
`Worksheet::cellExists()` before calling `getCell()`.
* More Financial function extracts, this time looking at the Periodic Cashflow functions
* Initial extract of Constant Periodic Interest and Payment functions
* Continue MathTrig Breakup - Completion!
Continuing the process of breaking MathTrip.php up into smaller classes. This round takes care of everything that was left:
- ABS
- DEGREES
- EXP
- RADIANS
- SQRT
- SQRTPI
- SUMSQ, SUMX2MY2, SUMX2PY2, SUMXMY2
The only notable logic change was that the 3 SUMX* functions had accepted arrays of unlike length; in that condition, they now return N/A, as Excel does. There had been no tests for this condition.
All the functions in MathTrig.php are now deprecated. Except for COMBIN, the test suite executes them only from MathTrig MovedFunctionsTest. COMBIN is still directly called by some Statistics Binomial functions which have not yet had the opportunity to be re-coded for the new location.
Co-authored-by: Mark Baker <mark@lange.demon.co.uk>
* Let's start with some appeasements to phpstan, just to reduce the baseline
* Appeasements to phpstan, taking the number of reported errors down to just 61
* Extract Normal and Standard Normal Distributions from the Statistical Class
* Extract ZTest from the Statistical Class, and move it to the Standard Normal Distribution class
Additional unit tests for NORMINV()
* Extract LogNormal distribution functions from Statistical
* Continue MathTrig Breakup - Penultimate?
Continuing the process of breaking MathTrip.php up into smaller classes. This round takes care of about half of what is left, so perhaps one round after this one will finish the job:
- ARABIC
- COMBIN; also implemented COMBINA
- FACTDOUBLE
- GCD (which accepts and ignores empty cells as arguments, but returns VALUE if all the arguments are that way; LCM does the same)
- LOG_BASE, LOG10, LN
- implemented MUNIT
- MOD
- POWER
- RAND, RANDBETWEEN (RANDARRAY is too complicated to implement with this ticket)
As you can see from the description, there are some functions which were combined in a single class. When not combined, I adopted PowerKiki's suggestion of using "execute" as the function name.
Co-authored-by: Mark Baker <mark@lange.demon.co.uk>
* Extract a few more Distribution functions from Statistical; this time EXPONDIST() and HYPGEOMDIST()
* Extract the F Distribution (although only F.DIST() is implemented so far
* Updae docblocks
* PHPCS
* Extract Binomial Distribution functions from Statistical
Replace the old MS algorithm for CRITBINOM() (which has now been replaced with te BINOM.INV() function) with a brute force approach - I'll look to refine it later. The MS algorithm is no longer documented, and the implementation produced erroneous results anyway
* Exract the NEGBINOMDIST() function as well; still need to add a cumulative flag to support the additional argument for the newer NEGBINOM.DIST() function
* Rationalise validation of probability arguments
* Difference in variance calculations between Excel/Gnumeric and Open/LibreOffice
* Simplify STDEV() function logic by remembering that STDEV() is simply the square root of VAR(), so we can simply use the VAR() calculaion rather than duplicating the basic logic... and also allow for the differences between Excel/Gnumeric and Open/LibreOffice
* Start implementing Newton-Raphson for the inverse of Statistical Distributions, starting with the two-tailed Student-T
* Additional unit tests and validations
* Use the new Newton Raphson class for calculating the Inverse of ChiSquared
* Extract Weibull distribution, and provide unit tests
* Extract ACCRINT() and ACCRINTM() Financial functions into their own class
Implement additional validations, with additional unit tests
Add support for the new calculation method argument for ACCRINT()
* Additional tests for Amortization functions
Continuing the process of breaking MathTrip.php up into smaller classes. This round takes care of all functions which might be an impediment to installing due to either uncovered code or "complexity":
- BASE
- FACT
- LCM
- MDETERM, MINVERSE, MMULT
- MULTINOMIAL
- PRODUCT
- QUOTIENT
- SERIESSUM
- SUM
- SUMPRODUCT
MathTrig and the members in directory MathTrig are now 100% covered. Many tests have been added, and some edge-case bugs are corrected. Some cases where PhpSpreadsheet had rejected numeric values stored as strings have been changed to accept them whenever Excel does; there had been no tests for that condition.
Boolean arguments are now accepted as arguments wherever Excel accpets them. Taking a cue from what has been done in Engineering, the parameter validation now happens in a routine which issues Exceptions for invalid values; this simplifies the code in the functions themselves. Thank you for doing that; I did not foresee how useful that was when I first looked at it.
Consistent with earlier changes of this nature, the versions in the MathTrig class remain, with a doc block indicating deprecation, and a stub call to the new routines.
All tests except for MINVERSE and MMULT are now handled in the context of a spreadsheet rather than a direct call to the calculation function which implements it. PhpSpreadsheet would need to handle dynamic arrays in order to test MINVERSE and MMULT in a spreadsheet context. Implementing that looks like it might be *very* challenging. It is not something I plan to look at, at least not in the near future.
One parsing problem turned up in the test conversion. It is in one of the SUMIF tests. It takes me to an area in Calculation where the comment says "I don't even want to know what you did to get here". It did not show up in the previous incarnation because, by using a direct call, the previous test managed to bypass the parsing. I have confirmed that this problem shows up in earlier releases of PhpSpreadsheet, so the changes in this PR did not cause it - they merely exposed it. I have left the test intact, but marked it "incomplete" for documentation purposes. I have not been able to get a handle on what's going wrong yet. I will probably open an issue on it if I can't resolve it soon. However, the test in question isn't a "real world" issue, and the error wasn't caused by this change, so I see no reason to delay this pending a resolution of the problem.
SUM had an idiosyncratic moment of its own. It had been ignoring non-numeric values, but Excel returns VALUE in that situation. So I changed it and wrote some new tests, which worked, but ... SUMIF uses several levels of indirection to get to SUM, and SUMIF *does* ignore non-numeric values, so a SUMIF test broke. SUM is a really simple function; the most practical approach seemed to be to clone it, with the string-accepting version being used by the Legacy version (which is called by SUMIF), and the non-string-accepting version being used in the Calculation Function table. That seems far easier and more practical than, for instance, adding a boolean parameter to the variable parameter list. As a follow-up, I will change SUMIF to explicitly call the appropriate new version, but I did not want to add that to this already large change.
SUM again - although it was fully covered beforehand, there was not a specific test member for it. There is now.
FACT had been coded to fail Gnumeric requests where the numeric argument has a decimal portion. However, Gnumeric does accept such an argument, and, unlike Excel and ODS, does not truncate it, but returns the result of a Gamma function call instead. This has been corrected.
When LCM included arguments which contained both 0 and a negative number, it returned 0 or NUM, whichever it found first. It is changed to always return NUM in that circumstance, as Excel does.
QUOTIENT had been documented as taking a variadic list of arguments. In fact, it takes exactly 2 - numerator and denominator - and the docblock and signature is fixed, even in the deprecated version.
The SERIESSUM docbock and signature are more accurate, even in the deprecated version. It is changed to ignore nulls, as Excel does, rather than return VALUE, and is one of the routines which previously rejected numbers in string form.
SUBTOTAL tests had used mocking for some reason. These are replaced with normal tests. And SUBTOTAL had a big surprise in store. That part of it which deals with hidden cells cares only whether the row is hidden, and doesn't care about the column's visibility.
I struggled with whether it should be SubTotal or Subtotal. I think the latter is correct, so that's how I proceeded. I don't think there are likely to be any other capitalization controversies.
* New Bessel Algorithm, providing a higher degree of precision (12 decimal places) and still matching/exceeding MS Excel's precision across the range of values
* Complete Breakup Of Calculation/DateTime Functions
In conjunction with parallel breakups happening in other areas of Calculation, this change breaks up all the DateTime functions into their own classes. All methods remaining in DateTime itself have a doc block deprecation notice, and consist only of stub code to call the replacement methods. Coverage of DateTime itself and all the replacement methods is 100%.
There is only one substantive change to the code (see next paragraph). Among the non-substantive changes, it now adopts the same parsing technique (throwing and catching exceptions) already in use in Engineering and MathTrig. Boolean parameters are allowed in lieu of numbers when Excel allows them. Most of the code changes involve refactoring due to the need to avoid Scrutinizer "complexity" failures in what it will consider to be new methods.
Issue #1936 was opened just as I was staging this. It is now fixed. One existing WORKDAY test was wrong (noted in a comment in the test data file), and a bunch of new tests are added.
I found it confusing to use DateTime as a node of the the class name since most of the methods invoke native DateTime methods. So, everything is moved to directory DateTimeExcel, and that is what is used in the class names.
There are several follow-up activities that I am planning to undertake if this PR is merged.
- ODS supports dates well before 1900. There are exactly 2 assertions for this functionality. More are needed (and some functions might have to change to accept this).
- WEEKDAY has some poorly documented extra options for "style" which are not yet implemented.
- Most tests have been changed to use a formula as entered on a spreadsheet rather than a direct call to the method which implements the formula. There are 3 exceptions at this time. WORKDAY and NETWORKDAYS, which include arrays as part of their parameters, are more complicated than most. YEARFRAC was just too large to deal with now.
- There are direct calls to the now-deprecated methods in both source code and tests, mostly in Financial code, but possibly in others as well. These need to be changed.
- Some constants, none "officially" documented, remain in the original class. These should be either deleted or marked deprecated. I wasn't sure if deprecation was even possible (or desirable), and did not want that to be something which would cause Scrutinizer to fail the change.
* Deprecate Now-unused Constants, Fix Yearfrac bug, Change 3 Tests
Add new DateTime/Constants class, initially populated with constants used in Weeknum.
MS has another inconsistency with how it handles null cells in Yearfrac. Change PhpSpreadsheet to behave compatibly with this bug.
I have modified YearFrac, WorkDay, and NetworkDays tests to be more to my liking. Many tests added to YearFrac because of the bug above. Only minor modifications to the existing tests for the others.
* Start work on breaking down some of the Financial Excel functions
* Unhappy path unit tests for Treasury Bill functions
* Codebase for Treasury Bills includes logic for a different days between settlement and maturity calculation for OpenOffice; but Open/Libre Office now uses the Excel days calculation, so this discrepancy between packages is no longer required
* We've already converted the Settlement and Maturity dates to Excel timestamps, so there's no need to try doing it again when calculating the days between Settlement and Maturity
* Add Unit Tests for the Days per Year helper function
* Extract Interest Rate functions - EFFECT() and NOMINAL() - with additional validation, and unhappy path unit tests
* First pass at extracting the Coupon Excel functions
* Simplify the validation methods
* Extended unit tests to cover all combinations of frequency and basis, including leap years
Fix for COUPDAYSNC() when basis is US 360 and settlement date is the last day of the month
* Ensure that all Financial function code uses the new Helpers class for Days Per Year
* jpgraph seems to be finally dying with PHP. Until we have a valid alternative, disabling this run for PHP because it errors
https://github.com/HuasoFoundries/jpgraph looks like a natural successor, but it isn't BC so it will require some work to integrate
* Bitwise Functions and 32-bit
When running the test suite with 32-bit PHP, a failure was reported in BITLSHIFT.
In fact, all of the following are vulnerable to problems, and didn't report
any failures only because of a scarcity of tests:
- BITAND
- BITOR
- BITXOR
- BITRSHIFT
- BITLSHIFT
Those last 2 can be resolved fairly easily by using multiplication by a power of 2
rather than shifting. The first 3 are a tougher nut to crack, and I will continue
to think how they might best be approached. For now, I have added skippable tests
for each of them, which at least documents the problem.
Aside from adding many new tests, some bugs were correctd:
- The function list in Calculation.php pointed BITXOR to BITOR.
- All 5 functions allow null/false/true parameters.
- BIT*SHIFT shift amount must be numeric, can be negative, allows decimal portion
(which is truncated to integer), and has an absolute value limit of 53.
- Because BITRSHIFT allows negative shift amount, its result can overflow
(in which case return NAN).
- All 5 functions disallow negative parameters (except ...SHIFT second parameter).
This was coded, but the code had been thwarted by an earlier is_int test.
* Full Support for AND/OR/XOR on 32-bit
Previous version did not support operands 2**32 through 2**48.
* Improve Coverage of BIN2DEC etc.
The following functions have some special handling
depending on the Calculation mode:
- BIN2DEC
- BIN2HEX
- BIN2OCT
- DEC2BIN
- DEC2HEX
- DEC2OCT
- HEX2BIN
- HEX2DEC
- HEX2OCT
- OCT2BIN
- OCT2DEC
- OCT2HEX
Ods accepts boolean for its numeric argument.
This had already been coded, but there were no tests for it.
Gnumeric allows the use of non-integer argument where Excel/Ods do not.
The existing code allowed this for certain functions but not for others.
Gnumeric consistently allows it, so there is no need for parameter
gnumericCheck in convertBase::ValidateValue.
Again, there were no tests for this.
There were some minor changes needed:
- In functions where you are allowed to specify the numnber of "places" in the
result, there is an upper bound of 10 which had not been enforced.
- Negative values were not handled correctly in some cases.
- There was at least one (avoidable) error on a 32-bit system.
- Some upper and lower bounds were not being enforced. In addition to enforcing
those, the bounds are now defined as class constants in ConvertDecimal.
Many tests have been added, so that Engineering is now almost 100% covered.
The exception is some BESSEL code. There have been some recent changes to
BESSEL which are not yet part of my fork, so I could not address those now.
However, I freely admit that, when I looked at the uncovered portion, it seemed
like it might be a difficult task, so I probably wouldn't have tackled it anyhow.
In particular, the uncovered code seemed to deal with very large numbers,
and, although PhpSpreadsheet and Excel both give very large results for these
conditions, their answers are not particularly close to each other. I think
we're dealing with resuts approaching infinity. More study is needed.
* Coverage for Helper/Samples
I was perplexed by the fact that Helper/Samples seemed to be entirely uncovered when running the test suite, since I know all the samples are run as part of the test. I think that what must be happening is that the Helper code is invoked mostly as part of a Data Provider (and therefore not counted), not as part of the test proper (which would count). So, this change adds a small number of tests which result in Samples being 100% covered.
Covering one statement was tricky - simulating the inability to create a test directory. Mocking, a technique I have not used before, solves this problem admirably.
* Suggestions From Mark Baker
Tests changed from assertEquals to assertSame.
Added @covers annotation to test class.
Validate parameter for method being mocked.
* First step extracting INDIRECT() and OFFSET() to their own classes
* Start building unit tests for OFFSET() and INDEX()
* Named ranges should be handled by the Calculation Engine, not by the implementation of the Excel INDIRECT() function
* When calling the calculation engine to get the range of cells to return, INDIRECT() and OFFSET() should use the instance of the calculation engine for the current workbook to benefit from cached results in that range
There's a couple of minor bugfixes in here; but it's basically just refactoring of the INDIRECT() and OFFSET() Excel functions into their own classes - still needs a lot of work on unit testing; and there's a lot more that could be improved in the code itself (including handling of the a1 flag for R1C1 format in INDIRECT()
* Continue MathTrig Breakup - Trig Functions
Continuing the process of breaking MathTrip.php up into smaller classes.
This round takes care of the trig and hyperbolic functions, plus a few others.
- COS, COSH, ACOS, ACOSH
- COT, COTH, ACOT, ACOTH
- CSC, CSCH
- SEC, SECH
- SIN, SINH, ASIN, ASINH
- TAN, TANH, ATAN, ATANH, ATAN2
- EVEN
- ODD
- SIGN
There are no bug fixes in this PR, except that boolean arguments are now
accepted for all these functions, as they are for Excel.
Taking a cue from what has been done in Engineering, the parameter validation
now happens in a routine which issues Exceptions for invalid values;
this simplifies the code in the functions themselves.
Consistent with earlier changes of this nature, the versions in the
MathTrig class remain, with a doc block indicating deprecation,
and a stub call to the new routines.
I think several more iterations will be needed to break up MathTrig completely.
* Extract LookupRef\INDEX() into index() method of LookupRef\Matrix class
Additional tests
* Bugfix for returning a column using INDEX()
* Some improvements to ROW() and COLUMN()
* Simplify some of the INDEX() logic, eliminating redundant code
* Fix for Issue #1887 - Lose Track of Selected Cells After Save
Issue #1887 reports that selected cells are lost after saving Xlsx. Testing indicates that this applies to the object in memory, though not to the saved spreadsheet.
Xlsx writer tries to save calculated values for cells which contain formulas. Calculation::_calculateFormulaValue issues a getStyle call merely to retrieve the quotePrefix property, which, if set, indicates that the cell does not contain a formula even though it looks like one. A side-effect of calls to getStyle is that selectedCell is updated. That is clearly accidental, and highly undesirable, in this case. Code is changed to save selectedCell before getStyle call and restore it afterwards.
The problem was reported only for Xlsx save. To be on the safe side, test is made for output formats of Xlsx, Xls, Ods, Html (which basically includes Pdf), and Csv. For all of those, the object in memory is tested after the save. For Xlsx and Xls, the saved file is also tested. It does not make sense to test the saved file for Csv and Html. It does make sense to test it for Ods, but the necessary support is not yet present in either the Ods Reader or Ods Writer - a project for another day.
* Move Logic Out of Calculation, Add Support for Ods ActiveSheet and SelectedCells
Mark Baker thought logic belonged in Worksheet, not Calculation.
I couldn't get it to work in Worksheet, but doing it in Cell works,
and that has already been used to preserve ActiveSheet over call to
getCalculatedValue, so this just extends that idea to SelectedCells.
Original tests could not completely support Ods because of a lack of support
for ActiveSheet and SelectedCells in Ods Reader and Writer.
There's a lot missing in Ods support, but a journey of 1000 miles ...
Those two particular concepts are now supported for Ods.
* Start refactoring the Lookup and Reference functions
- COLUMN(), COLUMNS(), ROW() and ROWS()
- LOOKUP(), VLOOKUP() and HLOOKUP()
- Refactor TRANSPOSE() and ADDRESS() functions into their own classes
* Additional unit tests
- LOOKUP()
- TRANSPOSE()
- ADDRESS()
- Move TREND() functions into the Statistical Trends class
- Unit tests for TREND()
- Create Confidence class for Statistical Confidence functions, and the CONFIDENCE() method
I ran the test suite using 32-bit PHP. There were 2 places where changes
were needed due to 32-bit timestamps.
Reader\\Xml.php was using strtotime as an intermediate step in converting
a string timestamp to an Excel timestamp. The XML file type stores pure timestamps
(i.e. no date portion) as, e.g., 1899-12-31T02:30:00.000, and that value
causes an error using strtotime on a 32-bit system. However, it is sufficient
to use that value in a DateTime constructor, and that will work for 32- and 64-bit.
There was no test for that particular cell, so I added one to the XML read test.
And that's when I discovered the getFormattedValue bug. The cell's format
is `hh":"mm":"ss`. The quotes around the colons are disrupting the formatting.
PhpSpreadsheet formats the cell by converting the Excel format
to a Php Date format, in this case `H\:m\:s`.
That's a problem,
since Excel thinks 'm' means *minutes*, but PHP thinks it means *months*.
This is not a problem when the colon is not quoted; there are ample tests for that.
I added my best guess as to how to recognize this situation,
changing `\:m` to `:i`. The XML read test
now succeeds, and no other tests were broken by this change.
Test Shared\\DateTest had one test where the expected result of converting to a
Unix timestamp exceeds 2**32. Since a Unix timestamp is strictly an int,
that test fails on a 32-bit system. In the discussion regarding recently merged
PR #1870, it was felt that the user base might still be using the functions
that convert to and from a timestamp. So, we should not drop this test, but,
since it cannot succeed on a 32-bit system, I changed it to be skipped
whenever the expected result exceeded PHP_INT_MAX. There are 3 "toTimestamp"
functions within that test. Only one of these had been affected, but I thought
it was a good idea to add additional tests to the others to demonstrate this
condition.
In the course of testing, I also discovered some 32-bit problems with
bitwise and base-conversion functions. I am preparing separate PRs to
deal with those.
* Start splitting some of the basic Statistical functions out into separate classes containing just a few similar functions
* Splitting some of the basic Statistical functions out into separate classes containing just a few similar functions - MAX(), MAXA(), MIN() and MINA()
* Splitting some more of the basic Statistical functions out into separate classes containing just a few similar functions - StandardDeviations and Variances
* 100% Coverage for Calculation/DateTime
The code in DateTime is now completely covered.
Along the way, some errors were discovered and corrected.
- The tests which have had to be changed at the start of every year are
replaced by more robust equivalents which do not require annual changes.
- Several places in the code where Gnumeric and OpenOffice were thought to differ
from Excel do not appear to have had any justification.
I have left a comment where such code has been removed.
- Use DateTime when possible rather than date, time, or strftime functions to avoid
potential Y2038 problems.
- Some impossible code has been removed, replaced by an explanatory comment.
- NETWORKDAYS had a bug when the start date was Sunday. There had been no tests
of this condition.
- Some functions allow boolean and null arguments where a number is expected.
This is more complicated than the equivalent situations in MathTrig because
the initial date for these calculations can be Day 1 rather than Day 0.
- More testing for dates from 1900-01-01 through the fictitious
everywhere-but-Excel 1900-01-29.
- This showed that there is an additional Excel bug - Excel evaluates
WEEKNUM(emptycell) as 0, which is not a valid result for
WEEKNUM without a second argument.
PhpSpreadsheet now duplicates this bug.
- There is a similar and even worse bug for 1904-01-01 in 1904 calculations.
Weeknum returns 0 for this,
but returns the correct value for arguments of 0 or null.
- DATEVALUE should accept 1900-02-29 (sigh) and relatives.
PhpSpreadsheet now duplicates this bug.
- Testing bootstrap sets default timezone. This appears to be a relic from
the releases of PHP where the unwise decision, subsequenly reversed,
was made to issue messages for
"no default timezone is set" rather than just use a sensible default.
This was a disruptive setting for some of the tests I added.
There is only one test in the entire suite which is default-timezone-dependent.
Setting and resetting of default timezone is moved to that test
(Reader/ODS/ODSTest), and out of bootstrap.
- There had been no testing of NOW() function.
- DATEVALUE test had no tests for 1904 calendar and needs some.
- DATE test changed 1900/1904 calendar in use without restoring it.
- WEEKDAY test had no tests for 1904 calendar and needs some.
- Which revealed a bug in Shared/Date (excelToDateTimeObject was not
recognizing 1904-01-01 as valid when 1904 calendar is in use).
- And an additional bug in that legal 1904-calendar values in the 0.0-1.0
range yielded the same "wrong" answers as 1900-calendar (see "One note" below).
Also the comment for one of the calendar-1904 tests was wrong in attempting
to identify what time of day the fraction represented.
I had wanted to break this up into a set of smaller modules, a process already
started for Engineering and MathTrig.
However the number of source code changes was sufficient that I wanted
a clean delta for this request.
If it is merged, I will work on breaking it up afterwards.
One note - Shared/Date/excelToDateTimeObject, when calendar-1900 is in use,
returns an unexpected result if its argument is between 0 and 1,
which is nominally invalid for that calendar.
It uses a base-1970 calendar in that instance. That check is not justifiable
for calendar-1904, where values in that range are legal,
so I made the check specific to calendar-1900,
and adjusted 3 1904 unit test results accordingly. However, I have to admit that
I don't understand why that check should be made even for calendar-1900.
It certainly doesn't match anything that Excel does.
I would recommend scrapping that code altogether.
If agreed, I would do this as part of the break-up into smaller modules.
Another note -
more controversially, it is clear that PhpSpreadsheet needs to support
the Excel and PHP date formats. Although it requires further study,
I am not convinced that it needs to support Unix timestamp format.
Since that is a potential source of Y2038 problems on 32-bit systems,
I would like to open a PR to deprecate the use of that format.
Please let me know if you are aware of a valid reason to continue to support it.
oleibman
Mark Baker
Matjaž Drolc
Nathan Dench
xandros15
Adrien Crivelli
Tiago Fernandes
Tiago Fernandes
Akira Taniguchi
Vivek
Patrick Brouwers