Skip to content

Regression Bug from v1.18 to v1.19 in Style::applyFromArray() method #2366

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

Closed
madhurbhaiya opened this issue Nov 1, 2021 · 13 comments
Closed
Labels

Comments

@madhurbhaiya
Copy link

madhurbhaiya commented Nov 1, 2021

This is:

- [*] a bug report  (Regression Bug)

What is the expected behavior?

In v1.18, Style formatting such as Background color, Border etc were getting correctly Applied to a range of Cells, using applyFromArray() method.

What is the current behavior?

In v1.19, Style formatting is broken now. Some of the noticeable issues (not exhaustive) are: Background color, Border style, Border color etc not getting applied.

What are the steps to reproduce?

<?php

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

use PhpOffice\PhpSpreadsheet\Exception;
use PhpOffice\PhpSpreadsheet\IOFactory;
use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Style\Alignment;
use PhpOffice\PhpSpreadsheet\Style\Border;
use PhpOffice\PhpSpreadsheet\Style\Fill;
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;

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

$first_row = 1;
$second_row = $first_row + 1;

// Apply Style to a Range of Cells, and Enter Cell Values
$styleArray = array(
    'font' => [
        'bold' => true,
        'color' => ['argb' => 'FFFFFF',],
    ],
    'alignment' => [
        'horizontal' => Alignment::HORIZONTAL_CENTER,
        'vertical' => Alignment::VERTICAL_CENTER,
    ],
    'borders' => array(
        'inside' => array(
            'borderStyle' => Border::BORDER_THIN,
            'color' => array('argb' => 'FFFFFFFF'),
        ),
    ),
    'fill' => array(
        'fillType' => Fill::FILL_SOLID,
        'startColor' => array('argb' => 'FC7D72'),
    ),
);

$spreadsheet->getActiveSheet()->getStyle("B$first_row:X$second_row")->applyFromArray($styleArray);

$spreadsheet->getActiveSheet()->setCellValue("B$first_row", '1');
$spreadsheet->getActiveSheet()->setCellValue("C$first_row", '2');
$spreadsheet->getActiveSheet()->setCellValue("D$first_row", '3');
$spreadsheet->getActiveSheet()->setCellValue("E$first_row", '4');
$spreadsheet->getActiveSheet()->setCellValue("F$first_row", '5');
$spreadsheet->getActiveSheet()->setCellValue("G$first_row", '6');


// Save file to a Path and Cleanup
$save_path = __DIR__.'/reg-bug.xlsx';
$writer = IOFactory::createWriter($spreadsheet, "Xlsx");
$writer->save($save_path);

Which versions of PhpSpreadsheet and PHP are affected?

PHP 7.4.24
PhpSpreadsheet v1.19

Bug is not present in v1.18

@madhurbhaiya
Copy link
Author

madhurbhaiya commented Nov 1, 2021

I think this issue has origins in PR: https://github.com/PHPOffice/PhpSpreadsheet/pull/1785/files

Attached are the two Excel Files generated using two different versions, v1.18 and v1.19, respectively. You can clearly see the difference in them.
reg-bug-1-19.xlsx
reg-bug-1-18.xlsx

@eigan
Copy link
Contributor

eigan commented Nov 1, 2021

Sorry, I can take a look at this later today (no sooner than 10 hours).

@eigan
Copy link
Contributor

eigan commented Nov 1, 2021

I took checkout on the commit in the upstream tree for the linked PR and looks like the style was still applied. Might be some other commit after that? Will need to investigate more later.

@Rockylars
Copy link

We upgraded as well and lost our colors too, currently reverting to 1.18 as we didnt have a test with colors in it, so our Production reports got all bleached. 😄

@oleibman
Copy link
Collaborator

oleibman commented Nov 1, 2021

Your sample code is setting fill argb (which should be 8 hex digits) but is only supplying 6 hex digits (as if you were supplying rgb). I think changing argb to rgb (or supplying the full 8 digits) gets you the desired result. I am not denying that there is a regression, but this PR may not be the culprit. In the meantime, you have an easy fix.

@oleibman
Copy link
Collaborator

oleibman commented Nov 1, 2021

It appears that the test for length for ARGB was introduced into Style/Color.php as part of PR #1694, which was, indeed, merged after this one.

@Rockylars
Copy link

Rockylars commented Nov 2, 2021

Correct, using 'rgb' => 'FC7D72' instead of 'argb' => 'FC7D72' solves it, i guess the rgb format in argb input was just an exploit that many used. I have not tested if adding FF in front of the argb format solves it as well, but im guessing it does too.

@madhurbhaiya
Copy link
Author

madhurbhaiya commented Nov 2, 2021

Adding FF in front of the rgb codes (6 digit ones) also solved the issue.

However, I think this regression should still be fixed. Code can be made smarter to automatically add FF in front while setting argb value, if the supplied color code string is of 6 characters only.

@petertiny
Copy link

Thanks for the solution !! It works great

@PowerKiKi
Copy link
Member

The introduction of size validation in 613a0b9 by Mark is working as intended. There are two different methods, setRGB() and setARGB(), specifically to differentiate between the two cases. The regression described here is the result of a misuse of the API, and while I understand it's annoying, it is unlikely to be fixed as it is by design.

@madhurbhaiya
Copy link
Author

If we are going to change Behaviour of an existing API method, then in my opinion, we should throw Exception, instead of assuming the color to be Black, in-case of invalid aRGB / RGB values provided.

Almost all user application code will not be having automated tests for checking Color in the Excel file generated. At best, they would have tests for successful generation of Excel only. In this case, there is a higher chance of catching this Regression in version update, instead of waiting for Customers/ End-users to report bleached Excel files, which 100s of them have received already.

@villarruel
Copy link

After debugging the setARGB function I realized that the function expects a value in hexadecimal but in uppercase, if it sends the color in lowercase the function sets a default color because it falls into an error. Don't add FF before hex if you use setARGB, better use setRGB with only 6 digits for hex.

Después de depurar la función setARGB me di cuenta que la función espera un valor en hexadecimal pero en mayúsculas, si le envía el color en minúsculas la función establece un color predeterminado por que cae en un error. No agreguen FF antes del hexadecimal si usan setARGB, mejor usen setRGB con solo 6 dígitos para hexadecimal.

Saludos.

@BrunoGGM
Copy link

My excel file didn't have any colors, effectively it works for me to change the setARG() functions to setRGB() and in applyFromArray() change from 'rgb' => 'FC7D72' instead of 'argb' => 'FC7D72'.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

8 participants