Update JpGraphRendererBase.php - check existing of PlotLabel

[yfinkel]

I've got
Uncaught PHP Exception Symfony\Component\ErrorHandler\Error\FatalError: "Error: Uncaught Error: Call to a member function getDataValue() on bool in .../vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Chart/Renderer/JpGraphRendererBase.php:337

I found that it's necessary to check existing of PlotLabel before using its method getDataValue().

This is:

• [x ] 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
• [ x] Code style is respected
• [x ] 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?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

[oleibman]

I think seriesPlot->SetLegend($dataLabel) needs to be inside the if statement, otherwise dataLabel will not be defined.

[oleibman]

Also, please add a unit test to demonstrate that there was a problem before your fix and you've fixed it.

[yfinkel]

I can't, because I use a big xlsx file in my project, which causes a crash in this place. I can't localize the issue in this file, because it's very big with many formulae and charts. But this change solved the problem for me.

…

-- Yury Finkel ср, 26 июн. 2024 г. в 11:39, oleibman ***@***.***>:

***@***.**** commented on this pull request. ------------------------------ In src/PhpSpreadsheet/Chart/Renderer/JpGraphRendererBase.php <#4074 (comment)> : > @@ -334,7 +334,9 @@ private function renderPlotLine(int $groupID, bool $filled = false, bool $combin // Set the appropriate plot marker $this->formatPointMarker($seriesPlot, $marker); } - $dataLabel = $this->chart->getPlotArea()->getPlotGroupByIndex($groupID)->getPlotLabelByIndex($index)->getDataValue(); + if ($this->chart->getPlotArea()->getPlotGroupByIndex($groupID)->getPlotLabelByIndex($index)) { + $dataLabel = $this->chart->getPlotArea()->getPlotGroupByIndex($groupID)->getPlotLabelByIndex($index)->getDataValue(); + } Also, please add a unit test to demonstrate that there was a problem before your fix and you've fixed it. — Reply to this email directly, view it on GitHub <#4074 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD5LHW6BVAGMDT25P77GE63ZJJV3PAVCNFSM6AAAAABJ5GPLY2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBQHE4DSOBSGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[oleibman]

Without an example, I am not completely unwilling to proceed, but I am a little reluctant. However, reworking your change will make me a little less reluctant.

There are 4 places in JpGraphRendererBase where dataLabel is set.

• renderPlotLine, around line 337.
• renderPlotBar, around line 411. This already tests for the situation you are trying to defend against, and I know that that situation is exercised in our unit test code.
• renderPlotScatter, around line 495.
• renderPlotRadar, around line 527.

If you were to make the code from renderPlotBar into a subroutine, and change each of these 4 instances to call the new subroutine instead of assigning a value to dataLabel directly, we would have some assurance that the other 3 are using code which is already tested. If you can confirm that the works in your case, I could proceed. As a bonus, we won't have to deal with the same situation for Radar and Scatter when someone comes across a similar situation with those in future. Something like (untested):

private function assignDataLabel(DataSeries $groupId, int $index): mixed
{
    if (!$this->chart->getPlotArea()->getPlotGroupByIndex($groupID)->getPlotLabelByIndex($index)) {
        $dataLabel = '';
    } else {
        $dataLabel = $this->chart->getPlotArea()->getPlotGroupByIndex($groupID)->getPlotLabelByIndex($index)->getDataValue();
    }

    return $dataLabel;
}

[yfinkel]

Well, I updated the code. The function name is getDataLabel(). It works well in my project. I will reappear only after a month, so do what you want with it. It's enough for me that it works for me.

[oleibman]

Thank you for your contribution.