* 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
* Eliminate Some Scrutinizer 'Major' Problems Part 3
Continuing the work of PR #3109 and PR #3122. Nothing unusual about the changes in this ticket. I expect 4 or 5 more after it will be needed to finish the job.
* Scrutinzer Strikes Again
A couple of new false positives.
* Tidy Up Earlier Change
Conform to a better approach we figured out later.
* Eliminate Some Scrutinizer 'Major' Problems Part 2
Mostly Scrutinizer bizarre false positives. From Calculation/Engineering/Bessell (and several others):
```php
private static function besselI2(float $x, int $ord): float
{
if ($x === 0.0) {
return 0.0;
}
```
Scrutinizer complains that `$x` can never equal 0.0 here. Huh???
Another example repeated several times, from Calculation/Engineering/BesselJ:
```php
$bool = false;
foreach (whatever) {
...
$bool = !$bool;
...
}
```
Scrutinizer complains about the `!$bool` assignment because it says `$bool` is always false. Again, huh???
* Change Logical Not Handling
From a suggestion from @MarkBaker, sidestep bug in Scrutinizer by recoding
```php
$jsum = !$jsum
```
as:
```php
$jsum = $jsum === false;
```
* Permit Date/Time Entered on Spreadsheet to be Calculated as Float
Fix#1416. I do not entirely understand the use case for this old issue, but resolving it seems straightforward. Issue complains that user-entered date/time fields may be interpreted as either float or int when PhpSpreadsheet reads them. Issue suggests getCalculatedValue treat all date/time fields as float; that seems like a breaking change. However, adding an option to permit it seems okay. That option might be implemented as either a property of Calculation, or a static property of Cell. Since the changed logic is found in Cell (and Shared/Date), I opted for the latter.
In Cell, the property `$parent` is incorrectly described in doc block as `Cells`, and should be `?Cells`. This change eliminates some Phpstan and Scrutinizer problems, and should allow the elimination of some try/catch blocks - I have not done an exhaustive search for those.
Calls to `isDateTime` could have affected activeSheet and selectedCells; they no longer can. Optional parameters are added to it and the functions it calls to accommodate the new functionality; the defaults for the new parameters will, of course, return the same result as the earlier versions of the functions would have returned.
* Scrutinizer - Self-inflicted
Tests used constant which I deprecated.
When adding support for non-standard namespacing to Reader Xlsx, I changed most (hopefully all) the uses of string literals for the namespaces to class constants instead. Writer Xlsx naturally uses all the same namespaces, but has continued to use string literals. This PR replaces those with the same constants used by Reader Xlsx.
Almost all of these are handled through annotations. This shouldn't be our "go-to" solution, but it becomes necessary because Scrutinizer's analysis is often incorrect. Here is a typical example, from Cells.php.
```php
if ($this->currentCellIsDirty && isset($this->currentCoordinate, $this->currentCell)) {
$this->currentCell->detach();
```
Scrutinizer complains that `$this->currentCell` can be null here, but the `isset` condition guarantees that it must be non-null. Perhaps Scrutinizer is worried that `isset` might be overridden, and will accept only an explicit equality test for null for each of the isset arguments. Changing the code to do this seems riskier than just adding the annotation.
A far more common, and more frustrating, example is:
```php
foreach ($simpleXmlElement as $element) {
var_dump($element->method());
}
```
Scrutinizer complains that element might be null. I don't think it can. I have previously added code in places to eliminate the objection, and that may be a practical solution when `$element` is used many times in the loop. But, when it's used only once, annotating the objection away seems like a better solution (less overhead, clearer code). Many of the changes in this PR fall into this category.
* Fix Remainder of Calculation vs. Phpstan Issues
I had tried to include these changes as part of an earlier effort, but something about them broke Phpstan. I removed them until I could determine the actual cause, which is ...
The array `$phpSpreadsheetFunctions` is a very large and very complicated array, typehinted as `array`. It is declared as private static; however, its content never changes (at least not now - there are some outstanding proposals that might change that). I have had some success with changing unmodifiable private static to private const. However, such a change here causes Phpstan to perform a lot more processing and eventually time out. So, leave it as static.
Having identified the cause of the problem, none of the other changes were problematic, so this PR applies the rest of them.
* Scrutinizer
One problem.
* Scrutinizer False Positives Without Suggested Annotation
See if minor code changes can make these go away.
* Scrutinizer Still Experimenting
Trying again.
Fix#3095. Issue is not a problem for Php8, but is for 7.4. Code used to write Xml for float custom property uses locale-dependent (in Php7) cast. Change to use locale-independent (all Php releases) `sprintf('%F'...)` instead.
Fix#3093. PR #2595 added the ability for Xlsx Reader to accept a custom color palette. With no examples other than the one at hand, which included a full complement of 64 colors, that PR required 64 colors in the palette. It turns out that Mac Numbers exports to Excel with a palette with less than 64 colors. So, with an example of that at hand, relax the original restriction and accept a palette of any size.
Fix#1531. This is a replacement for PR #3081 (see last paragraph below), which I will close.
Calculation has a property `suppressFormulaErrors`, which really doesn't work as one might expect. If a calculation throws an exception, the setting of this property might prevent the Exception from being thrown, but it will still trigger an Error. I do not think this makes sense, and will change it so the calculation will return `false`, which is part of the original design but which would essentially never happen. This allows the user to save a corrupt spreadsheet, but this was already possible through the use of `setPreCalculateFormulas(false)` on the Writer, so this doesn't really open any new exposures. It nevertheless might be considered a breaking change because of the difference in behavior.
Deprecation - the visibility of the existing property is public, which means it can be changed directly. A new private property is added with a public setter/getter. The new property will be used when the existing property is null (default), which will allow the existing property to be deprecated.
Function getFunctions is changed to static - the array which it returns is static. Existing callers using it as non-static will still function correctly.
Although I am enabling this ability, I don't necessarily think it's a good idea to make use of it. See the original issue for a discussion of why. It is not mentioned in the official documentation, and I will not be adding documentation for it. The originator discovered it by reading the code, and I think that is sufficient for what will often be an ill-advised choice.
Many of the large number of problems with Calculation.php in phpstan baseline are addressed. PR 3081 ran afoul of something in phpstan. The changes in this ticket are more limited, adding a number of doc blocks but leaving executable code unchanged.
Scrutinizer had previously suggested annotations for 3 LookupRef tests, but it no longer accepts its own annotation for those cases. This PR cleans them up. ColumnsTest and RowsTest are extremely straightforward.
IndexTest is a bit more complicated, but only because, unlike the other two, it had no test which executed in the context of a spreadsheet. And, when I added those, I discovered a couple of bugs. INDEX always requires at least 2 parameters (row# is always required), but its entry in the function table specified 1-4 parameters, now changed to 2-4. And, omitting col# is not handled the same way as specifying 0 for col#, though the code had treated them identically. (The same would have been true for row# but, because it is now required, ...)
* Phpstan Baseline Fixes 2022-09-21
Eliminate about 200 more lines from Phpstan baseline. For Helper/Sample and Helper/Html and others, many properties are declared as protected despite the fact that the classes do not extend any other class, and there are no classes which extend them. They are changed to private; this could be a breaking change in circumstances for which I cannot think of a use case (user extends class for some reason).
* Slightly Botched Merge Commit
Hope this fixes phpstan.
* Xlsx Reader External Data Validations Flag Missing
Fix#2677. This PR supersedes #2679, written by @technghiath, which lacks tests, and probably doesn't solve the problem entirely. The code causing the problem appears to be the last remnant in Xlsx Reader which calls `children` using a namespace prefix rather than a namespace. That is changed, and tests are added where the tag is unexpectedly missing, and also where it uses a non-standard namespace prefix.
* Scrutinizer
Reports 1 "new" error. It isn't, but fix it anyhow.
* Fix One Existing Scrutinizer Problem
Only remaining problem in Reader/Xlsx.
* More Carets in composer.json
Following up on PR #3086, there are 3 additional items in the require-dev section that should have carets. I probably accidentally removed them for tcpdf and mitoteam, which point to the current release anyhow. Dependabot appears to be responsible for mpdf, which is not pointing to the current release, but I have tested with current successfully.
* Minor Fix for Change Made Earlier Today
"Scrutinizer Tweak" causes a Phpstan error. Fix baseline to correct it.
Mark Baker
oleibman
Robin
Rod Elias
José Rodríguez
BrokenSourceCode