* Inconsistency Between Actual and Declared Type - Minor Break
Fix#3711. User set a cell value to float (implicitly by default value binder), then used `setDataType` to change its type to string. This caused a problem for Xlsx Writer, which uses the string cell values as an index into a Shared String array. However, as the cell actually contained a floating point value, Php treated it as an integer index; such a treatment is both deprecated, and leads to invalid values in the spreadsheet.
The use case for `setDataType` is not strong. The user always has the option to use `setValueExplicit` if the type is important. Setting a type afterwards, i.e. irrespective of the value, seems like a peculiar action. Indeed, there are no tests whatever for such use in the unit test suite.
There are two possible approaches to fixing this problem. The first is to add casts to the 3 or 4 places in Writer Xlsx which might be affected by this problem (hoping that you've found them all and realizing that similar changes might be needed for other Writers). The second is to change `setDataType` to call `setValueExplicit` using the current value of the cell, thereby possibly changing the cell value. I have gone with the second option - it seems like a much more logical approach, and guarantees that the content of the cell will always be consistent with its declared type. It is, however, a breaking change; if, for example, you have a cell with a string or numeric value and specify `boolean` to `setDataType`, the cell's value will change to `true` or `false` with no way to get back to the original.
* Strict Types for New Tests
Consistent with work being done in PR #3718.
* Improve Test
Better match to original issue.
* Typo
* ListWorksheetInfo/Names for Html/Csv/Slk
Fix#3706. ListWorksheetInfo is implemented for all Readers except Html. For most (not all), ListWorksheetInfo is more efficient than reading the spreadsheet. I can't think of a way to make that so for Html, but that shouldn't be a reason to leave it unimplemented.
ListWorksheetNames is not implemented for Html, Csv, or Slk. It isn't terribly useful for those formats, but that isn't a reason to omit it. The requester's use case consists of using IOFactory to create a reader for a file of unknown format and determining the first sheet name. That seems legitimate, but it is currently not possible without extra user code if the file is Html, Csv, or Slk; this PR will make it possible.
When Excel opens a Slk or Csv file, the sheet name is based on the file name. PhpSpreadsheet does this for Slk, but it uses a default name for Csv. I am not interested in creating a break for that behavior, but I have added a new boolean property `sheetNameIsFileName` with a setter to Csv Reader. The requester actually mentioned that possibility in our discussion, although it is not essential to the request.
As an adjunct to the issue, the requester wishes to use the worksheet name in `setLoadSheetsOnly`. That is already possible for Html, Csv, and Slk, but that particular property is ignored for those formats. I do not see a reason to change that behavior. This treatment is now explicitly noted in the documentation for property `loadSheetsOnly`.
There had been no tests for what happens when `loadSheetsOnly` is specified but no sheets match the criteria for the formats for which this makes sense (Xlsx, Xls, Ods, Gnumeric, Xml). The behavior was not consistent - some formats threw an Exception while others continued with a single empty worksheet. All cases attempt to set the active sheet, and they will now all throw identical Exceptions when they attempt to do so in this situation. Tests are added for each.
There also had been no tests for `loadSheetsOnly` returning more than one sheet. One is added.
* Update LoadSheetsOnlyTest.php
Add strict types to this new test, consistent with work being done in PR #3718.
* Update LoadSheetsOnlyTest.php
Add strict types to this new test, consistent with work being done in PR #3718.
While we might never be able to have 100% of our code strict, we can at
the very least do it for all of our tests. This ensures that our tests
are using our API with the types as intended by the test author, and not
silently be cast to what our API requires.
And quite a bit more manual changes. The idea is that typing of our
tests can be a bit more loose, so we assume PHPDoc is mostly correct. If
that happens to be wrong, it should be caught by the tests themselves.
This is according to our formal, published, policy to only support
EOL PHP after 6 months.
See https://phpspreadsheet.readthedocs.io/en/latest/#php-version-support
Also share the exact same dev deps across all PHP version for GitHub
Actions so runs are faster, much most importantly they are stable and
predictable. And we decide manually when we want to migrate to PHPUnit
10.
Fixes#3634Closes#3710
There is a circular reference between Worksheet and Table, which can lead to a memory leak. I tried to break this connection in the Worksheet destructor earlier, but this seemed to cause some sort of error with Phpunit 10 on Github, an error that I am unable to duplicate in my local environments. This PR will allow me to explore the problem to see if I can come up with any useful diagnostic data. If I get to a combination that works, I will consider merging it.
* Use Column Style When Row Dimension Exists Without Style
Fix#3534. For new cells, style is set to Row Dimension Style when Row Dimension exists (even if Row Dimension Style does not exist), else Column Dimension Style when Column Dimension exists. However it should be set to Row Dimension Style when Row Dimension exists *and* Row Dimension Style exists, else ...
* Address Issue 3533
If cell is defined in xml, it uses neither row nor column style. If style is omitted in cell definition, default style is used.
* Update CHANGELOG.md
* Avoid Allocating RowDimension Unneccesarily
This PR builds on PR #3527, introduced but subsequently closed by @goetas. The observation was that AutoFilter did not need to allocate a new RowDimension when the row was not to be filtered. While vetting the PR, it became apparent that Xlsx Writer also allocates RowDimension unnecessarily, with some minor adjustments possible for Ods and Mpdf as well. My tests confirm the initial observation that there can be a considerable memory savings when RowDimension is allocated only when needed. So, even though the original PR is withdrawn, there seems to be value in proceeding with it anyhow.
* Add Some Tests
Fix#642. Opened over 5 years ago, probably the oldest problem I've worked on. And https://github.com/PHPOffice/PHPExcel/issues/1320, opened a year before that. And https://github.com/SpartnerNL/Laravel-Excel/issues/1521.
Shared/StringHelper::UTF8toBIFF8UnicodeLong calculates incorrect length for strings when they contain characters outside Unicode BMP. Xls uses UTF-16 to encode its strings, and characters outside BMP require a surrogate pair to encode. PhpSpreadsheet (and PhpExcel before it) have been counting these as a single character, but Excel counts them as 2. Change to compute the length as half the number of bytes in the UTF-16 string, as Excel does.
A formal test is added, but it's a bit difficult to follow. So I aso added a non-BMP emoji to 27template.xls, which will cause it to be both read by Xls reader and written by Xls writer. This would previously have created a corrupt worksheet. The emoji is now handled correctly.
* Correct Re-computation of Relative Addresses in Defined Names
Fix#3661. Insertion or deletion of rows or columns can cause changes to the ranges for Defined Names. In fact, only the absolute parts of such ranges should be adjusted, while the relative parts should be left alone. Otherwise, as the original issue documents, the adjustment to the relative portion winds up being double-counted when the Defined Name is referenced in a formula. The major part of this change is to ReferenceHelper and CellReferenceHelper to not adjust relative addresses for Defined Names. An additional small change is needed in the Calculation engine to `recursiveCalculationCell` when a Defined Formula is being calculated.
In a sense, this is a breaking change, but for an obscure use case which (a) was wrong, and (b) is unlikely to be of importance. Some of the tests in ReferenceHelperTest were wrong and are now corrected, with the results being cross-checked against Excel.
When a Defined Name using relative addressing is defined in Excel, the result is treated as relative to the active cell on the sheet in which the name is defined. PhpSpreadsheet treats it as relative to cell A1. I think that is a reasonable treatment, and will not change its behavior to match Excel's - that would definitely be a breaking change of some consequence.
An interesting use of relative address in a defined name is demonstrated at https://excelguru.ca/always-refer-to-the-cell-above/. Note that the steps there involve setting the selected cell to A2 before defining the name. When that spreadsheet is stored, the actual definition of the range is `A1048576`. Likewise, adding a defined name for the cell to the left would be stored as `XFD1`. This seems a little fragile, but Ods, which I believe does not have the same row and column limits as Excel, certainly treats these values the same as Excel. This particular construction is formally unit-tested. Note, however, that although using these Defined Names as a formula on their own works just fine, a construction like `=SUM(A1:CellAbove)`, as suggested in the article, seems to put PhpSpreadsheet calculation engine in a loop. In the likely event that I can't solve that before I merge this change, I will open a new issue to that effect when I do merge it. Note that this can be handled without defined names as `=SUM(A$1:INDIRECT(ADDRESS(ROW()-1,COLUMN())))`. PhpSpreadsheet will handle this as a cell formula, but not yet as a Named Formula.
The tests show a breakdown evaluating `=ProductTotal` (product of 2 formulas using defined names with relative addresses) on the sheet on which it is defined, but it works from a different sheet. The usual debugging techniques show me why this is happening, but I can't see how to overcome it. As above, if I can't solve it before I merge, I will open a new issue.
For those situations where I intend to open a new issue, tests are added but are marked Incomplete. Because of those, I will leave this PR in draft status for 2 weeks before moving forward with it.
* Fix productTotal Problem
Need to restore current cell after evaluating defined name.
* Fix Several Problems in a Very Complicated Spreadsheet
Fix#3679. That issue was opened for a problem which was already solved by PR #3659, however there were additional problems with the spreadsheet.
The main problem is that Data Validations and Conditional Styles can each be supplied in the Xml in either "external" or "internal" formats. The code for each to handle "external" assumes that each is the only "external" item on the worksheet in the Xml, but some of the worksheets in the sample spreadsheet provide both as "external" on some sheets. The code to fix this is verified against the supplied sample, however no formal test has been added for it. The sample is much too large and complicated to be added to the test suite - it takes several minutes to read, and even longer to write (`setPreCalculateFormulas(false)` is highly recommended). I will leave this ticket open for a few days to see if I can hand-craft a suitable test case, but I am not hopeful.
A second problem is that something in the Xlsx Reader `$xmlSheetNS->sheetData->row` loop breaks the selected cell for the worksheet. This is easily fixed and verified by eye (and with the supplied sample), but, again, no explicit test case is added.
A third problem is that drawings which are part of the supplied sample use `srcRect` tags in the Xml to effectively produce a cropped version of the image. This tag has hitherto been ignored. It is now supported in Xlsx Reader, Xlsx Writer, and Worksheet/BaseDrawing object. This is again verified with the supplied sample; unlike the other parts, it was easy to add a new formal test case for this part of the fix.
* Scrutinizer False Positives
Fix#3658. Readers will call `setCalculatedValue` to save the original calculated value for a cell that contains a formula; this affects the result of `getOldCalculatedValue`. The set routine automatically casts numeric values to float, which is not appropriate when the calculated value is explicitly set to a string type. An optional parameter is added to the set routine to indicate whether a cast is appropriate (default is true so userland uses of this method will remain unaffected). The readers for Xlsx, Xls, Xml, Ods, and Slk are all changed to set the new parameter appropriately. Gnumeric saves the formula, but does not appear to save the result, so no change is needed there. Html and Csv save the result but not the formula (for preCalculateFormulas true - they save the formula but not the result for false), so no change is needed for them.
It is not clear what the use case is for oldCalculatedValue, but it does date back to PhpExcel. A possible use case is demonstrated in the new tests - Excel function INFO is not implemented, so getCalculatedValue will generally return null on such a cell, but will return oldCalculatedValue when it sees that the function is not implemented but oldCalculatedValue has been set (typically by loading an existing spreadsheet), so the result that is on the spreadsheet will be available to the program which loaded it.
Reading a spreadsheet which has been created with preCalculateFormulas false will, or at least should, result in a null oldCalculatedValue. This was true for Xls, Xml, and Slk. Xlsx was actually storing 0 in this situation; that wasn't precisely a problem, but it wasn't necessary and seems misleading. Xlsx Writer is changed to no longer set a value in this situation. This is technically a break, but the existing code was wrong; one unit test needed a very minor modification as a result of this change.
Adrien Crivelli
oleibman
dependabot[bot]