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 column indices for embedded spreadsheets #664

Conversation

michael-roth
Copy link
Contributor

@michael-roth michael-roth commented Aug 2, 2021

This fixes a bug with embedded spreadsheet data.

Reproduction

  1. Use code below to reproduce
  2. Open generated presentation file
  3. Edit chart data to open embedded spreadsheet
use PhpOffice\PhpPresentation\IOFactory;
use PhpOffice\PhpPresentation\PhpPresentation;
use PhpOffice\PhpPresentation\Shape\Chart\Series;
use PhpOffice\PhpPresentation\Shape\Chart\Type\Bar;

$objPHPPowerPoint = new PhpPresentation();
$currentSlide = $objPHPPowerPoint->getActiveSlide();

$chart = new Bar();
$series = new Series('Series 1', [
    'A' => 90,
    'B' => 80,
    'C' => 50,
    'D' => 67,
    'E' => 99,
    'F' => 23,
]);
$chart->addSeries($series);

$chartShape = $currentSlide->createChartShape();
$chartShape->setIncludeSpreadsheet(true);
$chartShape->getPlotArea()->setType($chart);
$chartShape
    ->setWidth(400)
    ->setHeight(300)
    ->setOffsetX(100)
    ->setOffsetY(60)
;

$oWriterPPTX = IOFactory::createWriter($objPHPPowerPoint, 'PowerPoint2007');
$oWriterPPTX->save(__DIR__ . "/sample.pptx");

Expected result
Chart data is editable, chart won't break

Actual result
Chart data is not editable or chart labels break

Cause
The bug is caused by wrong column indices in the chart writer. PhpSpreadsheet uses one-based indices for columns while zero-based indices are used in the writer.

@auto-assign auto-assign bot requested a review from Progi1984 August 2, 2021 10:24
@Progi1984
Copy link
Member

@michael-roth Great fix. Could you rebase your PR to the develop branch and apply to RadarChart ?

I really need to add more unit tests for testing this. Could you fix you can add it ?

@michael-roth michael-roth force-pushed the bugfix/embedded-spreadsheet-data branch from f2c05bf to 104216a Compare August 4, 2021 07:45
@michael-roth
Copy link
Contributor Author

@Progi1984 Thanks. I rebased, fixed the remaining lines and added tests to check the series values.

What do you think? Let me know if you need more.

@Progi1984
Copy link
Member

@michael-roth Yeap, I have a last thing to test and it's ok. I add comment in code. 😍 on your bugfix.

@@ -107,20 +107,26 @@ public function testChartIncludeSpreadsheet(): void
$this->assertZipXmlElementExists('ppt/charts/' . $oShape->getIndexedFilename(), $element);
$element = '/c:chartSpace/c:chart/c:plotArea/c:lineChart/c:ser';
$this->assertZipXmlElementExists('ppt/charts/' . $oShape->getIndexedFilename(), $element);
$this->assertZipXmlElementCount('ppt/charts/' . $oShape->getIndexedFilename(), $element, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you for testChartIncludeSpreadsheet a dataProvider which pass in parameter

  • each chartType (Line/Radar....) where you have made a change in PptCharts
  • a string for element xml c:lineChart relative to each chartType
    ?

That allows me to check that each change in PptCharts is ok.

Copy link
Member

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A last feedback

@michael-roth
Copy link
Contributor Author

michael-roth commented Aug 6, 2021

@Progi1984 I worked on your feedback and changed a few things:

  1. Added abstract method getXmlElementName to AbstractChartType and implemented it in every chart type. This way I did not have to use it in the data provider and we don't have repetition of the constant strings.

  2. The order of elements was wrong for Doughnut and Radar charts, because of the defined <xsd:sequence>. I moved these elements in the writer methods to fix it.
    For Radar charts I also removed the c:smooth element. Radar charts seem to not support the smooth config.

  3. For Scatter charts there is a different xml path for the value cache (c:yVal instead of c:val), so I added a check in the unit test.

@michael-roth michael-roth force-pushed the bugfix/embedded-spreadsheet-data branch from d47e21c to c3e6ad9 Compare August 14, 2021 10:50
@mindline-analytics
Copy link
Contributor

Hi @Progi1984,
Hi @michael-roth,

I am waiting for this PR to be merged, as I use the embedded spreadsheets a lot.
Is there anything I can help with to get it ready to be merged? Unfortunately, I have no idea why phpstan fails on the dataProvider.

@Progi1984
Copy link
Member

Superseeded by #773

@Progi1984 Progi1984 closed this Dec 4, 2023
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.

3 participants