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

Merging a single cell now throws exception #2776

Closed
1 of 8 tasks
mmarton opened this issue Apr 25, 2022 · 6 comments
Closed
1 of 8 tasks

Merging a single cell now throws exception #2776

mmarton opened this issue Apr 25, 2022 · 6 comments

Comments

@mmarton
Copy link

mmarton commented Apr 25, 2022

This is:

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

What is the expected behavior?

Merging a single cell does nothing.
I don't know if this is a legal use-case, but it was an undocumented breaking change for me. (I don't do this on purpose, I have a dynamic layout and sometime it happend.)
I already fixed with some if statements, just wanted you to know about this.

What is the current behavior?

Merging a single cell throws an exception. "Merge must be set on a range of cells.;

What are the steps to reproduce?

$sheet->mergeCells('A1:A1');

Please provide a Minimal, Complete, and Verifiable example of code that exhibits the issue without relying on an external Excel file or a web server:

<?php

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

// Create new Spreadsheet object
$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();

// add code that show the issue here...
$spreadsheet->getActiveSheet()->mergeCells('A1:A1');

What features do you think are causing the issue

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

Which versions of PhpSpreadsheet and PHP are affected?

latest (1.23.0)

@mmarton mmarton changed the title Merging a single cell not throws exception Merging a single cell now throws exception Apr 25, 2022
@MarkBaker
Copy link
Member

Hmmm! I don't see any change in behaviour between 1.22.0 and 1.23.0

$worksheet->mergeCells('A1:A1');

works in both versions (although it probably shouldn't)

$worksheet->mergeCells('A1');

throws an exception in both versions

@mmarton
Copy link
Author

mmarton commented Apr 25, 2022

Oh, i oversimplified my example.

my original code on 1.22.0 was:

$sheet->mergeCellsByColumnAndRow(72, 1, 72, 1);

the new one (as ByColumnAndRow was deprecated) on 1.23.0:

$sheet->mergeCells([72, 1, 72, 1]);

the array gets resolved to 'BT1' at vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Worksheet/Worksheet.php:1759 and throws exception at 1785

@MarkBaker
Copy link
Member

Ah! That makes things clearer. An unintended breaking change. My apologies.

As you've already fixed it (you could also have used try/catch and done nothing on the catch), I'm inclined to leave it "as is" and see if anybody else raises it. The code should prevent creating a single cell merge range anyway, as it does when a string address was passed in before; it's just that the now-deprecated "byColumnAndRow" method previously allowed the creation of a single cell range.

@mmarton mmarton closed this as completed Apr 25, 2022
@morungos
Copy link

To confirm, this exact issue broke my code too. This will cause old code to break. It might be sloppy old code, but it is not all that obscure. I have a feeling this will hit a good few people down the line.

@MarkBaker
Copy link
Member

@morungos So what would be your suggested approach? Do you think that the restriction should be relaxed to allow something that shouldn't have been permitted in the first place? Or perhaps that it should simply ignore a merge request for a single cell reference? Or are there options that I haven't thought about?

@pmishev
Copy link

pmishev commented Jun 9, 2022

I just hit the same problem. In my case I was generating a footer row for variable number of columns like:

$worksheet->mergeCellsByColumnAndRow(1, $row, $maxColumn, $row);

Yes, it can be worked around with an if or a try...catch, but it is a bit ugly and one may not even consider that this needs to be specifically handled, until it breaks in production.
In my opinion, the best way to handle this situation is just not do anything, instead of throwing an exception.
I don't think it's really semantically wrong to say you want to merge a cell with itself.

MarkBaker added a commit that referenced this issue Jul 9, 2022
Note that this will be the last 1.x branch release before the 2.x release. We will maintain both branches in parallel for a time; but users are requested to update to version 2.0 once that is fully available.

### Added

- Added `removeComment()` method for Worksheet [PR #2875](https://github.com/PHPOffice/PhpSpreadsheet/pull/2875/files)
- Add point size option for scatter charts [Issue #2298](#2298) [PR #2801](#2801)
- Basic support for Xlsx reading/writing Chart Sheets [PR #2830](#2830)

  Note that a ChartSheet is still only written as a normal Worksheet containing a single chart, not as an actual ChartSheet.

- Added Worksheet visibility in Ods Reader [PR #2851](#2851) and Gnumeric Reader [PR #2853](#2853)
- Added Worksheet visibility in Ods Writer [PR #2850](#2850)
- Allow Csv Reader to treat string as contents of file [Issue #1285](#1285) [PR #2792](#2792)
- Allow Csv Reader to store null string rather than leave cell empty [Issue #2840](#2840) [PR #2842](#2842)
- Provide new Worksheet methods to identify if a row or column is "empty", making allowance for different definitions of "empty":
  - Treat rows/columns containing no cell records as empty (default)
  - Treat cells containing a null value as empty
  - Treat cells containing an empty string as empty

### Changed

- Modify `rangeBoundaries()`, `rangeDimension()` and `getRangeBoundaries()` Coordinate methods to work with row/column ranges as well as with cell ranges and cells [PR #2926](#2926)
- Better enforcement of value modification to match specified datatype when using `setValueExplicit()`
- Relax validation of merge cells to allow merge for a single cell reference [Issue #2776](#2776)
- Memory and speed improvements, particularly for the Cell Collection, and the Writers.

  See [the Discussion section on github](#2821) for details of performance across versions
- Improved performance for removing rows/columns from a worksheet

### Deprecated

- Nothing

### Removed

- Nothing

### Fixed

- Xls Reader resolving absolute named ranges to relative ranges [Issue #2826](#2826) [PR #2827](#2827)
- Null value handling in the Excel Math/Trig PRODUCT() function [Issue #2833](#2833) [PR #2834](#2834)
- Invalid Print Area defined in Xlsx corrupts internal storage of print area [Issue #2848](#2848) [PR #2849](#2849)
- Time interval formatting [Issue #2768](#2768) [PR #2772](#2772)
- Copy from Xls(x) to Html/Pdf loses drawings [PR #2788](#2788)
- Html Reader converting cell containing 0 to null string [Issue #2810](#2810) [PR #2813](#2813)
- Many fixes for Charts, especially, but not limited to, Scatter, Bubble, and Surface charts. [Issue #2762](#2762) [Issue #2299](#2299) [Issue #2700](#2700) [Issue #2817](#2817) [Issue #2763](#2763) [Issue #2219](#2219) [Issue #2863](#2863) [PR #2828](#2828) [PR #2841](#2841) [PR #2846](#2846) [PR #2852](#2852) [PR #2856](#2856) [PR #2865](#2865) [PR #2872](#2872) [PR #2879](#2879) [PR #2898](#2898) [PR #2906](#2906) [PR #2922](#2922) [PR #2923](#2923)
- Adjust both coordinates for two-cell anchors when rows/columns are added/deleted. [Issue #2908](#2908) [PR #2909](#2909)
- Keep calculated string results below 32K. [PR #2921](#2921)
- Filter out illegal Unicode char values FFFE/FFFF. [Issue #2897](#2897) [PR #2910](#2910)
- Better handling of REF errors and propagation of all errors in Calculation engine. [PR #2902](#2902)
- Calculating Engine regexp for Column/Row references when there are multiple quoted worksheet references in the formula [Issue #2874](#2874) [PR #2899](#2899)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants