Dependabot submitted PRs #2719 and #2720 to upgrade Phpstan. As with most Phpstan upgrades, there are new error messages; this PR is an attempt to fix all 58 of the new problems in order to allow the upgrade to proceed.
Most of these fixes involve the addition of doc-block type annotations, often involving the assignment of the 'objectionable' portion of the statement to a new variable. Some use explicit casting when I am sure that's safe. Some (Reader/Ods) involve defeating result caching by Phpstan.
* Add editAs Property for 2-cell Anchor Drawings
This change builds on PR #2532 (@naotake51 as PR #2532), using ideas from PR #2237 (@AdamGaskins), which has had changes requested for several months. It covers a lot of the same ground as 2532. In Excel, two-cell anchor drawings can be edited as "twocell", "onecell", or "absolute". This PR adds support for those options, with a sample file that demonstrates the difference in addition to unit tests. Several other tests are added to improve the spotty coverage for Drawings.
There have been several other tickets referencing two cell anchors, including issue #1159 and PR #1160 (@sgarwood, who also added support for editAs), PR #643, and issue #126, all now closed but not necessarily entirely resolved. I will try to ensure that those tickets are addressed with this one.
And, in trying to make sure 1160 is covered, I stumbled upon a bug. If you use the same image resource to create two+ memory drawings, the MemoryDrawing destructor for the first will cause the rest to generate a very long warning message. This is not a problem for Php8+, only for Php7-. I have suppressed the message in the MemoryDrawing constructor. 1160 went stale due to an unresolved test error, but I don't think this was the problem. At any rate, its test works now.
* Scrutinizer
It reported 1 minor issue (fixed normally), and 2 major. One is fixed with a kludge. The other is a case where Scrutinizer's analysis is just wrong, and I can't figure out a kludge. But I was able to add an annotation (the first time I've managed to get one past phpcs/php-cs-fixer). We'll see.
These changes have already been implemented twice, and been regressed twice. I'll try once more (with a different approach), then give up ...
As configured, Phpstan running under Php7 reports no errors. However, running under Php8, it reports 100 (!) errors. The vast majority of these are due to two reasons:
- renaming parameters in Php builtin functions in preparation for named parameters.
- using the new class GdImage rather than type resource as the argument type for many image-based functions.
Regardless of the cause, this will be a problem sooner or later. This PR is an attempt to get ahead of that problem. For source members, it mostly adds annotations or updates doc-blocks. Only 2 members have changes to executable code, and these are very minor - BitWise and Writer/Xlsx. For test members, all baseline errors are deleted and the code is fixed. Php7 and Php8 both report no errors with this configuration.
* Add two cell anhor drawing.
* Add "Support for two cell anchor drawing of images." to CHANGELOG.md
* Add pull-request link to "Support for two cell anchor drawing of images." of CHANGELOG.md
You can set up a conditional style to, say, apply to cells equal to boolean values. For such conditions, the Excel XML specifies `TRUE` or `FALSE`. It is noteworthy that false matches empty cells as well as FALSE, but not 0; similarly TRUE does not match 1. The Xlsx Writer just casts these values to string, which will not work properly. The Xlsx Reader treats the values as strings, so it won't work properly either. This PR corrects both. Also the doc blocks in Style/Conditional allow bool in some places, but not in others; these are corrected but no executable code is changed there.
These changes were already implemented as PR #2428. They were, alas, regressed by PR #2585. If at first you don't succeed ...
As configured, Phpstan running under Php7 reports no errors. However, running under Php8, it reports 100 (!) errors. The vast majority of these are due to two reasons:
- renaming parameters in Php builtin functions in preparation for named parameters.
- using the new class GdImage rather than type resource as the argument type for many image-based functions.
Regardless of the cause, this will be a problem sooner or later. This PR is an attempt to get ahead of that problem. For source members, it tweaks only the Phpstan configuration files, without changing any PhpSpreadsheet code. For the small number of test members involved, the code is fixed. Php7 and Php8 both report no errors with this configuration.
Because this involves no changes to code, and because Phpstan baseline is a common cause of merge difficulties, I will probably merge this in a day or two, more quickly than I customarily do.
Note that this method is used when translating Excel functions between en and other locale languages, as well as when converting formulae between different spreadsheet formats (e.g. Ods to Excel)
Nor is this a perfect solution, as there may still be issues when function calls have array arguments that themselves contain function calls; but it's still better than the current logic
* Initial work on implementing Array-enabled for the HLOOKUP() and VLOOKUP() functions
* In the MATCH() function, we should also use `evaluateArrayArgumentsIgnore()` because the lookupvalue and matchType arguments can be array arguments, but lookupArray is always a dataset matrix
* Split Information functions into a dedicated class and namespace and categorise as Value or Error
* Refactor all error functions into the new ExcelError class
* Initial work enabling Excel function implementations for handling arrays as aguments when used in "array formulae".
So far:
- handling for single argument functions
- for functions where only one of the arguments is an array (a matrix or a row/column vector)
- for when there are two array arguments, and one is a row vector, the other a column vector
- for when there are either 2 row vectors, or 2 column vectors
- for a matrix and either a row or column vector
Will work ok, as long as there are no more than two array arguments; still need to identify the logic to apply when there are more than two arrays; or there are two that aren't an already supported row vector/column vector pairing (ie two matrices).
* Throw an exception if we have three or more array arguments (after flattening) passed to a supported function until we can identify the abstruse non-euclidian logic behind how Excel handles building, using and presenting those n-dimensional result arrays
* Implement array arguments for the DATE() function so that we can verify that paired arrays/vectors work with functions that support more than 2 arguments
* Implement array arguments for the many of the Math/Trig functions
* Update change log
Dependabot pushed Phpstan 1.4.4 and then 1.4.5 (PR #2548). Because of some incompatible error messages between the current and new release, changes are required in order to be compatible with current and new releases.
* Allow single-cell checks on conditional styles, even when the style is configured for a range of cells
* Work on the CellMatcher logic to evaluate Conditionals for a cell based on its value, and identify which conditional styles should be applied
* Refactor style merging and cell matching for conditional formatting into separate classes; this should make it easier to test, and easier to extend for other CF expressions subsequently
* Added support for containsErrors and notContainsErrors
* Initial work on a wizard to help simplify created Conditional Formatting rules, to ensure that the correct expressions are set
* Further work on extending the Conditional Formatting rules to cover more of the options that are available in MS Excel
* Prevent phpcs-fixer from removing class @method annotations, used to identify the signature for magic methods used in Wizard classes
* Implement `fromConditional()`` method to allow the creation of a CF Wizard from an existing Conditional
* Ensure that xlsx Reader picks up the timePeriod attribute for DatesOccurring CF Rules
* Allow Duplicates/Uniques CF Rules to be recognised in the Xlsx Reader
* Basic Xlsx reading of CF Rules/Styles from <extLst><ext><ConditinalFormattings> element, and not just the <ConditinalFormatting> element of the worksheet
* Add some validation for operands passed to the CF Wizards
- remove any leading ``=` from formulae, because they'll be embedded into other formulae
- unwrap any string literals from quotes, because that's also handled internally
Handle cross-worksheet cell references in cellReferences and Formulae/Expressions
* re-baseline phpstan
* Update Change Log with details of the CF Improvements
* Refinement for XIRR
Fix#2469. The algorithm used for XIRR is known not to converge in some cases, some of which are because the value is legitimately unsolvable; for others, using a different guess might help.
The algorithm uses continual guesses at a rate to hopefully converge on the solution. The code in Python package xirr (https://github.com/tarioch/xirr/) suggests a refinement when this rate falls below -1. Adopting this refinement solves the problem for the data in issue 2469 without any adverse effect on the existing tests. My thanks to @tarioch for that refinement.
The data from 2469 is, of course, added to the test cases. The user also mentions that an initial guess equal to the actual result doesn't converge either. A test is also added to confirm that that case now works.
The test cases are changed to run in the context of a spreadsheet rather than by direct calls to XIRR calculation routine. This revealed some data validation errors which are also cleaned up with this PR. This suggests that other financial tests might benefit from the same change; I will look into that.
* More Unit Tests
From https://github.com/RayDeCampo/java-xirr/blob/master/src/test/java/org/decampo/xirr/XirrTest.javahttps://github.com/tarioch/xirr/blob/master/tests/test_math.py
Note that there are some cases where the PHP tests do not converge, but the non-PHP tests do. I have confirmed in each of those cases that Excel does not converge, so the PhpSpreadsheet results are good, at least for now. The discrepancies are noted in comments in the test member.
* New Error Reported with Phpstan 1.3
Dependabot opened a number of PRs. Most are successful, but this change is necessary to allow PR #2477 to complete successfully, and that is apparently a necessity for PR #2479.
Phpstan 1.3 objected to:
```php
trigger_error($errorMessage, E_USER_ERROR);
return false;
```
It claims the return statement is unreachable. This isn't precisely true - an error handler might allow the return to be reached. At any rate, I have slightly restructured the code so that Phpstan will not object either with 1.2 or 1.3, which should allow the blocked PRs to succeed. There had been no previous tests for what happens when there is a formula error when suppressFormulaErrors is true.
* Scrutinizer
Didn't like effect of changes which Phpstan liked. Hopefully this will work better.
* Improvement
Eliminate added function.
Replace mock tests with real ones when possible. The original tests are all still present; they just take place in a more representative scenario.
After this, there will be 4 remaining uses of mocking. Of these, 3 are needed for scenarios which are otherwise hard to test - WebServiceTest, CellsTest, and SampleCoverageTest. For the other one, AutoFilterTest, I just can't figure out what it's trying to accomplish, so have left it alone.
This change is almost entirely restricted to tests. There is a one-line change in src. When the first argument passed to OFFSET is null or nullstring, the returned value is currently 0. However, according to the documentation for Excel, it should be `#VALUE!`. The code is changed accordingly.
* Rename Two Test Files
When I run unit tests only for Reader/Xlsx, phpunit is issuing a deprecation message because the names of 2 files have an extra dot in them and thus don't match the class name in the file. I do not see these warnings when I run the entire test suite.
* Remove Phpstan Annotations
It was a bit difficult to handle a cast from mixed to string.
* Fix Same Phpstan Problem in One Other Test
This is the only other test case that tries to cast mixed to string.
* Null Passed to AutoFilter SetRange
Fix#2281. Delete auto filter set range to null, but should set it to null string. This causes a deprecation warning in Php8.1.
* Constructor Call Also Sets Range to Null
Should set it to null string.
* Fill Pattern Start and End Colors
Fix#2441. The Fill constructor sets start color to white and end color to black and the Xlsx writer writes these values to the output file. This appears to be the wrong setting for all 7 LIGHT* pattern types, 2 of the 7 DARK* patterns (DARKGRAY and DARKTRELLIS), and 1 of the 3 GRAY patterns (GRAY0625). When the wrong colors are written at save time, those patterns are not as expected. Xls writer does not appear to have the same problem.
The XML does not require either a start or end color, and the omission of these colors in the file being read was responsible for the problem. The code is changed to mimic that behavior by omitting the color tags at write time if they have not changed from when they were created by the Fill constructor (they will be written for gradient or solid patterns regardless).
This is another change which is easier to confirm via samples rather than tests. There are separate samples for Xlsx and Xls; as Excel will be quick to warn you, Xls is not as fully functional as Xlsx with respect to fill patterns. The samples do include a cell where one of the cells (LightGrid in C11) explicitly specifies the "default" colors.
* Scrutinizer
It somehow ascribed to me a problem in code which was unchanged by this PR. Correct it anyhow, along with some Phpstan fixes (errors now ignored because of change).
* Added Tests
Also corrected some docBlock problems with Style/*/parent and getSharedComponent.
* Create 2 Abstract Methods
Scrutinizer complained that 2 methods found in all Supervisor sub-types were not defined in Supervisor. Add abstract methods to satisfy it.
* Scrutinizer Ignoring Typehints
Try this instead.
* Slight Improvement
Better handling of Style->getParent().
* XLSX Image background in comments
* XLSX-Image-Background-In-Comments (#1547)
* Test fixes, convertion for comment sizes from px to pt, fix for setting image sizes from zip, set image type
* Merge remote-tracking branch 'origin/XLSX-Image-Background-In-Comments' into XLSX-Image-Background-In-Comments
* Tests to check reloaded document.
Co-authored-by: Burkov Sergey
PR #2428 reduced the count of Phpstan errors reported under Php8 from 218 to 13. This PR takes care of those 13, which could not necessarily be handled merely by tweaks to the configuration files.
The commonest problem was that Phpstan under Php7 thinks date_parse can return an array or false, but, under Php8, it thinks it can return only array. The documentation agrees with the Php7 interpretation, but I have not been able to construct a case, in any release of Php, where date_parse returns false. In case of error, it sets elements 'error' and 'error_count' in the returned array. Code is changed to handle either possibility in a manner of which Phpstan will approve.
Other problems are handled through type-hinting.
As configured, Phpstan running under Php7 reports no errors. However, running under Php8, it reports 218 (!) errors. The vast majority of these are due to two reasons:
- renaming parameters in Php builtin functions in preparation for named parameters.
- using the new class `GdImage` rather than type `resource` as the argument type for many image-based functions.
Regardless of the cause, this will be a problem sooner or later. This PR is an attempt to get ahead of that problem. It tweaks only the Phpstan configuration files, without changing any PhpSpreadsheet code. Php7 continues to report no errors, and the number of errors for Php8 is reduced to 13. Although those 13 can also be tweaked, there is a case to be made that code corrections might be better in at least some of those instances. I will attend to those as one or more PRs after this one.
Because this involves no changes to code, and because Phpstan baseline is a common cause of merge difficulties, I will probably merge this in a day or two, more quickly than I customarily do.
Fix#1691. PhpSpreadsheet allows the setting of different page size and orientation on each worksheet. It also allows the setting of page size and orientation on the PDF writer. It isn't clear which is supposed to prevail when the two are in conflict. In the cited issue, the user expects the PDF writer setting to prevail, and I tend to agree. Code is changed to do this, and handling things in this manner is now explicitly documented.
PhpSpreadsheet uses a default paper size of Letter, and a default orientation of Default (which Excel treats as Portrait). New static routines are added to change the default for sheets created subsequent to such calls. This could allow users to configure these defaults better for their environments. The new functions are added to the documentation.
Fix#2387. Fix#2075. There was substantial refactoring of Writer Xlsx styles in 18.0. An existing static property `$theme` was intended to be shared by both Writer Xlsx and the new Writer Xlsx Styles. However, the initialization of the property in the latter happened later than it should have. This PR makes that initialization happen as soon as the theme has been read. Also, declaring that property as static seems questionable; I have made it an instance member. This small re-factoring makes it possible to now support Themes in tab colors.
Since this PR changes Reader/Xlsx/Styles, add type-hinting throughout that module to eliminate Phpstan/Scrutinizer problems. I also removed method readStyle from Reader/Xlsx, since it was essentially duplicated in Reader/Xlsx/Styles. And I added a small number of tests to ensure that Styles is 100% covered. All of this is necessary in preparation for Namespacing phase 2.
Fix#2385. NumberFormatter is using sprintf on a float, and is seeing inconsistent rounding as a result (it will also occasionally result in `-0`). Change to round the number before presenting it to sprintf.
* Allow Skipping One Unit Test
Alone in the test suite, URLImageTest needs to access the internet. It's a little fragile (the site that it's looking for may go away or change), but no real problem. However, on my system, it runs afoul of my proxy. Rather than jumping through hoops when I run the test suite (which happens very often), I am changing the test to skip if an environment variable is set to a specific value. This should not adversely affect anyone, and the test will still run in github, but it will help me a lot.
* Scrutinizer
It complained that my one new if statement made the module too complex. There actually were a number of if-then-else situations that could be handled just as well with assertions. I have changed it accordingly.
* AutoFilter Improvements
Fix issue #2378. The following changes are made:
- NotEqual tests must be part of a custom filter. Documentation has been changed to indicate that.
- Method setAndOr was replaced by setJoin some time ago. Documentation now reflects that change.
- Documentation to indicate that string filters are not case-sensitive, same as in Excel.
- Filters testing against numeric value now include a numeric test (not numeric for not equal, numeric for all others).
- String filter had previously treated everything as a test for "equal". It now handles "not equal" and the variants of "greater/less" with or without "equal".
- Documentation correctly stated that no more than 2 rules are allowed in a custom filter. Code did not enforce this restriction. It now does, throwing an exception if an attempt is made to add a third rule.
- Deleted a lot of comments in Rule.php to make it easier to see what is not yet implemented (between, begins with, etc.). I may take these on in future.
- Added a number of tests for the new functionality.
* Not Sure Why Phpstan Results Differ Local vs Github
Let's see if this change suffices.
* Phpstan Still
Not sure how to convince it. Let's try this.
* Phpstan Solved
Figured out the problem on my local machine. Expect this to work.
* Update Some Doc Block Annotations
See PR #2010. That PR was never completed, and has gone stale. However, it was correct in identifying a situation where the doc block was not entirely accurate. It did not go far enough - several closely-related methods have similar problems. This PR attempts to fix the original problem and its close relations. Aside from the doc block changes, there are very minor changes to executable code. It also changes some of the unit tests targeted at the methods in question to eliminate mocking in favor of 'real' tests.
* Change Method to Static
Otherwise Scrutinizer will complain, even though Phpstan doesn't.
* Scrutinizer
Various clean-up activities.
* Scrutinizer
@#&$(*#&$ Got complexity down from 53 to (I think) 50. Don't really know what the target is.
* Code Changes Suggested By Review
Some improvements suggested in review by @PowerKiKi.
* Update Cells.php
* Merge Conflict
A change to a parameter name caused several problems when trying to fix it on Github. Fixing it locally should do the trick.
* Merge Conflicts in Phpstan Baseline
PR #2382 made a large number of changes to Phpstan Baseline, some of which conflicted with the Phpstan Baseline changes in this PR. This should resolve them all.
MarkBaker
Mark Baker
oleibman
naotake51
oleibman
leo-bsv
Adrien Crivelli