Fix for #1449 / Removing rows or columns that include range edges

[jabouillei]

• a bugfix
• a new feature

Checklist:

• Changes are covered by unit tests
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change
• Documentation is updated as necessary

Why this change is needed?

Removing rows or columns that overlap the edge of a range should adjust the edges of the range.
Also, formulas in column A should be adjusted even if only 5 columns are being removed starting with column G. (A formula in column A may refer to a range that includes column G.)

[jabouillei]

I was concerned that my patch/pull request might be causing errors with complex Excel files, but I am now confident that is not the case. The "unreadable content" is due to something that changed between 14.1 and 17.1. This pull request is solid. Unfortunately, I'll be returning to 14.1 because of the other issues, but please incorporate this pull request. Someone else will probably be helped by it at some point and it will avoid needing to merge again when I get back to PHPSpreadsheet someday. I was disappointed to find the fix was never incorporated in 14.1.

--- The rest of this comment is about the unrelated issue ---

I'm getting "unreadable content" errors regarding complex Excel files from our client when I use 17.1. (The client sends us an Excel file and we update it using PHPSpreadsheet and send it back.) I can't release our client's Excel file as a sample and I haven't been able to create a simple spreadsheet that exhibits the error, so I won't file a bug report. I put some hours into debugging, but it would take many more hours than I have available to spend on it. Perhaps someone reading this will understand - With 14.1, only clean definedNames ended up in the workbook.xml; with 17.1, ranges that PHPSpreadsheet didn't understand become "#REF!" or something involving "#REF!" and Excel reports that there is unreadable content. Excel fixes it, but it isn't presentable to clients.

Example: Excel produces a file that includes
<definedName name="ae">'[1]DataPg2-Universe'!$A$13</definedName>
We process the file and use PHP Spreadsheet to output it back into Excel and (using 17.1) it comes out with
<definedName name="ae">#REF!</definedName>
I reverted to 14.1 and the resulting workbook.xml was much smaller. All of the strange definedNames were removed. I have no idea why Excel put them in there in the first place. This smells to me like a problem with Excel including garbage in a file and 17.1 converting it to worse garbage while 14.1 stripped it.
I tried simply skipping any defined names that include #REF! in Writer/Xlsx/DefinedNames.php, but that wasn't enough to make Excel happy.

[MarkBaker]

Is it possible to provide unit tests to prove that these changes actually work as expected. I know that the ReferenceHelper isn't well covered by unit tests at the moment; but this is quite a significant change, making it difficult to assess

[truthptn]

We've been using this patch for years. I do have a small unit test that I used to kick the tires when attempting to use 17.1.

<?php
require_once('PHPSpreadsheet/autoload.php');
require_once('PHPSpreadsheet/phpoffice/phpspreadsheet/src/PhpSpreadsheet/IOFactory.php');
$objReader = PhpOffice\PhpSpreadsheet\IOFactory::createReader('Xlsx');
$objReader->setIncludeCharts(true);
$content = $objReader->load('mergedtest2021.xlsx');
foreach ($content->getWorksheetIterator() as $worksheet) {
    $worksheet->removeColumn('F',4);
    //$worksheet->removeRow(3,3);
}
$objWriter = PhpOffice\PhpSpreadsheet\IOFactory::createWriter($content, 'Xlsx');
$objWriter->setPreCalculateFormulas(false);
$objWriter->setIncludeCharts(true);
$objWriter->save('mergedtest2021_after.xlsx');
?>

I suspect the "setIncludeCharts()" is not relevant and is just there because my original testing was with a client file that included charts and this test is based on the original test from when I first wrote the patch for PHPExcel.

Here is the simple little Excel file for use with the test case:
mergedtest2021.xlsx

[PowerKiKi]

This need to support PHP 8, pass CI and include at least some basic unit tests to be merged

[jabouillei]

I probably shouldn't have, but I laughed when I saw this.  The idea of updating a pull request that has been around this long to php 8!  I imagine someday I'd get an email requesting that it be updated to php10.  The bottom line is PHPSpreadsheet doesn't handle certain actions involving deleting rows and columns correctly and I fixed it years ago.  I fixed it as part of my job for Proposal Tech and made the fix available for the community years ago.  If the community would rather have the broken behavior, I wash my hands of it. - Todd On Friday, October 29, 2021, 10:51:15 PM PDT, Adrien Crivelli ***@***.***> wrote: @PowerKiKi requested changes on this pull request. This need to support PHP 8, pass CI and include at least some basic unit tests to be merged — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[oleibman]

Closing. Superseded by 3528.