* 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`).
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.
* 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.
* Replace Dev jpgraph/jpgraph with mitoteam/jpgraph
PR #2979 added support for mitoteam/jpgraph as an alternative to jpgraph/jpgraph. The package jpgraph/jpgraph is abandoned in composer, and the version loaded with composer has been unusable for some time. This PR removes the dev requirement for jpgraph/jpgraph, and adds a dev requirement for mitoteam/jpgraph in its place.
With a usable graph library, a number of tests and samples that had been disabled are now re-enabled. A lot of new functionality has been added to Charts recently. Some of that new code has exposed bugs in JpgraphRendererBase. I have fixed those where I could. A handful of exceptions remain; I will investigate, and hopefully fix, those over time, but I don't feel it is necessary to fix them all before installing this PR - we are already way ahead of the game with the graphs that are working.
Three members had been ignoring code coverage in whole or in part because of the unavailability of a usable graph libray. Code coverage is restored in them. I am relieved to report that, although they aren't completely covered, adding them did not reduce code coverage by much - it is still over 90.4%.
I took a look at JpgraphRendererBase and Phpstan. Phpstan reports 128 problems. When I added some docblocks to correct some of those, the number increased to 284. Sigh. I will investigate over time, but, for now, we will still suppress Phpstan for JpgraphRendererBase.
I do not find a License file for mitoteam. However, there also wasn't one for jpgraph in the first place. Based on that and the discussion in #2996 (mitoteam will be used in exactly the same manner as mpdf), I don't think this is a problem. IANAL.
* PHP 8.2 Problems
Tons of "cannot create dynamic property" deprecations in jpgraph. Disable the test with most of those for now; leave the two with only a handful of messages enabled.
* Correct Failures in 2 Stock Charts
Down to 6 templates on which Render fails.
Addresses a problem identified in issue #2991. Chart name is set in constructor, but there is no method to subsequently change it. This PR adds a method to do so.
No source code changes, just additional tests. FormulaParser appears unused, replaced by newer code in Calculation. However, it's a public interface, so probably shouldn't be deleted without first deprecating it. I have no strong feelings about whether that should happen. However, as long as it's part of the package, we may as well have some formal unit tests for it.
This is a start in addressing issue #2965 (and earlier issue #749). Chart Titles are usually entered as strings or Rich Text strings, and PhpSpreadsheet supports that. They can also be entered as formulas (typically a pointer to a cell with the title text), and, not only did PhpSpreadsheet not support that, it threw an exception when reading a spreadsheet that did so.
This change does:
- eliminate the exception
- set a static chart title when it can determine it from the Xml
This change does not:
- fully support dynamic titles (e.g. if you change the contents of the source cell, or delete or insert cells or rows or columns)
- permit the user to set the title to a formula
- allow the use of formulas when writing a chart title to a spreadsheet
- provide styling for titles when it has read them as a formula
* Ensure multiplication is performed on a non-array value
* Simplify formula
Numbers should be numbers
* Provide test coverage for SUM combined with INDEX/MATCH
* PHPStan
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 Some Pdf Problems
Fix#1747. No support for text rotation in Pdf. That issue actually has a decent workaround, but PhpSpreadsheet should handle it on its own. Mpdf requires the proprietary text-rotate css attribute; Html and Dompdf will use the CSS3 attribute transform:rotate.
Fix#1713. Some paper-size values in PhpSpreadsheet are strings, some are 2-element float arrays. Dompdf accepts strings or 4-element float arrays, where the first 2 elements are always 0. Convert the PhpSpreadsheet array accordingly before passing it to Dompdf.
Some tests had been disabled when Dompdf and Tcpdf were slow to achieve PHP8 compliance. They achieved it some time ago. Re-enable the tests.
* Remove Tcpdf From One Test
No problem with the other tests I added it in for.
* Fix Spreadsheet Copy, Disable Clone, Improve Coverage
This PR was supposed to be merely to increase coverage in Spreadsheet. However, in doing so, I discovered that neither clone nor copy worked correctly. Neither had been covered in the test suite. Copy not only did not work, it broke the source spreadsheet as well. I tried to debug and got nowhere; I even tried using myclabs/deep-copy which is already in use in the test suite, but it failed as well. However, write and reload ought to work just fine for copy. It can't be used for clone; however, since copy does what clone ought to do, there's no reason why clone needs to be used, so __clone is changed to throw an exception if attempted.
One other source change was needed, an obvious bug where an if condition uses 'or' when it should use 'and'. Also, one docblock declaration needed a change. Aside from that, the rest of this PR is test cases, and overall coverage passes 89% for the first time.
* Clone is Okay After All
But copy wasn't, changing it to just return clone. Perhaps save and reload will be needed instead at some point, but not yet.
* An Error I Cannot Reproduce
PHP8.1 unit test says error because GdImage can't be serialized. I can't reproduce this error on any of my test systems. I have no idea why GdImage is even involved. Using try/catch to see if it helps.
* Weird Failures in Github
I thought restoring clone was a good idea. That left me in a state where, after one change, copy/clone no longer worked on Github (unable to reproduce on any of my test systems). After a second change, copy worked but clone didn't, again unable to reproduce. So, reverting to original version - copy does save and reload, clone throws exception.
Fix#2257. Fix#2929. Fix#2935 (probably in a way that will not satisfy the requester). 2257 and 2929 requested changes that ultimately affect the same section of code, so it's appropriate to deal with them together. 2257 requests the ability to make the chart background transparent (so that the Excel gridlines are visible beneath the chart), and the ability to hide an Axis. 2929 requests the ability to set a gradient background on the chart.
* Big Memory Leak in One Test
Many tests leak a little by not issuing disconnectWorksheets at the end. CellMatcherTest leaks a lot by opening a spreadsheet many times. Phpunit reported a high watermark of 390MB; fixing CellMatcherTest brings that figure down to 242MB. I have also changed it to use a formal assertion in many cases where markTestSkipped was (IMO inappropriately) used.
* Another Leak
Issue2301Test lacks a disconnect. Adding one reduces HWM from 242MB to 224.
Fix#2705. Add to Axis class, Reader Xlsx Chart, and Writer Xlsx Chart. Add feature to an existing 32* sample, to an existing 33* sample, and a formal unit test.
Fix#2934. Null is passed to StringHelper::strtolower which expects string. Same problem appears to be applicable to HLOOKUP.
I noted the following problem in the code, but will document it here as well. Excel's results are not consistent when a non-numeric string is passed as the third parameter. For example, if cell Z1 contains `xyz`, Excel will return a REF error for function `VLOOKUP(whatever,whatever,Z1)`, but it returns a VALUE error for function `VLOOKUP(whatever,whatever,"xyz")`. I don't think PhpSpreadsheet can match both behaviors. For now, it will return VALUE for both, with similar results for other errors.
While studying the returned errors, I realized there is something that needs to be deprecated. `ExcelError::$errorCodes` is a public static array. This means that a user can change its value, which should not be allowed. It is replaced by a constant. Since the original is public, I think it needs to stay, but with a deprecation notice; users can reference and change it, but it will be unused in the rest of the code. I suppose this might be considered a break in functionality (that should not have been allowed in the first place).
Fix#2931. If surface charts are written with c:grouping tag, Excel will treat them as corrupt. Also, some 3D-ish Xml tags are required when 2D surface charts are written, else they will be treated as 3D. Also eliminate a duplicate line identified by #2932.
* Html Reader Not Handling non-ASCII Data Correctly
Fix#2942. Code was changed by #2894 because PHP8.2 will deprecate how it was being done. See linked issue for more details. Dom loadhtml assumes ISO-8859-1 in the absence of a charset attribute or equivalent, and there is no way to override that assumption. Sigh. The suggested replacements are unsuitable in one way or another. I think this will work with minimal disruption (replace ampersand, less than, and greater than with entities representing illegal characters, then use htmlentities, then restore ampersand, less than, and greater than).
* Better Implementation
Use regexp to escape non-ASCII. Less kludgey, less reliant on the vagaries of the PHP maintainers.
* Additional Tests
Test non-ASCII outside of cell contents: sheet title, image alt attribute.
* Apply Same Change in Second Location
Forgot to change loadFromString.
* Additional Test
Confirm escaped ampersand is handled correctly.
This was supposed to be mopping up some longstanding chart issues. But one of the sample files exposed a memory leak in Xlsx Writer, unrelated to charts. Since that is my best sample file for this problem, I would like to fix both problems at the same time.
Xlsx Writer for Worksheets calls getRowDimension for all rows on the sheet. As it happens, the sample file had data in the last rows after a huge gap of rows without any data. It correctly did not write anything for the unused rows. However, the call to getRowDimension actually creates a new RowDimension object if it doesn't already exist, and so it wound up creating over a million totally unneeded objects. This caused it to run out of memory when I tried to make a copy of the 8K input file. The logic is changed to call getRowDimension if and only if (there is data in the row or the RowDimension object already exists). It still has to loop through a million rows, but it no longer allocates the unneeded storage.
As for the Chart problems - fix#1797. This is where the file that caused the memory leak originated. Many of its problems were already resolved by the earlier large set of changes to Charts. However, there were a few new properties that needed to be added to Layout to make things complete - numberFormat code and source-linked, and dLblPos (position for labels); and autoTitleDeleted needs to be added to Charts.
Also fix#2077, by allowing the format to be specified in the Layout rather than the DataSeriesValues constructor.
* Move Gridlines from Chart to Axis
This could, I hope, be my last major change to Chart for a while. When I first noticed this problem, I thought it would be a breaking change. However, although this change establishes some deprecations, I don't think it breaks anything. Major and minor gridlines had only been settable by the Chart constructor. This PR moves them where they belong, to Axis (eexisting Chart constructor code will still work). This allows them to be specified from both X and Y axis.
Chart is now entirely covered except for 2 statements, one deprecated and one that I just can't figure out. 99.71% for Charts, 88.96% overall. All references to the Chart directory in Phpstan baseline are eliminated.
* Minor Fixes, Unit Tests
Line style color type should default to null not prstClr.
Chart X-axis and Y-axis should alway be Axis, never null.
Add some unit tests.
* More Tests, Some Improvements
Make it easier to change line styles, adding an alternate method besides a setter function with at least a dozen parameters.
* Charts Additional Support for Layout and DataSeriesValues
The dLbls tag in more or less the Xml equivalent of the Layout class. It is currently read and written only for the Chart as a whole. It can, however, also be applied to DataSeriesValues. Further it has properties which are currently ignored, namely label fill, border, and font colors. All of these omissions are handled by this PR. There are other properties which can be applied to the labels, but, for now, only the 3 colors are added.
DataSeriesValues can have effects (like glow). Since DSV now descends from Properties, these are already supported, but support needs to be added to the Reader and Writer to handle them. This PR adds the support.
* Add Unit Tests
Based on new samples.
* Minor Improvements
Slight increase to coverage.
* Keep Calculated String Results Below 32K
This is the result of an investigation into issue #2884 (see also PR #2913). It is, unfortunately, not a fix for the original problem; see the discussion in that PR for why I don't think there is a practical fix for that specific problem at this time.
Excel limits strings to 32,767 characters. We already truncate strings to that length when added to the spreadsheet. However, we have been able to exceed that length as a result of the concatenation operator (Excel truncates); as a result of the CONCATENATE or TEXTJOIN functions (Excel returns #CALC!); or as a result of the REPLACE, REPT, SUBSTITUTE functions (Excel returns #VALUE!). This PR changes PhpSpreadsheet to return the same value as Excel in these cases. Note that Excel2003 truncates in all those cases; I don't think there is a way to differentiate that behavior in PhpSpreadsheet.
However, LibreOffice and Gnumeric do not have that limit; if they have a limit at all, it is much higher. It would be fairly easy to use existing settings to differentiate between Excel and LibreOffice/Gnumeric in this respect. I have not done so in this PR because I am not sure how useful that is, and I can easily see it leading to problems (read in a LibreOffice spreadsheet with a 33K cell and then output to an Excel spreadsheet). Perhaps it should be handled with an additional opt-in setting.
I changed the maximum size from a literal to a constant in the one place where it was already being enforced (Cell/DataType). I am not sure that is the best place for it to be defined; I am open to suggestions.
* Implement Some Suggestions
... from @MarkBaker.
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.
* Additional Support for Chart DataSeriesValues
Fix#2863. DataSeriesValues now extends Properties, allowing it to share code in common with Axis and Gridlines. This causes some minor breakages; in particular line width is now initialized to null instead of Excel's default value, and is specified in points, as the user would expect from Excel, rather than the value stored in Xml.
This change:
- adds support for 1 or 2 marker colors.
- adds support for `smoothLine` to DataSeriesValues.
- will determine `catAx` or `valAx` for Axis based on what is read from the Xml when available, rather than guessing based on format. (Another minor break.)
- reads `formatCode` and `sourceLinked` for Axis.
- correct 2 uses of `$plotSeriesRef` to `$plotSeriesIndex` in Writer/Xlsx/Chart.
- pushes coverage over 90% for Chart (88.70% overall).
* Update Change Log
I had updated previously but forgot to stage the member.
* Adopt Some Suggestions
Incorporate some changes suggested by @bridgeplayr.
* Use ChartColor for DSV Fill And Font Text
DataSeriesValues Fill could be a scalar or an array, so I saved it till last.
* Some Final Cleanup
No code changes.
Illustrate even more of the new features in existing sample files.
Deprecate *_ARGB in Properties/ChartColors in favor of *_RGB, because it uses only 6 hex digits. The alpha value is stored separately.
oleibman
MarkBaker
Jonathan Goode