Because they tend to clutter our code a lot and unfortunately, there are
lots of false positives. Instead, it would probably be better to deal
with false positives out of band, via the Scrutinizer web UI.
Fix#3739. User is seeing an intermittent notice when running samples using a web browser. The notice is comming from ob_clean, complaining that there is no buffer to delete. I frankly do not understand what the ob_clean is supposed to be paired with, nor why I cannot duplicate this result. Possibly, a better solution would be to eliminate the ob_clean; but using ob_get_length beforehand to see if there is anything to clean up seems safer, and I can't think of a downside.
The code in question is never executed when a sample is run from the command line. Consequently, no formal unit test is possible. The user reporting the problem was asked to test the change, and confirmed that the problem went away; no new problem arose on my system.
* fix: Load tables when flag IReader::READ_DATA_ONLY is used
* Add test for table read when IReader:::READ_DATA_ONLY is used
---------
Co-authored-by: Kevin Verschaeve <kevin.verschaeve@exotec.com>
* Break Some More Circular References
After identifying a circular reference between Worksheet and Table, which could lead to memory leaks, I identified several other similar relationships. I have added a destructor, or added added code to an existing destructor, for those possibilities. This didn't actually lead to any lowering of the memory usage of the unit test suite, so maybe there's no practical benefit, but it does seem like it should be theoretically useful. I am not aware of any practical way to unit test a destructor, although the added code will be exercised many times in most of our unit tests.
The only change here not involving a destructor is to `Worksheet::addChart` (it came to my attention because Worksheet and Chart have a circular reference). It is currently defined with two arguments, the second allowing you to place the chart in a specific location in the containing ArrayObject. However, as Phpstan and Scrutinizer both warn, the code to support this option will fail if it is ever executed (can't use array_splice on an ArrayObject); needless to say, this possibility is not exercised when executing the test suite. It could be made to work, but I really can't see any kind of use case to support this parameter. So I'm removing the second parameter. In theory, this is a breaking change, but, since the code can never work, in practice it won't be.
* Update Worksheet.php
* Update Worksheet.php
* Add iterateOnlyExistingCells to Constructors
Fix#3721. That issue can already be handled, but requires several statements when one ought to suffice. Adding an extra parameter to the RowCellIterator and ColumnCellIterator constructors is useful, easy, and becomes especially practical now that our supported Php releases all support named parameters (see new test Issue3721Test).
* Update Column.php
* Update .php-cs-fixer.dist.php
* Update Row.php
* Update Column.php
* Update ByColumnAndRowTest.php
* Unexpected Namespacing in rels File
Fix#3720. Third-party product created a spreadsheet which PhpSpreadsheet could not read because of unexpected namespacing in workbook.xml.rels.
The file which demonstrated the problem was attached to #3423, however I do not believe it was related to the original problem. Nevertheless, the original issue specifically called out Protection, so I put some Protection tests in the validation test for the fix. In doing so, I found that Style/Protection is particularly confusing. Its properties will often have the value `inherit`, which isn't all that helpful; and, even when the `locked` value is `protected`, the cell won't actually be locked unless the sheet is protected as well. The `hidden` property is even more obscure - it applies only to formulas, and refers to hiding the property on the formula bar, not in the cell. I have added methods `isLocked` and `isHiddenOnFormulaBar` to `Cell`. I corrected the docs to explain this. And, as long as I was looking at the docs, I corrected some examples to use `getHighestDataRow/Column` rather than `getHighestRow/Column`, a frequent problem for users (e.g. #3721).
As a side note, the change to Cell.php is my first use of the nullsafe operator. This is one of many new options available now that we require Php8.0+.
* Minor Simplifications
* Scrutinizer
It's being silly again. In many tests, we test a variable for non-null, then use that variable later and Scrutinizer knows it's not null. Not here. Oh well.
* Add Methods
Test if protected without allocating cell if it doesn't exist.
PHPDoc types can sometimes be entirely expressed as PHP native types. It
is better because it avoids code duplication and enables type runtime
check.
This will help us slowly migrate away from PHPDoc typing to PHP native typing.
* Inconsistency Between Actual and Declared Type - Minor Break
Fix#3711. User set a cell value to float (implicitly by default value binder), then used `setDataType` to change its type to string. This caused a problem for Xlsx Writer, which uses the string cell values as an index into a Shared String array. However, as the cell actually contained a floating point value, Php treated it as an integer index; such a treatment is both deprecated, and leads to invalid values in the spreadsheet.
The use case for `setDataType` is not strong. The user always has the option to use `setValueExplicit` if the type is important. Setting a type afterwards, i.e. irrespective of the value, seems like a peculiar action. Indeed, there are no tests whatever for such use in the unit test suite.
There are two possible approaches to fixing this problem. The first is to add casts to the 3 or 4 places in Writer Xlsx which might be affected by this problem (hoping that you've found them all and realizing that similar changes might be needed for other Writers). The second is to change `setDataType` to call `setValueExplicit` using the current value of the cell, thereby possibly changing the cell value. I have gone with the second option - it seems like a much more logical approach, and guarantees that the content of the cell will always be consistent with its declared type. It is, however, a breaking change; if, for example, you have a cell with a string or numeric value and specify `boolean` to `setDataType`, the cell's value will change to `true` or `false` with no way to get back to the original.
* Strict Types for New Tests
Consistent with work being done in PR #3718.
* Improve Test
Better match to original issue.
* Typo
* ListWorksheetInfo/Names for Html/Csv/Slk
Fix#3706. ListWorksheetInfo is implemented for all Readers except Html. For most (not all), ListWorksheetInfo is more efficient than reading the spreadsheet. I can't think of a way to make that so for Html, but that shouldn't be a reason to leave it unimplemented.
ListWorksheetNames is not implemented for Html, Csv, or Slk. It isn't terribly useful for those formats, but that isn't a reason to omit it. The requester's use case consists of using IOFactory to create a reader for a file of unknown format and determining the first sheet name. That seems legitimate, but it is currently not possible without extra user code if the file is Html, Csv, or Slk; this PR will make it possible.
When Excel opens a Slk or Csv file, the sheet name is based on the file name. PhpSpreadsheet does this for Slk, but it uses a default name for Csv. I am not interested in creating a break for that behavior, but I have added a new boolean property `sheetNameIsFileName` with a setter to Csv Reader. The requester actually mentioned that possibility in our discussion, although it is not essential to the request.
As an adjunct to the issue, the requester wishes to use the worksheet name in `setLoadSheetsOnly`. That is already possible for Html, Csv, and Slk, but that particular property is ignored for those formats. I do not see a reason to change that behavior. This treatment is now explicitly noted in the documentation for property `loadSheetsOnly`.
There had been no tests for what happens when `loadSheetsOnly` is specified but no sheets match the criteria for the formats for which this makes sense (Xlsx, Xls, Ods, Gnumeric, Xml). The behavior was not consistent - some formats threw an Exception while others continued with a single empty worksheet. All cases attempt to set the active sheet, and they will now all throw identical Exceptions when they attempt to do so in this situation. Tests are added for each.
There also had been no tests for `loadSheetsOnly` returning more than one sheet. One is added.
* Update LoadSheetsOnlyTest.php
Add strict types to this new test, consistent with work being done in PR #3718.
* Update LoadSheetsOnlyTest.php
Add strict types to this new test, consistent with work being done in PR #3718.
Adrien Crivelli
oleibman
jenasnic
Progi1984
Kevin Verschaeve
dmytromikhieiev1985