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

Getting a style for a CellAddress instance fails if the worksheet is set in the CellAddress instance #3439

Closed
AdamReece-WebBox opened this issue Mar 7, 2023 · 3 comments
Assignees

Comments

@AdamReece-WebBox
Copy link

There appears to be a bug when applying a number format to a Style instance when Style was created with a CellAddress instance that has got a Worksheet instance defined.

What is the expected behaviour?

Style instance to be retrieved for further use.

What is the current behaviour?

An exception is thrown due to "Invalid cell coordinate". Stack trace is as follows:

PhpOffice\PhpSpreadsheet\Exception:
Invalid cell coordinate 'CLIENT ACCOUNTS'!N2

  at vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Cell/Coordinate.php:42
  at PhpOffice\PhpSpreadsheet\Cell\Coordinate::coordinateFromString('\'CLIENT ACCOUNTS\'!N2')
     (vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Style/Style.php:218)
  at PhpOffice\PhpSpreadsheet\Style\Style->applyFromArray(array('numberFormat' => array('formatCode' => 'yyyy/mm/dd;@')))
     (vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Style/NumberFormat.php:186)
  at (blah blah Symfony framework stuff here)

What are the steps to reproduce?

First create an instance of a CellAddress class and specify its worksheet instance.
Then try to apply a number format to that cell by passing through the CellAddress instance.

/** @var \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet $worksheet */
$worksheet = ...;

// Make a `CellAddress` instance. This would usually be necessary for more complex iterators and such worksheet building code.
$cellAddress = new \PhpOffice\PhpSpreadsheet\Cell\CellAddress("A1", $worksheet);

// Grab a `Style` instance for that `CellAddress` to do whatever presentation necessary.
$style = $worksheet->getStyle($cellAddress);

// Crash here when applying a number format to the cell(s) in question.
$style->getNumberFormat()->setFormatCode(\PhpOffice\PhpSpreadsheet\Style\NumberFormat::FORMAT_DATE_YYYYMMDDSLASH);

This problem does not occur if you omit the worksheet instance when building the CellAddress instance, though you may need to reference the worksheet later.

What features do you think are causing the issue

Most likely Styles.

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

This happens when building a spreadsheet prior to a file save. We typically export as XLSX though I suspect this issue isn't related to the desired format of the resulting spreadsheet.

Which versions of PhpSpreadsheet and PHP are affected?

Tried on PhpSpreadsheet version 1.24.0. PHP version is 7.3 because "insert mundane project specific reasons here", though this issue doesn't appear to be related to the PHP version.

Looking through the same stack trace on the current version (1.28.0 at time of writing) the issue will occur here too as the relative code is unchanged.

On closer inspection

At the deepest point the issue occurs when \PhpOffice\PhpSpreadsheet\Cell\Coordinate::coordinateFromString() tries to parse a cell address that contains a worksheet, e.g. "'CLIENT ACCOUNTS'!N2".

Looking up the stack trace there are a number of opportunities in which the worksheet name could be cut out of the cell address, and I'm probably not best placed to decide where this would be best done. Essentially the following needs to happen somewhere:

if (false !== ($worksheetNameMarker = strrpos($cellAddress, '!'))) {
    $cellAddress = substr($cellAddress, $worksheetNameMarker + 1);
}

There are 2 reasonable places for this to happen:

  1. \PhpOffice\PhpSpreadsheet\Cell\Coordinate::coordinateFromString(): Immediately, before trying to parse the coordinate string.
  2. \PhpOffice\PhpSpreadsheet\Style\Style::applyFromArray(): Just after retrieving the selected cells? Though using variable name $pRange instead of $cellAddress.
    $pRange = $this->getSelectedCells();

I suspect the 2nd point (Style::applyFromArray()) would be most appropriate because Coordinate::coordinateFromString is not even expecting a range of cells at this deep point. This function already iterates over the range of cells (if a range was provided), so if the specific worksheet part of the coordinate is no longer necessary this would be a good chance to cut it.

Whichever position, this would correct "'CLIENT ACCOUNTS'!N2" to "N2" and all continues to work as expected.

@MarkBaker
Copy link
Member

Thanks for the diagnostic work with this issue.
You were right about the change to applyFromArray(), though it was slightly more complicated with Named Ranges

@AdamReece-WebBox
Copy link
Author

Thanks for submitting a fix for this.

Interesting, I hadn't spotted the existence of that trimSheetFromCellReference function, or the class its in. Another interesting utility class to look around. :)

@oleibman
Copy link
Collaborator

Closing, appears to have been resolved five months ago.

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

3 participants