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

Conditional formatting overwrites the format when no format is set #3202

Closed
1 of 8 tasks
ndench opened this issue Nov 23, 2022 · 2 comments · Fixed by #3372
Closed
1 of 8 tasks

Conditional formatting overwrites the format when no format is set #3202

ndench opened this issue Nov 23, 2022 · 2 comments · Fixed by #3372

Comments

@ndench
Copy link
Contributor

ndench commented Nov 23, 2022

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 expected behavior?

Conditional formats should not be changed when reading then writing an excel file.

What is the current behavior?

Reading an excel file that contains a conditional formatting rule, where the format is No format set, and then writing that file immediately results in the rule being changed to have a format.

Having No format set is a valid rule because it's the easiest way to not show any conditional formatting when the cell is blank.
ie. Have a rule for value > 0, another for value < 0, then have a No format set for value is blank that has stop if true enabled.
See "Method 1" on this post.

What are the steps to reproduce?

<?php

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

// Read the excel file that has a single formatted cell
$reader = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx();
$excel = $reader->load('no-format.xlsx');

// Immediately write the excel file back out without making any changes
$writer   = new \PhpOffice\PhpSpreadsheet\Writer\Xlsx($excel);
$writer->save('no-format-output.xlsx');

Here is the original file that has no content, and only a a single conditional formatting rule for A1 contains a blank value with the format Not format set
no-format.xlsx
no-format

Here is the output file after running the above script, you can see the conditional formatting rule now has a format of light red fill, dark red text.
no-format-output.xlsx

no-format-output

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?

  • XLSX

Which versions of PhpSpreadsheet and PHP are affected?

  • PHP 8.1.2
  • PhpSpreadsheet 1.25.2 (1.23.0 also affected)

Anything else?

I love PhpSpreadsheet and I'm more than happy to help debug this further/fix the issue if someone can point me in the right direction. I've tried stepping through with xdebug but it's kinda hard to follow and locate the issue.

@MarkBaker
Copy link
Member

This is new for me, I hadn't realised that this was possible in Excel.
When we create a Conditional Formatting Rule, we always create a Style object, whether when loading a file, or when creating a CF Rule within PHP Code; and the Writers expect that Style object when saving the file.

Clearly this is a bug; and the Style should be nullable. It's a fix that will have to be done in several small steps; the internal representation will have to allow for nullable style, and all of the Writers as step #1, with the Readers as step #2, and these can be done individually - so it will likely be at least 2 releases before we can implement this fully.

@ndench
Copy link
Contributor Author

ndench commented Nov 24, 2022

Thanks for the response @MarkBaker. Are these changes something I can help with?

@MarkBaker MarkBaker self-assigned this Nov 26, 2022
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Feb 17, 2023
Fix PHPOffice#3370. Conditional styles are always generated with 5 borders (right, left, top, bottom, diagonal) even though the border style is none in each case. For the spreadsheet in question, top and bottom were inappropriate and interfered with the desired formatting. A new border style, BORDER_OMIT is added which will cause the Xlsx Writer to not generate that style. All conditional borders will be initialized with that value. Any border included in the Xml will, of course, change it to the specified type.

Fix PHPOffice#3202. User wants a condition to use "No format set" as you can in Excel. A new boolean property `$noFormatSet`, along with setter and getter, is added to Style/Conditional. It is initialized to false. User can call setter to change it. More importantly for the issue in question, if the Xlsx Reader encounters a `cfRule` tag which does not have a `dxfId` attribute (i.e. no style is associated with the rule), it will set noFormatSet to true. Similarly, the Xlsx writer will not generate a `dfxId` tag when noFormatSet is true.

This change is applicable only to Xlsx. Html, Csv, and Ods do not have support for Conditional Formatting. Limited support was added to Xls with PR PHPOffice#2696 in April 2022 and PR PHPOffice#2702 about a month later. However, with the current release code, Xls equivalents of the two new test spreadsheets in this PR are too complicated to be handled correctly by PhpSpreadsheet - loading and then saving them as Xls results in Excel complaining of corruption, and the results don't meet expectations. Since I have no idea how BIFF works, and since the problems with those spreadsheets are not caused by this PR, I am not planning to address those problems at this time.
oleibman added a commit that referenced this issue Feb 24, 2023
* WIP Conditional Formatting Improvements for Xlsx

Fix #3370. Conditional styles are always generated with 5 borders (right, left, top, bottom, diagonal) even though the border style is none in each case. For the spreadsheet in question, top and bottom were inappropriate and interfered with the desired formatting. A new border style, BORDER_OMIT is added which will cause the Xlsx Writer to not generate that style. All conditional borders will be initialized with that value. Any border included in the Xml will, of course, change it to the specified type.

Fix #3202. User wants a condition to use "No format set" as you can in Excel. A new boolean property `$noFormatSet`, along with setter and getter, is added to Style/Conditional. It is initialized to false. User can call setter to change it. More importantly for the issue in question, if the Xlsx Reader encounters a `cfRule` tag which does not have a `dxfId` attribute (i.e. no style is associated with the rule), it will set noFormatSet to true. Similarly, the Xlsx writer will not generate a `dfxId` tag when noFormatSet is true.

This change is applicable only to Xlsx. Html, Csv, and Ods do not have support for Conditional Formatting. Limited support was added to Xls with PR #2696 in April 2022 and PR #2702 about a month later. However, with the current release code, Xls equivalents of the two new test spreadsheets in this PR are too complicated to be handled correctly by PhpSpreadsheet - loading and then saving them as Xls results in Excel complaining of corruption, and the results don't meet expectations. Since I have no idea how BIFF works, and since the problems with those spreadsheets are not caused by this PR, I am not planning to address those problems at this time.

* Update Documentation, Write Alignment and Font Less Often

It doesn't cause any particular harm except for small increases in file size and run time, but Alignment tags are written even when (a) all its attributes are null for Conditional Formatting, and (b) when the xml specifically indicates that Alignment should not be applied. Similarly, Font is written even when all its attributes are null for Conditional Formatting.

There are some errors in the Conditional Formatting documentation. Specifying a solid fill color in a Conditional Style requires the use of endColor, not StartColor. The discussion of Order of Evaluating is not entirely accurate. I have changed it to what I believe is an accurate explanation of how Excel works; and also added a mention that other spreadsheet programs might not work the same way, adding a couple of illustrations of the difference. The description of the multiple conditions did not quite match the diagram. 'Stop if true' was a blank paragraph; it is now described, and the new 'No format set' option is described in that paragraph since (I think) it would be used most often in conjunction with 'Stop if true'.

* Xlsx Writer Allow StartColor for Conditional Solid Fill

To set a solid fill in a non-conditional style, you set StartColor (xml will use that value as fgColor and a default value as bgColor). If you instead set EndColor (xml will use that value as bgColor and a default value as fgColor), the styling will not work as expected.

However, for conditional styles, if you set StartColor (xml will use that value as fgColor and not specify bgColor), the styling will not work as expected. If you instead set EndColor (xml will use that value as bgColor and not specify fgColor), the styling will work as expected.

Together, this means that you need to use different methods for non-conditional style fill than for conditional style fill. This isn't a big problem, but it is a bit weird. This PR changes Xlsx Writer so that if (a) fill is olid and (b) startColor is specified and (c) endColor is null, the xml will be written as bgColor without specifying fgColor. This means that you can set StartColor for both conditional and non-conditional and get the expected styling. You may, of course, continue to specify EndColor instead for conditional.

* Fix Some (Not Many) Xls Problems

I will open an issue for the (pre-existing) remainder.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants