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

Bug in ReferenceHelper::duplicateStylesByRow() #4246

Closed
8 tasks
patrickvuarnoz opened this issue Nov 27, 2024 · 2 comments · Fixed by #4247
Closed
8 tasks

Bug in ReferenceHelper::duplicateStylesByRow() #4246

patrickvuarnoz opened this issue Nov 27, 2024 · 2 comments · Fixed by #4247

Comments

@patrickvuarnoz
Copy link

This is:

- [x] a bug report
- [ ] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

What is the current behavior?

When trying to insert a large amount of rows via Worksheet::insertNewRowBefore() it will throw an exception saying something like Invalid cell coordinate AAAA1. Tracing the issue via stack trace showed that there is a bug in ReferenceHelper::duplicateStylesByRow():

    private function duplicateStylesByRow(Worksheet $worksheet, int $beforeColumn, int $beforeRow, string $highestColumn, int $numberOfRows): void
    {
        $highestColumnIndex = Coordinate::columnIndexFromString($highestColumn);
        for ($i = $beforeColumn; $i <= $highestColumnIndex; ++$i) {
            // Style
            $coordinate = Coordinate::stringFromColumnIndex($i) . ($beforeRow - 1);
            if ($worksheet->cellExists($coordinate)) {
                $xfIndex = $worksheet->getCell($coordinate)->getXfIndex();
                for ($j = $beforeRow; $j <= $beforeRow - 1 + $numberOfRows; ++$j) {
                    if (!empty($xfIndex) || $worksheet->cellExists([$j, $i])) {
                        $worksheet->getCell(Coordinate::stringFromColumnIndex($i) . $j)->setXfIndex($xfIndex);
                    }
                }
            }
        }
    }

The portion where it calls $worksheet->cellExists([$j, $i]) swaps the column and row numbers. It should be the other way around like $worksheet->cellExists([$i, $j]) because the iteration is over i=column and j=row.

Maybe the bug comes from the similar function ReferenceHelper::duplicateStylesByColumn() where this exact same line exists but there it is correct because the iteration is over j=column and i=row.

What are the steps to reproduce?

Tried to recreate a piece of sample code but did not get it done in a reasonable amount of time because it also depends on the Excel file and on the XfIndex (don't know what that is) of the cell. But the error is pretty obvious just looking at the code.

What features do you think are causing the issue

  • Reader
  • Writer
  • Styles
  • Data Validations
  • Formula Calculations
  • Charts
  • AutoFilter
  • Form Elements

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

Can't say.

Which versions of PhpSpreadsheet and PHP are affected?

Our version is 2.2.2, but looking at the code base at Github it is still present in the current version.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Nov 28, 2024
Fix PHPOffice#4246. This can cause an Exception in unusual circumstances.
@oleibman
Copy link
Collaborator

Thank your for the analysis. As you can see, I have a PR. I am normally reluctant to backport bug fixes from master to earlier branches; it is just too complicated and too time-consuming. That being said, the fact that you found this bug in an earlier release might make a difference. So, let me ask a couple of questions. What is keeping you from Release 3? And, if you must stay on release 2, why 2.2.2, which has some security exposures, rather than 2.3.3, which has patched them? If I do backport the fix, it will definitely be to the 2.3.* branch, not 2.2.*.

As for xfIndex, all the styles used in a spreadsheet are stored in an array. xfIndex is the index into that array for the cell in question. Cells with the same style should have the same xfIndex.

@patrickvuarnoz
Copy link
Author

Thanks for your quick action and response. Honestly, we didn't check composer since after the summer holiday break and by that missed all the updates to PHPSpreadsheet since then. No need to backport anything for us, we should be able to upgrade to the latest version as soon as the PR is included.

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