The latest release seems to not want you to give a class element both a Php type and a doc-block type. I used the "fix" operand to delete the redundant doc-block declarations, with no other changes. So there should be no change to executable code.
Fix#3900. The code to adjust Decimal and Thousands separators to user-specified choices expects to convert periods to commas and vice versa - hard-coded without taking the user specification into account. This is likely to be the only significant use case, however, as the issue demonstrates, it is not the only possibility. In particular, it will mess up Csv output if decimal separator is kept as period, but thousands separator is set to null string. The code is altered to replace period with the user-selected decimal separator, and comma with the user-selected thousands separator. No existing tests broke as a result of this change in behavior.
This PR resolves about half the statements which we tell Phpstan to ignore. There will still be 23 such annotations spread over 10 source modules for various reasons (too complicated, suspect PhpSpreadsheet code, one Phpstan bug), plus some deliberate errors in the test suite.
I check from time to time. There are a number of problems now, mostly due to the elimination of Php 7.4 and replacement of doc-block typing with explicit Php typing.
- Bitwise functions were particularly affected by PR #3718 and PR #3793.
- Chart/Axis and Writer/Xlsx were amusingly affected by PR #3836, which added a scaling option which included an array indexed by the known allowable factors, one of which is 1 trillion, which cannot be represented as an integer on a 32-bit system. Issue3833Test, introduced by the same PR (and not suffering any errors) was expanded to test this value.
- Some minor changes to Reader/Xls and Shared/OLE/PPS to accommodate hex values which are negative in 32-bit but which Php-32 may wind up casting to large floating point numbers; it is not clear to me why these hadn't shown up as problems previously. Possibly this is the result of changes in the most recent Php versions.
- BitAndTest, BitOrTest, BitXorTest and Shared/DateTest were adversely affected by PR #3859 when arguments and/or expected results too large for a 32-bit integer were supplied.
- ImExpTest required a slightly reduced precision for 32-bit. No idea why this hadn't shown up earlier.
Change "mixed" declarations to more accurate types in test members; in particular, change those that would be flagged if we were to run Phpstan at level 9 (we currently run level 8). I may or may not follow up with source code (over 700 level-9 problems remain for src), but, as with strict typing, there is no reason to avoid the effort for test members.
It was necessary to update some doc blocks in src to accommodate this change. However, no executable code is touched.
Fix#2809. User felt font files should be searched recursively. The problem could be overcome merely by specifying a more precise font location. However, user has a point - the file layout on user's system is pretty common, and can be integrated into PhpSpreadsheet code easily. As a bonus, the DocBlock for setTrueTypeFontPath, which the user felt was misleading, doesn't even have to change.
This was suggested by the investigation of issue #3811. No fix is necessary for the issue. However, two possible code solutions (Php setlocale, which comes with certain design flaws, and StringHelper set(Decimal/Thousands)Separator were suggested, and neither is adequately tested. This PR adds such tests.
Unusually, getting StringHelper Decimal Separator, Thousands Separator, and Currency Code can result in a change to those properties. So, the existing design in several tests where those properties are captured in Setup and restored in Teardown do not work quite as designed. Instead, the ability to set those properties to their default value (null) is added, and the tests re-done to restore the default in Teardown.
The two methods yield the same results when parsing input. However, they diverge when examining output fields through `getFormattedValue`. Such output is currently correct (usually) when using setlocale, but not when using StringHelper. The former works through the 'trick' of using `sprintf(%f)`, which generates a locale-aware string. However, using non-locale-aware `sprintf(%F)` followed by `str_replace` will produce the correct result for both setlocale and StringHelper. One place in the code uses a cast to string, which is incorrect for both methods. Following that up with the same str_replace makes it correct for both. These changes permit, but do not require, the user to avoid setlocale altogether.
It remains an open question whether Settings/Calculation::setLocale should set DecimalSeparator, CurrencySeparator, and CurrencyCode. That makes logical sense, but it would be a breaking change, and having to explicitly set those values when using setLocale does not seem especially burdensome. For now, such a change will not be made.
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.
* Check For and Eliminate Octal Literals
Php8.1 introduced a new form of octal literal (0o123); the old form (0123) continues to be supported, but can certainly be misleading. There are exactly 6 uses of the old form in the code base (5 in tests, 1 in infra). 4 of these are clearly unintended (02 or 03), and the leading 0 should just be dropped for those. The other 2 are for file permissions and are easily replaced.
* Minor Changes to LocaleGenerator
Testing the permissions change exposed some problems when the locale files don't exist. This never shows up as an issue when the files do exist, which is pretty much all the time, but correct the theoretical exposures anyhow.
* Populate bg and en_uk Locale Files from Spreadsheet
Also rename locale generator so that it is executed first. This is when it should happen because other tests depend on its results.
* Some Bulgarian Function Omissions
Figured out a reasonable way to compare old Bulgarian function file to new; this identified a handful of now-corrected omissions and errors. Note that JIS function, in old list, is not on spreadsheet, but the Bulgarian translation is the same as English (so the translation isn't needed), and the function has been replaced by DBCS, which is also not on the spreadsheet.
* Preserve Transparency in Memory Drawing
Fix#3626. User felt that Exact setting of column width should not add padding. In order to avoid making this a breaking change, I am adding a new boolean property `usePaddingForExact` with setter and getter to Shared/Font; default is false (current behavior).
* Change Parameter
Instead of bool, make it null|float|int, allowing user to set whatever is deemed appropriate.
Successor to PR #3523. There are 494 single-line changes (`public function provider` to `public static function provider`) in this PR. None of these were made manually; they were all created with the following script (adapted from
https://stackoverflow.com/questions/25909820/how-to-recursively-iterate-through-files-in-php):
```php
$dir = 'C:/git/unit10prep2/tests/PhpSpreadsheetTests';
$it = new RecursiveDirectoryIterator($dir);
// Loop through files
foreach(new RecursiveIteratorIterator($it) as $file) {
if ($file->getExtension() === 'php') {
$contents = file_get_contents($file);
$new = preg_replace('/public function (\\w*)([Pp])rovider/', 'public static function $1$2rovider', $contents);
if ($new !== $contents) {
echo "changing $file\n";
file_put_contents($file, $new);
}
}
}
```
After this PR, there will be one more, with a small number of test changes, and enabling PhpUnit 10 for Php 8.1+.
* PhpUnit 10 Compatibility Part 1
This is not a change to move to PhpUnit 10. There is no compelling reason to do so at this time, although it is bound to happen eventually. There are a staggering number of problems (somewhere around 3,000) with the current test suite under PhpUnit 10; this is an attempt to get ahead of the curve by addressing them now.
Method `setOutputCallback` has gone away. This affects only Helper/SampleTest. It appears that `ob_start` and its allies provide an effective equivalent. FWIW, the absence of `setOutputCallback` is a good indication of whether or not PhpUnit 10 is in use, and I will use that fact in a few tests.
Class `ComplexAssert` with no constructor, and always used with `new ComplexAssert()`, extends `TestCase`. Apparently, the constructor for TestCase requires an argument, and PhpUnit 10 complains about not supplying one. Adding an empty constructor to ComplexAssert avoids this problem.
There are two very minor source changes, to Calculation/Calculation and Reader/Xlsx, where problems were exposed with PhpUnit 10 that had not been previously been exposed. AFAIK, these are the only source changes required; the rest of the changes are to test members.
The bulk of the problems are because PhpUnit 10 insists that provider methods be static. Most of those can be changed by a script without any further action; those changes will constitute the Part 2 counterpart of this PR. In this PR you will find the exceptional cases that can't be automated for one reason or another. The tests for Database functions have mild complications that are easily handled. Most of the other provider changes in this PR are because the method names didn't follow an established pattern ('provider' isn't part of the method name); those are also easily handled manually. Modifying the following tests provided significant challenges:
- Writer/Xls/WorkbookTest testAddColor
- Worksheet/Table/TableTest testSetRangeValidRange
The handling of warning messages issued by the code differs in PhpUnit 10. According to the change log, "This means that using PHP functionality which triggers E_DEPRECATED, E_NOTICE, E_STRICT, or E_WARNING or calling code which triggers E_USER_DEPRECATED, E_USER_NOTICE, or E_USER_WARNING can no longer hide a bug in your code." To me, the effect of that change seems to be exactly the opposite - such messages were available to the test with PhpUnit 9 (so we could test for them), and are no longer available (so we can't). I haven't even succeeded with a custom error message handler as part of the script. I will continue to investigate, but, for now, will skip some tests under PhpUnit 10 for the following:
- Shared/OleTest testChainedWriteMode and testChainedBadPath
- Reader/Html/HtmlLoadStringTest testLoadInvalidString
- Reader/Html/HtmlTest testBadHtml
* Scrutinize, and Parent Construct
Parent construct suggested by @MarkBaker.
* Redo Tests Dependent on Warning Messages
Warning (and other) messages are handled differently in PhpUnit 10 than in earlier versions.
Wanting a sanity test for ExactFontTest, I used the MD5 of a known file. However, there are various versions of the file in the wild, and use of these variants cause test to be skipped. Allow known variations to satisfy MD5 check.
* More Coverage In Unit Tests
Minimal source code changes.
* Scrutinizer
One legitimate complaint and one from out of left field.
* Scrutinizer Lunacy
If this doesn't stop its complaint, I give up.
* Scrutinizer - Whatever
Try again.
* Glutton For Punishment
Try again.
Fix#3405. Autosize is definitely not working well with CJK characters (column is not wide enough). User reports a workaround using `mb_strwidth` to calculate and set the column width. PhpSpreadsheet uses `mb_strlen` for width calculations. Change it to use mb_strwidth instead. For non-CJK strings, the results will be identical (and there are already unit tests on such strings which assert the expected results, and these tests did not need to change). For CJK strings, the results will be wider. The string I'm using to test comes from the issue. It currently results in a column width of 30.564. When I open the resulting sheet in Excel and auto-fit the column width, the width winds up as 43.00. So, as long as the computed width exceeds 43.00, the spreadsheet will show the full cell. With the new calculation, the computed width is 55.2722, satisfying our condition. This is wider than expected, but that is generally true for this type of computation. For example, for 'abcdefghijklmnopqrstuvwxyz', the computed width (before and after this change) is 31.7065, but Excel auto-fit actually uses 24.73.
Disappointingly, "exact width calculation" does not solve this problem. It does seem to do a little better than "approximate" for non-CJK, but its CJK calculation is not wide enough. This might or might not indicate a bug in Php function `imagegetttfbbox`; I do not know enough about it to report a bug. Anyhow, since we're dependent on that result, there is no equivalent in this case for swapping mb_strlen out for mb_strwidth.
* Resolve Phpstan Messages - FINALE - Shared/OLE
Reduce number of Phpstan messages by addressing their issues. This is the last of a series of related tickets to achieve that end. Only a handful of messages will be left in Phpstan baseline after this change is merged.
Shared/Ole.php and Shared/Ole/ChainedBlockStream.php were completely uncovered in the test suite. So I wrote some tests and found they didn't work at all. There were 2 problems. The first was that ChainedBlockStream checks for a mode of `r`, but Ole invokes it with mode `rb`. ChainedBlockStream will now just check the first character of the mode.
The second was a more interesting problem. Ole was reading "an *unsigned* long". However, what it should have been reading was "a 32-bit *signed* long". The irony of this mismatch is that, once the mode problem was corrected, 32-bit Php handled Ole and ChainedBlockStream correctly, but 64-bit Php did not. The code is corrected to work properly for both 32- and 64-bit.
* Scrutinizer
2 dead assignments - sufficient to just make calls which are expected to fail.
* Resolve Phpstan Messages Reader/Xls Shared/Escher
Reduce number of Phpstan messages by addressing their issues.
* Scrutinizer
Hyperactivity.
* More Scrutinizer
Getting close.
* Even More Scrutinizer
Closer and closer.
* Allow More Fonts/Fontnames for Exact Width Calculation
Fix#3190. A limited set of explicitly-named font files can be used when an exact width calculation is required. User noted that font files are named differently on Mac than on Windows (if the fonts are installed on Linux, the font names probably match Windows). Since the algorithm for generating the Mac file name from the font name seems easy, that algorithm is invoked when the Windows-named file is not found.
Moving on from there, it seems odd that only a small set of fonts are supported. It is, of course, impossible to support all possible fonts out of the box. However, it is possible to allow the user to supply additional mappings from font name to file name (or override existing mappings if neither the Windows nor Mac name matches the user's system), and doing so is permitted with this change:
```php
\PhpOffice\PhpSpreadsheet\Shared\Font::setExtraFontArray([
'fontname' => [ /* More than 1 can be specified */
'x' => 'fontfilenamefornormal.ttf',
'xb' => 'fontfilenameforbold.ttf',
'xi' => 'fontfilenameforitalic.ttf',
'xbi' => 'fontfilenameforbolditalic.ttf',
],
]);
```
* Unexpected Test Difference
... between Windows and Linux.
As was pointed out, the entire `pow` piece, as well as extracting exponent and exponent sign, was not necessary as casting to `float` will correctly handle scientific notation in a string.
Also added case-insensitive flag to the regexp to handle upper and lower case `e`.
Implemented the suggested named capture groups (nice trick) in the regexp to pull out only the values we need to perform the calculation. Making use of the null coalescing operator to keep things nice and clean. Note that I did have to add the `PREG_UNMATCHED_AS_NULL` to `preg_match` to ensure PHP returned null (since null coalescing only works on null and not '') when a match wasn't found for a capture group.
Also extended the regular expression to handle scientific notation (both positive and negative, and positive and negative exponents) as well as a negative symbol before or after a leading percentage sign.
As suggested, the initial check has been tightened up using a regular expression.
Expressing allows for leading and trailing spaces, leading and trailing percentage symbols, with spaces mixed throughout.
Added 57 permutations of leading and trailing spaces, leading and trailing percent symbols, decimals, and negatives. All seem to be working. I will also attach an Excel worksheet that verifies the behaviour of all 57 test cases aligns with Excel.
Adding two test cases:
1) Test the function in `StringHelper.php`. Modeled this test on the test for the `convertToNumberIfFraction` function.
2) Test a spreadsheet with a string percentage in a formula to see if it calculates the formula correctly.
* 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.
No source code changes. Mitoteam added a change to better accomodate Cygwin; pick up their new release. While testing, discovered one test that was already skipped on Windows and needs to be skipped on Cygwin as well.
* Floating-point Equality in Two Tests
Merging a change today, Git reported failures that did not occur during "normal" unit testing. The merge still succeeded, but ... The problem was an error comparing float values for equal, and the inequality occurred beyond the 14th decimal digit. Change the tests in question, which incidentally were not part of the merged changed, to use assertEqualsWithDelta.
* Egad - 112 More Precision-related Problems
Spread across 9 test members.
Shared/Font is hardly covered in unit tests (as opposed to Style/Font which is completely covered). And it presented some good opportunities for code optimization. I wrote and tested the new unit tests first, then optimized the code and confirmed that everything still works.
There is still a bit of a gap with "exact" measurements. I had tests ready, but had to withdraw them when I discovered they weren't quite portable (see https://github.com/php/php-src/issues/9073).
Fix#2897. We have been relying on iconv/mb_convert_encoding to detect invalid UTF-8, but all techniques designed to validate UTF-8 seem to accept FFFE and FFFF. This PR explicitly converts those characters to FFFD (Unicode substitution character) before validating the rest of the string. It also substitutes one or more FFFD when it detects invalid UTF-8 character sequences.
A comment in the code being change stated that it doesn't handle surrogates. It is right not to do so. The only case where we should see surrogates is reading UTF-16. Additional tests are added to an existing test reading a UTF-16 Csv to demonstrate that surrogates are handled correctly, and that FFFE/FFFF are handled reasonably.
* Eliminate Most Scrutinizer Problems in Test Suite
Mostly minor code changes, with some annotations.
* Missed 2 php-cs-fixer Problems
They should be fixed now.
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.
* 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.
* 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
oleibman
Adrien Crivelli
Mark Baker
fjohnston@avatarasoftware.com
MarkBaker
oleibman