From ecb4a7fe278329e032d0a79809cc5446b6a08893 Mon Sep 17 00:00:00 2001 From: Owen Leibman Date: Fri, 2 Jul 2021 14:33:43 -0700 Subject: [PATCH] DocBlock Changes for Chart/Title This is a leftover Scrutinizer change, but it needed more attention than most others. Chart/Title DocBlocks define caption as `null|string`. However, in the wild, Excel usually presents the caption as an array, and not an array of strings but rather of RichText items. I am not sure why an array is needed since a RichText item can contain many text runs, but things are what they are. Reader/Xlsx/ChartTitleTest reads a spreadsheet with the captions stored as a RichText array. Since it performs array operations on something the DocBlock says cannot be an array, Scrutinizer objects, although not seriously enough to fail the module. Phpstan also objects; its objection is silenced with an annotation. Aside from this test, there are other tests which do set the caption to a string, and Excel seems to handle that without a problem. So, I have changed the DocBlock to specify `array|RichText|String`. I have dropped null as a possibility; nullstring will do equally well. Because getCaption can now return multiple datatypes, I think a new function which can return the text portion of the entire caption as a single string is needed. I have added it. This simplifies the test named above, and some code in Writer/Html. The latter is not part of unit testing because the version of JpGraph found in Composer is too antiquated. I verified the Html change manually by running samples/Chart/32_Chart_read_write_HTML.php using a recent version of JpGraph. It was as a result of this test that I uncovered issue #2203. I did not see anything about Charts in docs, so did not add a description of the new function there. Phpstan is happy with the changes. We'll see how Scrutinizer feels when I push it. --- phpstan-baseline.neon | 10 ---- src/PhpSpreadsheet/Chart/Title.php | 37 ++++++++++-- src/PhpSpreadsheet/Writer/Html.php | 23 +------- tests/PhpSpreadsheetTests/Chart/TitleTest.php | 58 +++++++++++++++++++ .../Reader/Xlsx/ChartsTitleTest.php | 10 +--- 5 files changed, 92 insertions(+), 46 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Chart/TitleTest.php diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 3538d091..01dfda8a 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -6210,16 +6210,6 @@ parameters: count: 45 path: src/PhpSpreadsheet/Writer/Xlsx/Chart.php - - - message: "#^Call to function is_array\\(\\) with string will always evaluate to false\\.$#" - count: 3 - path: src/PhpSpreadsheet/Writer/Xlsx/Chart.php - - - - message: "#^Result of && is always false\\.$#" - count: 1 - path: src/PhpSpreadsheet/Writer/Xlsx/Chart.php - - message: "#^Strict comparison using \\=\\=\\= between PhpOffice\\\\PhpSpreadsheet\\\\Chart\\\\PlotArea and null will always evaluate to false\\.$#" count: 1 diff --git a/src/PhpSpreadsheet/Chart/Title.php b/src/PhpSpreadsheet/Chart/Title.php index af9fa088..090c4f3f 100644 --- a/src/PhpSpreadsheet/Chart/Title.php +++ b/src/PhpSpreadsheet/Chart/Title.php @@ -2,14 +2,16 @@ namespace PhpOffice\PhpSpreadsheet\Chart; +use PhpOffice\PhpSpreadsheet\RichText\RichText; + class Title { /** * Title Caption. * - * @var string + * @var array|RichText|string */ - private $caption; + private $caption = ''; /** * Title Layout. @@ -21,9 +23,9 @@ class Title /** * Create a new Title. * - * @param null|mixed $caption + * @param array|RichText|string $caption */ - public function __construct($caption = null, ?Layout $layout = null) + public function __construct($caption = '', ?Layout $layout = null) { $this->caption = $caption; $this->layout = $layout; @@ -32,17 +34,40 @@ class Title /** * Get caption. * - * @return string + * @return array|RichText|string */ public function getCaption() { return $this->caption; } + public function getCaptionText(): string + { + $caption = $this->caption; + if (is_string($caption)) { + return $caption; + } + if ($caption instanceof RichText) { + return $caption->getPlainText(); + } + $retVal = ''; + foreach ($caption as $textx) { + /** @var RichText|string */ + $text = $textx; + if ($text instanceof RichText) { + $retVal .= $text->getPlainText(); + } else { + $retVal .= $text; + } + } + + return $retVal; + } + /** * Set caption. * - * @param string $caption + * @param array|RichText|string $caption * * @return $this */ diff --git a/src/PhpSpreadsheet/Writer/Html.php b/src/PhpSpreadsheet/Writer/Html.php index ded9a964..1ac4d4ba 100644 --- a/src/PhpSpreadsheet/Writer/Html.php +++ b/src/PhpSpreadsheet/Writer/Html.php @@ -749,7 +749,7 @@ class Html extends BaseWriter $html .= PHP_EOL; $imageDetails = getimagesize($chartFileName); $filedesc = $chart->getTitle(); - $filedesc = $filedesc ? self::getChartCaption($filedesc->getCaption()) : ''; + $filedesc = $filedesc ? $filedesc->getCaptionText() : ''; $filedesc = $filedesc ? htmlspecialchars($filedesc, ENT_QUOTES) : 'Embedded chart'; if ($fp = fopen($chartFileName, 'rb', 0)) { $picture = fread($fp, filesize($chartFileName)); @@ -770,27 +770,6 @@ class Html extends BaseWriter return $html; } - /** - * Extend Row if chart is placed after nominal end of row. - * This code should be exercised by sample: - * Chart/32_Chart_read_write_PDF.php. - * However, that test is suppressed due to out-of-date - * Jpgraph code issuing warnings. So, don't measure - * code coverage for this function till that is fixed. - * Caption is described in documentation as fixed, - * but in 32_Chart it is somehow an array of RichText. - * - * @param mixed $cap - * - * @return string - * - * @codeCoverageIgnore - */ - private static function getChartCaption($cap) - { - return is_array($cap) ? implode(' ', $cap) : $cap; - } - /** * Generate CSS styles. * diff --git a/tests/PhpSpreadsheetTests/Chart/TitleTest.php b/tests/PhpSpreadsheetTests/Chart/TitleTest.php new file mode 100644 index 00000000..9247b0c2 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Chart/TitleTest.php @@ -0,0 +1,58 @@ +getCaption()); + self::assertSame('hello', $title->getCaptionText()); + } + + public function testStringArray(): void + { + $title = new Title(); + $title->setCaption(['Hello', ', ', 'world.']); + self::assertSame('Hello, world.', $title->getCaptionText()); + } + + public function testRichText(): void + { + $title = new Title(); + $richText = new RichText(); + $part = $richText->createTextRun('Hello'); + $font = $part->getFont(); + if ($font === null) { + self::fail('Unable to retrieve font'); + } else { + $font->setBold(true); + $title->setCaption($richText); + self::assertSame('Hello', $title->getCaptionText()); + } + } + + public function testMixedArray(): void + { + $title = new Title(); + $richText1 = new RichText(); + $part1 = $richText1->createTextRun('Hello'); + $font1 = $part1->getFont(); + $richText2 = new RichText(); + $part2 = $richText2->createTextRun('world'); + $font2 = $part2->getFont(); + if ($font1 === null || $font2 === null) { + self::fail('Unable to retrieve font'); + } else { + $font1->setBold(true); + $font2->setItalic(true); + $title->setCaption([$richText1, ', ', $richText2, '.']); + self::assertSame('Hello, world.', $title->getCaptionText()); + } + } +} diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/ChartsTitleTest.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/ChartsTitleTest.php index f9fa54ab..5cde677d 100644 --- a/tests/PhpSpreadsheetTests/Reader/Xlsx/ChartsTitleTest.php +++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/ChartsTitleTest.php @@ -8,15 +8,9 @@ use PHPUnit\Framework\TestCase; class ChartsTitleTest extends TestCase { - private static function getTitleText(?Title $title): ?string + private static function getTitleText(?Title $title): string { - if (null === $title || empty($title->getCaption())) { - return null; - } - - return implode("\n", array_map(function ($rt) { - return $rt->getPlainText(); - }, $title->getCaption())); // @phpstan-ignore-line + return ($title === null) ? '' : $title->getCaptionText(); } public function testChartTitles(): void