Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Row and column integer error when calling IOFactory::load #3305

Closed
W1DJM opened this issue Jan 19, 2023 · 4 comments · Fixed by #3306
Closed

Row and column integer error when calling IOFactory::load #3305

W1DJM opened this issue Jan 19, 2023 · 4 comments · Fixed by #3306

Comments

@W1DJM
Copy link

W1DJM commented Jan 19, 2023

This is a bug report.

What is the expected behavior?

The file should load to be processed further.

What is the current behavior?

An error is thrown when load is called.

Error 0 on line 49 in \PhpSpreadsheet\Cell\CellAddress.php -> Row and Column Ids must be positive integer values

What are the steps to reproduce?

This is not happening with every file but files that used to load without error are now causing the error shown above. So far the files that are failing are all XLS. There has been limited use with XLSX and, so far, no failures with XLSX. I believe it is related to the file structure or something within the file itself as I can copy and paste the rows into a newly created file and it will load and process correctly.

<?php
require __DIR__ . '/vendor/autoload.php';

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
$testAgainstFormats = [\PhpOffice\PhpSpreadsheet\IOFactory::READER_XLS,
                                     \PhpOffice\PhpSpreadsheet\IOFactory::READER_XLSX];
$spreadsheet = \PhpOffice\PhpSpreadsheet\IOFactory::load('FailTestShort.xls',0,$testAgainstFormats);  // <<-- file attached - has only 2 rows

### What features do you think are causing the issue

- [X] Reader (and/or possibly IOFactory::load

### Does an issue affect all spreadsheet file formats? If not, which formats are affected?

Right now it only seems to be affecting XLS format files.

### Which versions of PhpSpreadsheet and PHP are affected?

PHP 8.2.1 and a fresh copy of PhpSpreadsheet that was pulled today (19 Jan 2023).

[FailTestShort.xls](https://github.com/PHPOffice/PhpSpreadsheet/files/10460781/FailTestShort.xls)
@oleibman
Copy link
Collaborator

Thank you for the sample file. I am able to duplicate your error. It appears that the code which is causing the failure is not executed in our test suite. It also includes the comment "not sure why two row indexes are necessary?", and, based on this report, that seems like a good question. I am researching how best to fix this.

@W1DJM
Copy link
Author

W1DJM commented Jan 20, 2023

Thanks for the quick response. Do you know why some files work fine but others, like the sample I provided, don't?

@oleibman
Copy link
Collaborator

It depends on what data the spreadsheet provided. The data in your spreadsheet indicated a a vertical break before cell H0, which is not a valid reference. I am not sure what use, if any, is made of the row number for a vertical break; Excel clearly tolerates row 0 for those. The exact cause of your issue is (a) we deprecated all byColumnAndRow functions including setBreakByColumnAndRow; (b) we changed all our byColumnandRow code to use the non-deprecated routines passing an array rather than a string; (c) the validation logic accepts all addresses passed as strings but does an additional test when the address is passed as an array which resulted in an exception for the invalid address. This difference would normally have been caught in unit testing, but the section of code which reads page breaks in Xls files happened to be uncovered (we have about 91% coverage now).

TLDR - the files which work without problems either don't specify a vertical break, or specify it with a row > 0.

@W1DJM
Copy link
Author

W1DJM commented Jan 20, 2023

Well I'll be. I tried resetting all of the breaks on the file that was failing and it worked just fine. I wouldn't have thought to do that in a million years. Looking forward to a new drop to test with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants