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

Fix default value for Conditional::$text #3946

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Conversation

Phenix789
Copy link
Contributor

@Phenix789 Phenix789 commented Mar 12, 2024

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

Worksheet:640 call Conditional::getText(), but if the Conditional::$text field is not initialized with a value by calling getText() PHP throw a typed error.
The fix is just to set a default value to empty string to avoid error by calling getText() uninitialized.
In addition Worksheet call getText() to check if the value is not empty, so empty string is already handled.

@oleibman
Copy link
Collaborator

The commentary to your PR says that you are making 2 changes, but I see only 1. Should there be another?

Please add a unit test case which would have failed before your change but which will succeed with it.

@Phenix789
Copy link
Contributor Author

I update PR description and yes it's only 1 fix.
But it's hard to write a test for this case, it just adding a default value for a property to avoid uninitialized error.


Furthermore, after several investigations, it appears that Conditional::$text is required for the "text" types, and using a Wizard seems more appropriate than directly using new Conditional().
My fix seems only to hide the error.
So, I think a will close this PR.
However, having a PHP error might not be the most understandable error either, right ?
What could we do ? Any suggestion ?
Thanks

@oleibman
Copy link
Collaborator

Your test should be something like the following:

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Style\Conditional;
use PhpOffice\PhpSpreadsheet\Style\Fill;
use PhpOffice\PhpSpreadsheet\Style\Style;
use PhpOffice\PhpSpreadsheet\Writer\Xlsx as XlsxWriter;

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

        $cellRange = 'C3:E5';
        $style = new Style();
        $style->applyFromArray([
            'fill' => [
                'color' => ['argb' => 'FFFFC000'],
                'fillType' => Fill::FILL_SOLID,
            ],
        ]);

        $condition = new Conditional();
        $condition->setConditionType(Conditional::CONDITION_CONTAINSTEXT);
        $condition->setOperatorType(Conditional::OPERATOR_CONTAINSTEXT);
        $condition->setStyle($style);
        $sheet->setConditionalStyles($cellRange, [$condition]);

        $writer = new XlsxWriter($spreadsheet);

        $writerWorksheet = new XlsxWriter\Worksheet($writer);
        $data = $writerWorksheet->writeWorksheet($sheet, []);
        self::assertStringContainsString('<conditionalFormatting sqref="C3:E5">', $data);

This code will error out with the current version of PhpSpreadsheet because of the error you're reporting, but will succeed after your change is made, which is exactly what we are looking for in a test. Let me know if you need help making the code above into a formal unit test.

@oleibman oleibman added this pull request to the merge queue Apr 17, 2024
Merged via the queue into PHPOffice:master with commit e9f03df Apr 17, 2024
13 of 14 checks passed
@oleibman
Copy link
Collaborator

Thank you for your contribution.

@Phenix789
Copy link
Contributor Author

Thanks for the merge 👍.
And sorry for neglecting this PR.
I've been quite busy these past few weeks, but I was planning to finish it soon.

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

Successfully merging this pull request may close these issues.

2 participants