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.
* Tweaks to Input File Validation
This started as a response to issue #1718, for which it is a partial (not complete) solution. The following changes are made:
- canRead can currently throw an exception. This seems wrong. It should just return true/false.
- Breaking change of sorts. When AssertFile encounters a non-existent or unreadable file, it throws InvalidArgumentException. This does not make sense. I have changed it to throw PhpSpreadsheet/Reader/Exception.
- Since the previous bullet item required changing of most of the Reader files anyhow, this is a good time to add explicit typing for canRead in the function signature rather than the DocBlock. Since all the canRead functions inherit from an abstract version in IReader, they all have to be changed simulatneously. Except for Xlsx and Ods, most of the Reader files are otherwise unchanged.
- AssertFile is changed to add an optional "zip member" parameter. It will check for the existence of an appropriate member in what is supposed to be a zip file. It is used by Xlsx and Ods.
- Verifying that a given file is a valid zip ought to be a feature of ZipArchive. Thanks to a particularly nasty bug in php/libzip (see https://bugs.php.net/bug.php?id=81222), it is unsafe to attempt to open a zero-length file as a zip archive. There is a solution, but it does not apply to all the PHP releases which we support, and isn't even necessarily supported on all the point versions of the PHP versions which we do support. I have coded up a manual test for "valid zip", with a comment pointing to the spec.
- In theory, tests now cover 100% of the code in Shared/File. In practice ... One of the tests require that chmod works properly, which is not quite true on Windows systems, so that test is skipped on Windows. Another test requires that php.ini uses a non-default value for upload_temp_dir (can't be overridden in application code), which is probably not the case when Github runs the unit tests, so that test is skipped when appropriate. I have run tests for both on systems where they are not skipped.
* Update File.php
* Scrutinizer Timeout
It's not actually timing out, it's just waiting for something to finish that finished ages ago. Making a meaningless comment change in hopes that will clear the jam. Not particularly hopeful.
filter_var( $flag, FILTER_VALIDATE_BOOLEAN) chosen to handle both 0/1 and true/false values in the file being read.
* Fix writing data validation flag showDropDown (Inverted logic in reader was replicated to Writer)
* Initial adjustments to Xlsx Reader for two possible locations for AutoFiter information, either on the sheet itself for older files, or in the tables/tableX file for more recent files
* Refactor AutoFilter Reader logic into separate methods; preparatory work toward the eventual goal of moving it into its own dedicated AutoFilterTables class
* Basic unit tests to verify that the Xlsx Reader can read both the older and Office365 variants of the files used to store AutoFilter structure
* Xls Reader Handle MACCENTRALEUROPE With or Without Hyphen
Fixes issue #549 and https://github.com/Maatwebsite/Laravel-Excel/issues/989 (which is the source of the new test file). Some systems accept MACCENTRALEUROPE as the name for the appropriate encoding, and some accept MAC-CENTRALEUROPE. I fortunately have access to at least one of each type, and have run the tests on each.
CodePage.php has an array of translations from codepage number to string. I now allow the value to itself be an array; if so, the code will test each in turn to see if it can be used in iconv. I did not go fishing for other similar problems. If such show up, they can be dealt with in the same manner as this one. I don't really expect others, since this is a problem not merely for Xls, but, even then, it applies only to BIFF5 and earlier.
I also moved XlsTest from Reader to Reader/Xls.
* Cache Successful Result For Future Use
Per suggestion from @MarkBaker
This is a leftover Scrutinizer change, but it needed more attention than most others. Chart/Title DocBlocks define caption as `null|string`. However, in the wild, Excel usually presents the caption as an array, and not an array of strings but rather of RichText items. I am not sure why an array is needed since a RichText item can contain many text runs, but things are what they are.
Reader/Xlsx/ChartTitleTest reads a spreadsheet with the captions stored as a RichText array. Since it performs array operations on something the DocBlock says cannot be an array, Scrutinizer objects, although not seriously enough to fail the module. Phpstan also objects; its objection is silenced with an annotation. Aside from this test, there are other tests which do set the caption to a string, and Excel seems to handle that without a problem. So, I have changed the DocBlock to specify `array|RichText|String`. I have dropped null as a possibility; nullstring will do equally well.
Because getCaption can now return multiple datatypes, I think a new function which can return the text portion of the entire caption as a single string is needed. I have added it. This simplifies the test named above, and some code in Writer/Html. The latter is not part of unit testing because the version of JpGraph found in Composer is too antiquated. I verified the Html change manually by running samples/Chart/32_Chart_read_write_HTML.php using a recent version of JpGraph. It was as a result of this test that I uncovered issue #2203. I did not see anything about Charts in docs, so did not add a description of the new function there.
Phpstan is happy with the changes. We'll see how Scrutinizer feels when I push it.
See issue #2203. An undotted i uncrossed t. When using namespaces, need to call attributes() to access the attributes before trying to access them directly. Failure to do so in castToFormula caused problem for shared formulas.
Surprisingly, this didn't show up in unit tests. Perhaps sharing the same formula between two cells isn't common. It did show up in Chart Samples. I've added a test.
I was really inclined to merge this right away. Not to worry - I can control myself. It should be moved fairly quickly nevertheless.
Just reviewing Scrutinizer's list of "bugs". There are 19 ascribed to me. For some, I will definitely take no action (e.g. use of bitwise operators in AND, OR, and XOR functions). However, where I can clean things up so that Scrutinizer is satisfied and the resulting code is not too contorted, I will make an attempt.
This is the last of this set of changes. It corrects 2 problems according to Scrutinizer, and about 20 per Phpstan.
Just reviewing Scrutinizer's list of "bugs". There are 19 ascribed to me. For some, I will definitely take no action (e.g. use of bitwise operators in AND, OR, and XOR functions). However, where I can clean things up so that Scrutinizer is satisfied and the resulting code is not too contorted, I will make an attempt.
This PR corrects 2 problems according to Scrutinizer, and 1 per Phpstan. Only test members are involved.
Just reviewing Scrutinizer's list of "bugs". There are 19 ascribed to me. For some, I will definitely take no action (e.g. use of bitwise operators in AND, OR, and XOR functions). However, where I can clean things up so that Scrutinizer is satisfied and the resulting code is not too contorted, I will make an attempt.
This PR corrects 2 problems according to Scrutinizer, and about 30 per Phpstan.
Adrien Crivelli
jomisacu
oleibman
Sergiy Petrov
ayacoo
Georgiy
Alayn Gortazar
James Lucas
Mark Baker