* Scrutinizer Changes
Scrutinizer appears to be working again. But the PRs that have used it have neither added new issues nor fixed existing ones. This PR should fix some exisiting; let's see what Scrutinizer does with it.
* Address Some False Positives
In Reader/Xlsx/Chart.
* R1C1 Format and Internationalization, plus Relative Offsets
Fix#1704, albeit imperfectly. Excel's implementation of this feature makes it impossible to fix perfectly. I don't know why it was necessary to internationalize R1C1 in the first place - the benefits are so minimal,and the result is worksheets that break when opened in different locales. Ugh. I can't even find complete documentation about the format in different languages; I am using https://answers.microsoft.com/en-us/officeinsider/forum/all/indirect-function-is-broken-at-least-for-excel-in/1fcbcf20-a103-4172-abf1-2c0dfe848e60 as my definitive reference.
This fix concentrates on the original report, using the INDIRECT function; there may be other areas similarly affected. As with ambiguous date formats, PhpSpreadsheet will do a little better than Excel itself when reading spreadsheets with internationalized R1C1 by trying all possibilities before giving up. When it does give up, it will now return `#REF!`, as Excel does, rather than throwing an exception, which is certainly friendlier. Although read now works better, when writing it will use whatever the user specified, so spreadsheets breaking in the wrong locale will still happen.
There were some bugs that turned up as I added test cases, all of them concerning relative addressing in R1C1 format, e.g. `R[+1]C[-1]`. The regexp for validating the format allowed for minus signs, but not plus signs. Also, the relevant functions did not allow for passing the current cell address, which made relative addressing impossible. The code now allows these, and suitable test cases are added.
* Use Locale for Formats, but Not for XML
Implementing a suggestion from @MarkBaker to use the system locale for determining R1C1 format rather than looping through a set of regexes and accepting any that work. This is closer to how Excel itself operates. The assumption we are making is to use the first character of the translated ROW and COLUMN functions. This will not work for Russian or Bulgarian, where each starts with the same letter, but it appears that Russian, at least, still uses R1C1. So our algorithm will not use non-ASCII characters, nor characters where ROW and COLUMN start with the same letter, falling back to R/C in those cases. Turkish falls into that category. Czech uses an accented character for one of the functions, and I'm guessing to use the unaccented character in that case. Polish COLUMN function is NR.KOLUMNY, and I'm guessing to use K in that case.
The function that converts R1C1 references is also used by the XML reader *where the format is always R1C1*, not locale-based (confirmed by successfully opening in Excel an XML spreadsheet when my language is set to French). The conversion code now handles that distinction through the use of an extra parameter. Xml Reader Load Test is duplicated to confirm that spreadsheet is loaded properly whether the locale is English or French. (No, I did not add an INDIRECT function to the Xml spreadsheet.)
Tests CsvIssue2232Test and TranslationTest both changed locale without resetting it when done. That omission was exposed by the new code, and both are now corrected.
* OpenOffice and Gnumeric
OpenOffice and Gnumeric make it much easier to test with other languages - they can be handled with an environment variable. Sensibly, they require R and C as the characters for R1C1 notation regardless of the language. Change code to recognize this difference from Excel.
* Handle Output of ADDRESS Function
One other function has to deal with R1C1 format as a string. Unlike INDIRECT, which receives the string on input, ADDRESS generates the string on output. Ensure that the ADDRESS output is consistent with the INDIRECT input.
ADDRESS expects its 4th arg to be bool, but it can also accept int, and many examples on the net supply it as an int. This had not been handled properly, but is now corrected.
* More Structured Test
I earlier introduced a new test for relative R1C1 addressing. Rewrite it to be clearer.
* Add Row for This to Locale Spreadsheet
It took a while for me to figure out how it all works. I have added a new row (with English value `*RC`) to Translations.xlsx, in the "Lookup and Reference" section of sheet "Excel Functions". By starting the "function name" with an asterisk, it will not be confused with a "real" function (confirmed by a new test). This approach also gives us the flexibility to do something similar if another surprise case occurs in future; in particular, I think this is more flexible than adding this as another option on the "Excel Localisation" sheet. It also means that any errors or omissions in the list below will be handled as with any other translation problem, by updating the spreadsheet without needing to touch any code.
The spreadsheet has the following entries in the *RC row:
- first letter of ROW/COLUMN functions for da, de, es, fi, fr, hu, nl, nb, pt, pt_br, sv
- no value for locales where ROW/COLUMN functions start with same letter - bg, ru, tr
- no value for locales with a multi-part name for ROW and/or COLUMN - it, pl (I had not previously noted Italian as an exception)
- no value for locales where ROW and/or COLUMN starts with a non-ASCII character - cs (this would also apply to bg and ru which are already included under "same letter")
- it does nothing for locales which are defined on the "Excel Localisation" sheet but have no entries yet on the "Excel Functions" sheet (e.g. eu)
Note that all but the first bullet item will continue to use R/C, which leaves them no worse off than they were before this change.
* Memory Leak in Sample35
All but 6 chart samples can be rendered by Sample35. Of those 6, 3 of the problems are because the script runs out of memory processing them. Adopting a suggestion from @MAKS-dev in issue #2092, adding a call to gc_collect_cycles after the charts from each spreadsheet is rendered appears to make it possible to include those 3 spreadsheets in Sample35 after all.
Also take advantage of this opportunity to correct a number (hopefully all) of Scrutinizer problems with JpgraphRendererBases.
* Minor Fix
Problem running 8.1 unit tests.
* Resolve Problems with Pie 3D Charts
Minor fix, leaving only one spreadsheet unusable in Sample35. The reasons for its unusability are now documented in the code.
* Mitoteam Made Changes
Discussing this problem with them, they decided they should make a change for Pie3D rather than forcing us to use the workaround pushed earlier. Change to require mitoteam 10.2.3, revert workaround.
* 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.
Fix#1929. This was already substantially fixed, but there was a lingering problem with an unexpected leading space. It turns out there was also a problem with leading zeros, also fixed. There are also problems involving commas; fixing those seems too complicated to delay these changes, but I will add it to my to-do list.
They just released a Php8.2-compatible version. We should use that version going forward. Some tests had been disabled in 8.2 due to the problems which the new release fixes; these are now restored.
* More Scrutinizer Catch Up
Continue the work of PR #3043 by attending to the 12 remaining 'new' issues.
* Php 8.1 Problem
One new null-instead-of-string problem.
This PR expands on PR #2195 from @nkjackzhang. That PR has been stalled for some time awaiting requested fixes. Those fixes are part of this PR, and additional tests and samples are added. The original request was to handle `vertical-align:middle` in Css (Excel uses `center`). This PR does its best to also handle vertical alignment Excel values not found in Css - `justify` (as `middle`) and `distributed` (as `middle`). It likewises handles valid Css values not found in Excel (`baseline`, `sub`, and `text-bottom` as `bottom`; `super` and `text-top` as `top`; `middle` as `center`).
It also handles horizontal alignment Excel values not found in Css - `center-continuous` as `center` and `distributed` as `justify`; I couldn't think of a reasonable equivalent for `fill`, so it is ignored.
The values assigned for vertical and horizontal alignment are now lower-cased (special handling required for `centerContinuous`).
The last of these changes for now. No remaining Phpstan complaints in any Writer/Xlsx. Number of lines remaining in Phpstan baseline is now below 3500.
A lot of easily fixed problems throughout Writer/Xlsx/*, mostly supplying int rather than string as input to WriteAttribute/WriteElement. There are, in fact, so many opportunities, that I will split it over 2 or 3 PRs. But this first one will get Phpstan baseline down to the goal on its own.
Some of the other problems are also easily fixed. In particular, the docBlocks in Style/ConditionalFormatting/ConditionalDataBar do not allow for null values, and should.
For one of the Phpstan upgrades, some message text had changed so drastically that the only practical solution at the time was to move the messages from phpstan-baseline.neon to phpstan.neon.dist. This was not ideal, but it allowed us time to move on and study the errors, which I have now done. At one point, Parser is expecting a variable to be an array, and that was not clear from the code. If not an array, the code will error out (which was Phpstan's concern); I have changed it to throw an exception instead. This satisfies Phpstan, and I can get the message out of neon.dist (without needing to restore it to baseline). Unsurprisingly, the exception was never thrown in the existing test suite, although I added a couple of tests to exercise that code.
In Helper/Dimension, Phpstan flagged a statement inappropriately. I suppressed the message using an annotation and filed a bug report https://github.com/phpstan/phpstan/issues/7563. A fix for the problem was merged yesterday, which is good, but it puts us in a tenuous position. The annotation is needed now, but, when the fix is inevitably pushed to the version we use, the no-longer-needed annotation will trigger a different message. Recode so that neither the current nor the future versions will issue a message, eliminating the annotation in the process.
Fix#3011. Some properties for Trendlines were omitted in the original request for this feature. Also, the trendlines sample spreadsheet included two charts. The rendering script 35_Chart_render handles this, but overlays the first output file with the second. It is changed to produce files with different names.
When Reader/Xlsx/WorkbookView was split off from Reader/Xlsx.php, one statement accidentally brought `!empty($this->loadSheetsOnly)` with it. That property does not exist in WorkbookView, so the test is useless (it is always empty); and, in fact, the caller passes its own version of loadSheetsOnly as a parameter, so it isn't needed even it did exist. In Php8.2, this might be a deprecation, although it hasn't shown up in the GitHub 8.2 tests. Fix it anyhow.
* Charts - Add Support for Date Axis
Fix#2967. Fix#2969 (which had already been fixed prior to opening the issue, but had added urgency for Date Axes). Add ability to set axis type to date axis, in addition to original possiblities of value axis and category axis.
* Update 33_Chart_create_line_dateaxis.php
No idea why php-cs-fixer is complaining. It didn't do so when I first uploaded. I can't duplicate problem on my own system. Not enough detail in error message for me to act. Grasping at straws, I have moved the function definition (which is the only use of braces in the entire script) from the end of the script to the beginning.
* Update 33_Chart_create_line_dateaxis.php
Some comments were mis-aligned. This may be related to the reasons behind PR #3025, which didn't take care of this because this script had not yet been merged.
* Html Writer - Do Not Generate background-color When Fill is None
For PR #3002, I noted that there was a problem with Dompdf truncating images. I raised an issue with them (https://github.com/dompdf/dompdf/issues/2980), and they agree that there is a bug; however, they also suggested a workaround, namely omitting background-color from any cells which the image overlays. That did not at first appear to be a solution which could be generalized for PhpSpreasheet. However, investigating further, I saw that Html Writer is generating background-color for all cells, even though most of them use the default Fill type None (which suggests that background-color should not be specified after all). So this PR changes HTML Writer to generate background-color only when the user has actually set Fill type to something other than None. This is not a complete workaround for the Dompdf problem - we will still see truncation if the image overlays a cell which does specify a Fill type - however, it is almost certainly good enough for most use cases.
In addition to that change, I made the generated Html a little smaller and the code a little more efficient by combining the TD and TH styles for each cell into a single declaration and calling createCssStyle only once.
* Revamp One Test
Look for both td.style and th.style instead of just td.style in test.
The setup for unit testing in Github in the "Install dependencies" log reports 11 members as "does not comply with with psr-4 autoloading standard." In each case, it is because the test namespace does not match the directory; in most cases, it was caused by the member being moved from one directory to another without changing the namespace declaration. No harm results from these problems, but there's also no reason to not correct them.
* Upgrade Dev TCPDF to 6.5
Implementation of https://github.com/tecnickcom/TCPDF/pull/467, which is available in just-released Tcpdf 6.5, will improve look of Tcpdf rendering for PhpSpreadsheet. Fix#1164.
One test had been suppressed for Tcpdf, ostensibly because it was not compatible with Php8. As it turns out, the PhpSpreadsheet code which invokes Tcpdf was (harmlessly) incorrect, so the Php8 issue was actually with PhpSpreadsheet, not Tcpdf. That code is corrected, and the test is no longer suppressed.
* Update Change Log
Pick up some earlier changes as well as this one, and deprecations which had been omitted from the 1.24 change log.
oleibman
Mark Baker
Alexis Lefebvre