Overlapping conditional formatting rules get overwritten based on the cell coordinate.

[Awilen-Bernkastel]

This is:

• 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 formatting rules overlapping the exact same cell range coordinates definition in an Xlsx file get added to the Worksheet's conditionalStylesCollection property.

What is the current behavior?

Conditional formatting rules overlapping the exact same cell range coordinates definition in different definitions in an Xlsx file get overwritten in the Worksheet's conditionalStylesCollection property.

What are the steps to reproduce?

Open and Save a Worksheet that has overlapping conditional styles on the same cell range definition, but spanned over different sqref.

The behavior can be found in /src/Reader/Xlsx/ConditionalStyles.php, in the method ConditionalStyles::setConditionalStyles(Worksheet $worksheet, array $conditionals, SimpleXMLElement $xmlExtLst).

The use of explode doesn't allow taking into account different cell range definitions that span the same range. Having two rules spanning for one "E2:H3" and for the other "E2:H3 E5:H6" will overwrite the conditional formatting of the first rule.

Here is a file showing conditional formatting without errors, that will error out after saving it with PhpSpreadsheet:
test_overlapping_coordinates.xlsx

A dirty fix of array_merge'ing the array of conditionals Worksheet::conditionalStylesCollection with the incoming $styles instead of assigning the incoming array of $styles in Worksheet::setConditionalStyles() fixes the issue for me.

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?

I do not know if this affects any other file format than Xlsx.

Which versions of PhpSpreadsheet and PHP are affected?

PhpSpreadsheet: 1.29.8
PHP: 8.1

[oleibman]

I believe that not-yet-merged PR #4314 addresses this problem. If you can test against that, please do and let me know the results.

[Awilen-Bernkastel]

I do not believe so. I am also the author of ticket #4312 mentioned in the PR you quoted. I have applied the PR to the version I pulled with Composer and it did fix the priorities issue, but not the issue I am raising in this ticket here.

[oleibman]

Thank you for pointing out the distinction. This could be tougher to solve than the priority problem, but it might be able to build on what I'm doing with Data Validations (PR #4240), whose ranges can be specified in the same manner. I will take a look. Depending on how complicated it seems, It is possible that I will merge the priority change (and the DV change) before I have figured this one out.

[oleibman]

Please try again. I (hope I) have added the necessary code to 4314 to address this problem. I think this will make it close enough to a breaking change that it needs to go in at the same time as the DV changes, or else will have to wait a long time. I also added a change to Worksheet::getConditionalStyles so that you can specify a parameter to return all matching rules, and in priority order.

[Awilen-Bernkastel]

I'm afraid the changes to Worksheet::getConditionalStyles do not fix the issue at hand.

I hope you don't mind me sharing my analysis of the code pathway that in my opinion, leads to the issue I'm experiencing.

The issue starts in /Reader/Xlsx.php, in the method loadSpreadsheetFromFile, on line 811

(new ConditionalStyles($docSheet, $xmlSheet, $dxfs))->load();

In the load() method from /Reader/Xlsx/ConditionalStyles.php, there is a call to $this->readConditionalStyles($this->worksheetXml) that lists the cfRules like so:

$conditionals[(string) $conditional['sqref']][(int) ($cfRule['priority'])] = $cfRule;

meaning each raw sqref gets a reference in $conditionals. Let's say like I mentioned in the opening post, that we have the following two raw sqrefs: "E2:H3" and "E2:H3 E5:H6", then both raw sqrefs will be included in $conditionals, as-is.

This call to readConditionalStyles is included as an argument to setConditionalStyles:

$this->setConditionalStyles(
     $this->worksheet,
     $this->readConditionalStyles($this->worksheetXml),
     $this->worksheetXml->extLst
);

In setConditionalStyles, there is this logic:

// Extract all cell references in $cellRangeReference
$cellBlocks = explode(' ', str_replace('$', '', strtoupper($cellRangeReference)));
foreach ($cellBlocks as $cellBlock) {
    $worksheet->getStyle($cellBlock)->setConditionalStyles($conditionalStyles);
}

This explode turns every raw sqref into its range components which are then iterated on. That means in the example I provided, the loop will run three times, once for the range "E2:H3", and twice for the range "E2:H3 E5:H6", leading to the range "E2:H3" looping twice. This is the crux of the issue, in my opinion.

The call $worksheet->getStyle($cellBlock)->setConditionalStyles($conditionalStyles); then leads to the following code in /Reader/Xlsx/ConditionalStyles.php:

$this->getActiveSheet()->setConditionalStyles($this->getSelectedCells(), $conditionalStyleArray);

which then leads in /Style/Style.php, to:

public function setConditionalStyles(array $conditionalStyleArray)
{
    $this->getActiveSheet()->setConditionalStyles($this->getSelectedCells(), $conditionalStyleArray);

    return $this;
}

and the final assignment in /Worksheet/Worksheet/php:

public function setConditionalStyles($coordinate, $styles)
{
    $this->conditionalStylesCollection[$coordinate] = $styles;
 
    return $this;
}

In the example I provided, the doubling-up of the sqref "E2:H3" in the loop after the call to explode() will lead to a double-assignment and overwrite one of the set of Conditionals in this last setConditionalStyles.

[oleibman]

I don't mind your additional analysis in the slightest. However, I am confused by it. You say that setConditionalStyles contains the following logic:

// Extract all cell references in $cellRangeReference
$cellBlocks = explode(' ', str_replace('$', '', strtoupper($cellRangeReference)));
foreach ($cellBlocks as $cellBlock) {
    $worksheet->getStyle($cellBlock)->setConditionalStyles($conditionalStyles);
}

Perhaps I am looking at the wrong module, but in my PR, Reader/Xlsx/ConditionalStyles.php consists entirely of:

   private function setConditionalStyles(Worksheet $worksheet, array $conditionals, SimpleXMLElement $xmlExtLst): void
    {
        foreach ($conditionals as $cellRangeReference => $cfRules) {
            ksort($cfRules); // no longer needed for Xlsx, but helps Xls
            $conditionalStyles = $this->readStyleRules($cfRules, $xmlExtLst);

            // Extract all cell references in $cellRangeReference
            // N.B. In Excel UI, intersection is space and union is comma.
            // But in Xml, intersection is comma and union is space.
            $cellRangeReference = str_replace(['$', ' ', ',', '^'], ['', '^', ' ', ','], strtoupper($cellRangeReference));
            $worksheet->getStyle($cellRangeReference)->setConditionalStyles($conditionalStyles);
        }
    }

It has the same comment as your quoted code, but no cellblocks and no explode. I don't see any explodes anywhere in that member. So, let's first get aligned on that.

[oleibman]

Next, it would be helpful if you told me what was wrong when you load and save your spreadsheet. For example, entering X in cell Y should cause cell Z to display in a certain style, whereas it is displaying differently. Right now, I am just using the admittedly error-prone technique of comparing the before and after xml, and that comparison looks right to me.

Also, for the record, I don't see the explode code in our current production code base. I do see it in the release1291 and release210 branches, but not in release222. I will not be backporting this change to any of those branches; it will be applied only to master. Changes to the other branches are mainly security-related fixes; this doesn't qualify.

[Awilen-Bernkastel]

Thank you for your replies!

First and foremost, I am working on version 1.29.8 off of Composer. It may be time to upgrade, I will have to check if our application needs any changes.

Regarding the problem.

When I load the spreadsheet (edited and saved prior in Excel) and then subsequently save it with PhpSpreadsheet without modification, the cfRules plainly disappear. Here is a screeshot of the rules in a different spreadsheet showing the issue:

Before saving:

After saving:

I do not mind that the ranges are not exactly the same, the call to explode and then the assignment that is in our current copy of the PhpSpreadsheet codebase does that.

Regarding your PR

Version 1.29.8 seems to include quite an old code despite keeping being updated. Commit 1e71154 introduced the file /Reader/Xlsx/ConditionalStyles.php which contains the explode statement over $ref ($ref which later got refactored into $cellRangeReference in commit 4a04499). $ git checkout 1.29.8 will give you a detached HEAD to the code I'm using, which contains the problem.

After review and careful observation, I believe the current code over in the master branch fixes the issue I'm experiencing. I will now trial the latest version of PhpSpreadsheet over our application and test for functionality compatibility and breakage.

Best regards.

[Awilen-Bernkastel]

Update: the latest version of PhpSpreadsheet works well with our application and doesn't mangle the cfRule sqrefs anymore, and works well with our application.

I believe this issue can be closed.