[confusing] getStyle sets current selected fields

[chielsen]

As I only found out when looking at the source code, this doesn't work as expected:

<?php

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

// Create new Spreadsheet object
$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();

$first = $sheet->getStyle('A1:A10');
$second = $sheet->getStyle('A1:B10');

$first->applyFromArray(
	[
		'fill' => [
			'fillType'   => \PhpOffice\PhpSpreadsheet\Style\Fill::FILL_GRADIENT_LINEAR,
			'rotation'   => 90,
			'startcolor' => [
				'argb' => 'FFFFFF',
			],
			'endcolor'   => [
				'argb' => 'FFA0A0A0',
			],
		]
	]
);
$second->applyFromArray(
	['fill' =>
		 [
			 'fillType' => \PhpOffice\PhpSpreadsheet\Style\Fill::FILL_SOLID,
			 'color'    => ['argb' => 'FFe7c16a'],
		 ]
	]
);
$second->applyFromArray(['fill' =>
[
				'fillType' => \PhpOffice\PhpSpreadsheet\Style\Fill::FILL_SOLID,
				'color' => ['argb' => 'FFe7c16a'],
			]
]);

The reason is that getStyle set the selected range. So when we do the second getStyle it overrides the first call. This is very confusion, especially since the name is getStyle

Proposal

It would be nice if the returned object holds the affected cells.
If that is too complicated, maybe just rename getStyle, so that it is clear it sets the selected cells.

Which versions of PhpSpreadsheet and PHP are affected?

1.0.0

[PowerKiKi]

Your example would be better expressed as:

<?php

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

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Style\Fill;

$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();

$first = $sheet->getStyle('A1');
$second = $sheet->getStyle('A2');

$first->getFill()->setFillType(Fill::FILL_SOLID);

var_dump([
    $sheet->getStyle('A1')->getFill()->getFillType(),
    $sheet->getStyle('A2')->getFill()->getFillType(),
]);

Expected output would be:

array(2) {
  [0] =>
  string(5) "solid"
  [1] =>
  string(4) "none"
}

But actual output is:

array(2) {
  [0] =>
  string(4) "none"
  [1] =>
  string(5) "solid"
}

So I agree it can be confusing. It means that you should use getStyle() and immediately after actually set styles on it. I would review a PR to change this behavior, but this is will be non-trivial because Style is more of a singleton than a real instance of style for each cells/ranges. Alternatively I would accept a PR to clarify documentation where needs be.

[chielsen]

What do you guys prefer: Storing the selected fields in de returned object?
I didn't look at the rest of the code if that would cause a lot of issues.

[PowerKiKi]

I think keeping the same API and "only" changing the behavior would be the best, because it avoids breaking change and the API seems intuitive enough, except for the case described here. But that probably won't be a simple task. I'll let you have a better look at the code and we might talk more about it if/when you have an idea on how to do things

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

[oleibman]

A method to avoid this issue was merged 2 weeks ago by PR #4073.