fix: pdf display with tcpdf - #93
jdurand-teclib wants to merge 1 commit into
Conversation
Rom1-B
left a comment
There was a problem hiding this comment.
Please add tests
diff --git a/.gitignore b/.gitignore
index 57872d0..420fb40 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1 +1,2 @@
/vendor/
+var/
diff --git a/phpunit.xml b/phpunit.xml
new file mode 100644
index 0000000..b827cdf
--- /dev/null
+++ b/phpunit.xml
@@ -0,0 +1,18 @@
+<phpunit
+ bootstrap="tests/bootstrap.php"
+ colors="true"
+ testdox="true"
+ cacheDirectory="var/phpunit"
+>
+ <source>
+ <include>
+ <directory>src</directory>
+ </include>
+ </source>
+
+ <testsuites>
+ <testsuite name="Tests">
+ <directory suffix="Test.php">tests</directory>
+ </testsuite>
+ </testsuites>
+</phpunit>
diff --git a/tests/SimplePDFTest.php b/tests/SimplePDFTest.php
new file mode 100644
index 0000000..339a3b7
--- /dev/null
+++ b/tests/SimplePDFTest.php
@@ -0,0 +1,100 @@
+<?php
+
+/**
+ * -------------------------------------------------------------------------
+ * LICENSE
+ *
+ * This file is part of PDF plugin for GLPI.
+ *
+ * PDF is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * PDF is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Reports. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * @author Nelly Mahu-Lasson, Remi Collet, Teclib
+ * @author Teclib
+ * @copyright Copyright (c) 2009-2026 PDF plugin team
+ * @license AGPL License 3.0 or (at your option) any later version
+ * @link https://github.com/pluginsGLPI/pdf/
+ * @link http://www.glpi-project.org/
+ * @package pdf
+ * @since 2009
+ * http://www.gnu.org/licenses/agpl-3.0-standalone.html
+ * --------------------------------------------------------------------------
+ */
+
+use Glpi\Tests\GLPITestCase;
+
+class SimplePDFTest extends GLPITestCase
+{
+ private function getStringWidth(PluginPdfSimplePDF $pdf, string $string): float
+ {
+ $property = new ReflectionProperty(PluginPdfSimplePDF::class, 'pdf');
+
+ return $property->getValue($pdf)->GetStringWidth($string);
+ }
+
+ public function testWrapCellContentLeavesHtmlUntouched(): void
+ {
+ $pdf = new PluginPdfSimplePDF();
+ $html = '<b>' . str_repeat('a', 200) . '</b>';
+
+ $this->assertSame($html, $this->callPrivateMethod($pdf, 'wrapCellContent', $html, 10));
+ }
+
+ public function testWrapCellContentLeavesContentUntouchedWhenWidthIsNotPositive(): void
+ {
+ $pdf = new PluginPdfSimplePDF();
+ $msg = str_repeat('a', 200);
+
+ $this->assertSame($msg, $this->callPrivateMethod($pdf, 'wrapCellContent', $msg, 0));
+ }
+
+ public function testBreakWordToFitLeavesWordUntouchedWhenItAlreadyFits(): void
+ {
+ $pdf = new PluginPdfSimplePDF();
+
+ $this->assertSame('short', $this->callPrivateMethod($pdf, 'breakWordToFit', 'short', 100));
+ }
+
+ public function testBreakWordToFitSplitsOnDelimiters(): void
+ {
+ $pdf = new PluginPdfSimplePDF();
+ $word = str_repeat('a', 20) . '/' . str_repeat('b', 20) . '-' . str_repeat('c', 20);
+ $width = $this->getStringWidth($pdf, str_repeat('a', 30));
+
+ $result = $this->callPrivateMethod($pdf, 'breakWordToFit', $word, $width);
+ $chunks = explode(' ', $result);
+
+ $this->assertSame($word, str_replace(' ', '', $result));
+ $this->assertGreaterThan(1, count($chunks));
+ foreach ($chunks as $chunk) {
+ $this->assertLessThanOrEqual($width, $this->getStringWidth($pdf, $chunk));
+ }
+ }
+
+ public function testBreakWordToFitFallsBackToCharacterSplitWithoutDelimiters(): void
+ {
+ $pdf = new PluginPdfSimplePDF();
+ $word = str_repeat('a', 200);
+ $width = $this->getStringWidth($pdf, str_repeat('a', 10));
+
+ $result = $this->callPrivateMethod($pdf, 'breakWordToFit', $word, $width);
+ $chunks = explode(' ', $result);
+
+ $this->assertSame($word, str_replace(' ', '', $result));
+ $this->assertGreaterThan(1, count($chunks));
+ foreach ($chunks as $chunk) {
+ $this->assertNotSame('', $chunk);
+ $this->assertLessThanOrEqual($width, $this->getStringWidth($pdf, $chunk));
+ }
+ }
+}
diff --git a/tests/bootstrap.php b/tests/bootstrap.php
new file mode 100644
index 0000000..f87da9a
--- /dev/null
+++ b/tests/bootstrap.php
@@ -0,0 +1,40 @@
+<?php
+
+/**
+ * -------------------------------------------------------------------------
+ * LICENSE
+ *
+ * This file is part of PDF plugin for GLPI.
+ *
+ * PDF is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * PDF is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Reports. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * @author Nelly Mahu-Lasson, Remi Collet, Teclib
+ * @copyright Copyright (c) 2009-2022 PDF plugin team
+ * @license AGPL License 3.0 or (at your option) any later version
+ * @link https://github.com/pluginsGLPI/pdf/
+ * @link http://www.glpi-project.org/
+ * @package pdf
+ * @since 2009
+ * http://www.gnu.org/licenses/agpl-3.0-standalone.html
+ * --------------------------------------------------------------------------
+ */
+
+$current_plugin_folder = basename(dirname(__DIR__));
+
+require __DIR__ . '/../../../tests/bootstrap.php';
+require dirname(__DIR__) . '/vendor/autoload.php';
+
+if (!Plugin::isPluginActive($current_plugin_folder)) {
+ throw new RuntimeException(sprintf('Plugin %s is not active in the test database', $current_plugin_folder));
+}| /* Pass image as inline data to TCPDF header to avoid permissions error on image's folder */ | ||
| $logo = is_file($logo_path) ? '@' . file_get_contents($logo_path) : ''; |
There was a problem hiding this comment.
TCPDF's default Header() only accepts a real file path (is_file($logofile) check, tcpdf.php:2680-2693); it never special-cases the '@'-inline-data convention Image() supports, so the logo silently never renders. Writing it to a temp file keeps the current architecture (subclassing TCPDF to override Header(), like core's GLPIPDF, is the other option but is a bigger change).
| /* Pass image as inline data to TCPDF header to avoid permissions error on image's folder */ | |
| $logo = is_file($logo_path) ? '@' . file_get_contents($logo_path) : ''; | |
| /* TCPDF's Header() only accepts a real file path (is_file() check), not the '@data' syntax Image() supports */ | |
| $logo = ''; | |
| if (is_file($logo_path)) { | |
| $tmp_logo = GLPI_TMP_DIR . '/' . uniqid('pdf_header_logo_') . '.' . pathinfo($logo_path, PATHINFO_EXTENSION); | |
| if (copy($logo_path, $tmp_logo)) { | |
| $logo = $tmp_logo; | |
| } | |
| } |
| private function breakWordToFit($word, $width) | ||
| { | ||
| if ($this->pdf->GetStringWidth($word) <= $width) { | ||
| return $word; |
There was a problem hiding this comment.
breakWordToFit() returns the word untouched when it finds no delimiter to split on (line 267-269), even though it doesn't fit. Adding a character-level fallback there (not the tag-column bug specifically: Rule::getUuid() tags do contain -/., so breakpoints normally exist there).
| return $word; | |
| // No delimiter to split on: force a character-level break so the word still fits. | |
| $result = ''; | |
| $chunk = ''; | |
| foreach (preg_split('//u', $word, -1, PREG_SPLIT_NO_EMPTY) as $char) { | |
| if ($chunk !== '' && $this->pdf->GetStringWidth($chunk . $char) > $width) { | |
| $result .= $chunk . ' '; | |
| $chunk = ''; | |
| } | |
| $chunk .= $char; | |
| } | |
| return $result . $chunk; |
This PR is a bit special:
With TCPDF 7, even if the interface contract is the same, the rendering engine changed a bit and some adjustment were necessary on our side, otherwise pdf display would completely break.
There is however one thing I do not manage to figure out: some contents like the tag column from documents can overflow their cell, where it was perfectly handled by the library before.
I used claude to help me fix this particular issue, but I'm not really conviced by the code produced. I believe indeed it can be a bit hard to maintain, but I do not find any other proper way to do it.
The concerned code is in PluginPdfSimplePDF::displayInternal() methods (simplepdf.class.php:297). It concist of 2 methods successively called to manage the line change ourselves, since TCPDF doesn't manage to do it.
Here is the result we can observe due to these lines, from a computer export that has documents attached:

And here is what it does if you do not have these changes:

I joined both pdf
computer_with_changes.pdf
computer_without_changes.pdf
So my question is the following: Do you think there is a better, more suitable way to fix this ?
I've spent a lot of time in TCPDF documentation and in the mapping file TCPDF wrote to transition from v6 to v7, to try to sport some breaking changes with the writeHTMLCell method we use to render our pdf cell and play with the arguments, because some of them changed a bit, but nothing concluding
https://www.rubydoc.info/gems/rfpdf/1.17.1/TCPDF:writeHTMLCell
https://github.com/tecnickcom/TCPDF/blob/main/MAPPING.md#htmlcss
Also the header images do not render. I had to change the way we retrieve them since the library cannot access our image file not more, so I send it in a form of an inline file, but the image is not displayed, even tho it is sent to the TCPDF->setHeaderData() method. You can check in the PDF, the GLPI logo supposed to be on top left of each page is not present.