Dependabot opened PR #2365 to upgrade php-cs-fixer from 2.19.2 to 3.2.1. Changes are required before that can be merged successfully. I believe all the necessary changes are in this PR.
One of the changes is to replace .php_cs.dist with .php-cs-fixer.dist.php. I have made those two identical for this PR so that there will be a meaningful delta listing. After this change is merged, master can be merged into 2365, which will hopefully pass all tests and be mergeable at that point. We can delete the unneeded file after that merge.
Spacing is changed in a handful of source members because of extra stringency in 3.2.1.
Example: right shift shared formula: IF(A$1=0,0,A1/A$1)
Expected value: IF(B$1=0,0,B1/B$1)
Actual value: IF(B$1=0,0,A1/B$1)
Similar behavior is observed when copying formulas vertically.
This issue occurs because a fixed and a non-fixed cell hit the same element of the $newCellTokens array by index $cellIndex
* Update Doc Blocks to Discourage Use of Unix Timestamps
This was suggested by issue #2347. Unix timestamps have clear disadvantages compared with the alternate methods of supplying date and time to PhpSpreadsheet - DateTime objects, Excel date time fields, and strings. In particular, Unix timestamp is not Y2038-safe on a 32-bit system, and it reflects the time in UTC, which may come as a surprise to the end-user (as it did in the cited issue). The alternate methods do not come with such baggage. This change updates some doc blocks to note that Unix timestamps are discoburage (N.B. - not deprecated). No executable code is changed.
* Document in Code As Well as Commmit Message
Per suggestion from @PowerKiKi.
* Missed One DocBlock
Including it now.
Co-authored-by: Adrien Crivelli <adrien.crivelli@gmail.com>
Fix issue #2368. PR #2265 moved the place where data validations were written to the worksheet. PR #1694 was installed afterwards, and accidentally restored the original location, so validations are now being written twice.
See issue #2315. It is nominally solved by PR #2312, but that PR is completely unsuitable for merging. This one-line change is a replacement for that PR.
As with many problems of this type, it is not clear how how to create a spreadsheet with this sort of harmless corruption in the wild. An example was supplied with the issue, and I have tested manually against it. However, the file is huge and not suitable for a formal unit test. I do not understand BIFF well enough to try and craft a suitable example on my own.
Co-authored-by: Adrien Crivelli <adrien.crivelli@gmail.com>
PR #1844 fixes it, but changes were requested. It has been almost 3 months and those changes have not been made. This PR replaces that one; it should be suitable for all supported releases of PHP through 8.1, and includes a formal unit test.
Fixes#1685Closes#1844
Prevent calling clone and getHashCode when not needed
because these calls are very expensive.
When applying styles to a range of cells can we cache the
styles we encounter along the way so we don't need to look
them up with getHashCode later.
See issue #2331. Timestamp is expected in format yyyy-mm-dd (plus other information), with the expectation that month and day are 2 digits zero-filled on the left if needed. The user's file instead used a space rather than zero as filler. Although I don't know how the unexpected timestamp was created, it was easy enough to alter the timestamp in an otherwise normal spreadsheet, and use that file as a test case.
See issue #2123. HLOOKUP needs to do some conversions between column numbers and letters which it had not been doing.
HLOOKUP tests were performed using direct calls to the function in question rather than in the context of a spreadsheet. This contributed to keeping this error obscured even though there were, in theory, sufficient test cases. The tests are changed to perform in spreadsheet context. For the most part, the test cases are unchanged. One of the expected results was wrong; it has been changed, and a new case added to cover the case it was supposed to be testing.
After getting the HLOOKUP tests in order, it turned out that a test using literal arrays which had been succeeding now failed. The array constructed by the literals are considerably different than those constructed using spreadsheet cells; additional code was added to handle this situation.
This change was suggested by issue #2316. There was a problem reading Xlsx comments which appeared with release 18.0 but which was already fixed in master. So no source change was needed to fix the issue, but I thought we should at least add the test case to our unit tests.
In developing that case, I discovered that, although comment text was read correctly, there was a problem with comment author. In fact, there were two problems. One was new, with the namespacing changes - as in several other cases, the namespaced attribute `authorId` needed some special handling. However, another problem was much older - the code was checking `!empty($comment['authorId'])`, eliminating consideration of authorId=0, and should instead have been checking `isset`. Both problems are now fixed, and tested.
My bulletproofing of these tests was not yet sufficient. Although I have never had a failure in probably thousands of tests, one user submitted a PR which did fail testing NOW, fortunately not in a test that is required to pass. The problem is that it is not sufficient merely to set the cell value inside a do-while loop; it is necessary to calculate it in order to cache its result so that results based on that cell will be internally consistent.
No source code is changed for this PR, just some tests.
* isFormula Referencing Sheet With Space in Title
See issue #2304. User sets a cell to `ISFORMULA(cell)`, where `cell` exists on a sheet whose title contains a space, and receives an error. Coordinates are not being passed correctly to Functions::isFormula; in particular, the sheet name is not enclosed in apostrophes, e.g. `Sheet Name!A1` rather than `'Sheet Name'!A1`. (Note that sheet name was not specified in Cell; PhpSpreadsheet adds it before calling isFormula.) Sheets without embedded spaces (or other non-word characters) are handled correctly with or without apostrophes, but spaces require surrounding apostrophes.
As part of this investigation, I determined that Excel handles defined names and cell ranges in ISFORMULA (subject to spills), and that PhpSpreadsheet does not. It is changed to handle them. In the absence of spill support, it will use only the first cell in the range.
Existing tests for ISFORMULA used mocking unneccesarily. They are moved to a separate test member, and mocking is no longer used.
* PhpUnit and Jpgraph
35_Char_render.php had previously been a problem only for PHP8+. It is now a problem for PHP7.4, and will therefore be skipped all the time.
* Xlsx Reader Better Namespace Handling Phase 1 Second Bugfix
See issue #2301. The main problem in that issue had been introduced with 18.0 and had already ben fixed in master. However there was a subsequent problem that had been introduced in master, an undotted i uncrossed t with namespace handling. When using namespaces, need to call attributes() to access the attributes before trying to access them directly. Failure to do so in parseRichText caused fonts declared in Rich Text elements to be ignored.
* Add An Assertion
Addresses problem in 2301 that had already been fixed.
* Permit CSV Delimiter to be Set to Null
See issue #2287. A 1-character change. The delimiter variable is defined as nullable, and getDelimiter can return null; setDelimiter should follow suit.
* Scrutinizer Inanity
Are you sure the test always returns null?????
Yes, I'm sure, that's why it's part of the test.
Let's see if we can recode it and miss this "problem".
Fixes issue #2266. Writer/Xlsx fails when there is no longer a sheet which corresponds to the definition of a local defined name. The code is changed to drop such an orphaned name. Writer/Xls does not fail under the same cicrcumstances, so no correction is needed there. Writer/Ods fails in a different manner, and is corrected to no longer do so.
* Validate Input to SetSelectedCells
See issue #2279. User requests an enhancement so that you can set a Style on a Named Range. The attempt is failing because setting the style causes a call to setSelectedCells, which does not account for Named Ranges. Although not related to the issue, it is worth noting that setSelectedCells does nothing to attempt to validate its input.
The request seems reasonable, even if it is probably more than Excel itself offers. I have added code to setSelectedCells to recognize Named Ranges (if and only if they are defined on the sheet in question). It will throw an exception if the string passed as coordinates cannot be parsed as a range of cells or an appropriate Named Range, e.e.g. a Named Range on a different sheet, a non-existent named range, named formulas, formulas, use of sheet name qualifiers (even for the same sheet). Tests are, of course, added for all of those and for the original issue. The code in setSelectedCells is tested in a very large number of cases in the test suite, none of which showed any problems after this change.
* Scrutinizer
2 minor (non-fatal) corrections, including 1 where Phpstan and Scrutinizer have a different idea about return values from preg_replace.
See issues #1432 and #2149. Data validations on an Xlsx worksheet can be specified in two manners - one (henceforth "internal") if a list is specified from the same sheet, and a different one (henceforth "external") if a list is specified from a different sheet. Xlsx worksheet reader formerly processed only the internal format; PR #2150 fixed this so that both would be processed correctly on read. However, Xlsx worksheet writer outputs data validators only in the internal format, and that does not work for external data validations; it appears, however, that internal data validations can be specified in external format.
This PR changes Xlsx worksheet writer to use only the external format. Somewhat surprisingly, this must come after most of the other XML tags that constitute a worksheet. It shares this characteristic (and XML tag) with conditional formatting. The new test case DataValidator2Test includes a worksheet which has both internal and external data validation, as well as conditional formatting.
There is some additional namespacing work supporting Data Validations that needs to happen on Xlsx reader. Since that is substantially unchanged with this PR, that work will happen in a future namespacing phase, probably phase 2. However, there are some non-namespace-related changes to Xlsx reader in this PR:
- Cell DataValidation adds support for a new property sqref, which is initialized through Xlsx reader using a setSqref method. If not initialized at write time, the code will work as it did before the introduction of this property. In particular, before this change, data validation applied to an entire column (as in the sample spreadsheet) would be applied only through the last populated row. In addition, this also allows a user to extend a Data Validation over a range of cells rather than just a single cell; the new method is added to the documentation.
- The topLeft property had formerly been used only for worksheets which use "freeze panes". However, as luck would have it, the sample dataset provided to demonstrate the Data Validations problem uses topLeft without freeze panes, slightly affecting the view when the spreadsheet is initially opened; PhpSpreadsheet will now do so as well.
It is worth noting issue #2262, which documents a problem with the hasValidValue method involving the calculation engine. That problem existed before this PR, and I do not yet have a handle on how it might be fixed.
* Fraction Formatting
See issue #2253. User's analysis was correct - leading zeros in the decimal portion were being stripped out, so 0.0625 and 0.625 were being treated the same. As it turns out, integers also aren't handled well (`0 0/1` anyone?). The latter problem had been hidden because caller tested for integer first and skipped call if true; but FractionFormatter::format is public and should work correctly regardless. All Phpstan baseline entries for FractionFormatter and NumberFormatter are eliminated. New test data is added; no need for changes to test code.
* Scrutinizer
Ensure result is string.
See issue #2239. Problem is dealt with at the source, by making sure that Reader Xls checks for use of 'GENERAL' rather than 'General'. There doesn't seem to be a reason to test in other places, or to test for other casing variants.
* Html Reader Comments
See issue #2234. Html Reader processes Comment as comment, then processes it as part of cell contents. Change to only do the first. Comment Test checks that comment read by Html Reader is okay, but neglects to check the value of the cell to which the comment is attached. Added that check.
* Disconnect Worksheets
... at end of test.
* Csv Handling of Booleans (and an 8.1 Deprecation)
PhpSpreadsheet writes boolean values to a Csv as null-string/1, and treats input values of 'true' and 'false' as if they were strings. On the other hand, Excel writes boolean values to a Csv as TRUE/FALSE, and case-insensitively treats a matching string as boolean on read. This PR changes PhpSpreadsheet to match Excel.
A side-effect of this change is that it fixes behavior incorrectly reported as a bug in PR #2048. That issue was closed, correctly, as user error. The user had altered Csv Writer, including adding ```declare(strict_types=1);```; that declaration was the cause of the error. The "offending" statements, calls to strpbrk and str_replace, will now work correctly whether or not strict_types is in use.
And, just as I was getting ready to push this, the dailies for PHP 8.1 introduced a change deprecating auto_detect_line_endings. Csv Reader uses that setting; it allows it to process a Csv with Mac line endings, which happens to be something that Excel can do. As they say in https://wiki.php.net/rfc/deprecations_php_8_1, where the proposal passed without a single dissenting vote, "These newlines were used by “Classic” Mac OS, a system which has been discontinued in 2001, nearly two decades ago. Interoperability with such systems is no longer relevant." I tend to agree, but I don't know that we're ready to pull the plug yet. I don't see an easy way to emulate that functionality. For now, I have silenced the deprecation notices with at signs. I have also added a test case which will fail when support for that setting is pulled; this will give time to consider alternatives.
* Scrutinizer: Handling ini_set
This could be interesting. It doesn't like not handling an error condition for ini_set. Let's see if this satisfies it.
* New Looming Problems with PHP8.1
More deprecations. The following corrections are made in this PR:
- Calculation.php has a call to ctype_upper and apparently one of the samples manages to pass it an int. That function treats int differently from numeric strings, and that treatment is on the deprecation list. Enclosing the argument in quotes cannot cause a problem unless the int represents the ASCII value of an uppercase letter, which I cannot believe is the case; anyhow, if it is, the code will wind up with a nonsense result, e.g. if column is C and row is 1, the cell will be resolved as C1, but if column is int 67 (ASCII for C) and row is 1, the cell will be resolved as 671, not C1.
- Several Worksheet iterators need one or more functions to explicitly declare their return types. Thankfully, this does not seem to break earlier PHP versions.
- LocaleFloatsTest - see issue #1863. This was supposed to fail in PHP 8.0, but var_dump continued to support the old way (for 64-bit PHP only, not for 32-bit). PHP 8.1 appears to correct that omission, and the test will now fail. It doesn't show up as a failure in Github because of an accident - the attempt to set the locale to France in Github fails, so it skips the test before attempting the var_dump. But it does fail locally on my system. I have changed the test to use sprintf rather than var_dump; I think users are far more likely to use sprintf rather than var_dump in their applications. (They are, of course, even more likely to just cast to string, but the result of doing that is already different in 8.0 than in 7.4.) I would be equally happy to delete the test altogether.
There remain PHP 8.1 problems with Mpdf which are, of course, out of scope here.
There is one additional problem that I do not address in this ticket. The auto_detect_line_endings setting is being deprecated. This has some implications for Csv. I have another PR ready for Csv, and will discuss that problem there.
* Minor Scrutinizer Error
Hopefully fixed now.
* Fix 112 Scrutinizer Problems in 1 Module
The module is Reader/Xls/Escher - reading pictures from an Xls workbook. The errors fall into precisely 2 categories.
- Assigning a value to a variable which is not subsequently used (35). Although the statements therefore don't accomplish anything, I think they have documentary value for understanding the file layout. So, I have commented out the statements in question rather than deleting them.
- Class property `$this->object` can belong to any of several classes (77). When you invoke a method on it, Scrutinizer and Phpstan flag the statement if not all the candidate classes support the method. Neither has enough information to recognize that the method always exists for any object which reaches the statement. Scrutinizer is noisier about it - it issues a separate message for each class that doesn't support the method, while Phpstan issues a single message. Adding a `method_exists` test is sufficient for Phpstan. We'll see what Scrutinizer thinks when I push the change. If it still doesn't like it, we've eliminated only 35 problems. Phpunit coverage confirms that `method_exists` is always true at the appropriate point.
* Scrutinizer Can Be VERY Annoying
I wasn't looking to do a major rewrite. I was hoping 112 fixes would suffice. Oh well, let's see what happens now.
Owen Leibman
Adrien Crivelli
Zaytcev Ivan
Roland Eigelsreiter
Einar
Richard van Velzen
jomisacu
Sergiy Petrov
ayacoo
Georgiy
Alayn Gortazar