Skip to content

fix: pdf display with tcpdf - #93

Draft
jdurand-teclib wants to merge 1 commit into
feature/glpi-12.0from
fix/pdf-display-tcpdf-7
Draft

jdurand-teclib wants to merge 1 commit into
feature/glpi-12.0from
fix/pdf-display-tcpdf-7

Conversation

@jdurand-teclib

Copy link
Copy Markdown

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.

image image image

Here is the result we can observe due to these lines, from a computer export that has documents attached:
image

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

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.

@Rom1-B Rom1-B left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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));
+}

Comment thread inc/simplepdf.class.php
Comment on lines +135 to +136
/* 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) : '';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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).

Suggested 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;
}
}

Comment thread inc/simplepdf.class.php
private function breakWordToFit($word, $width)
{
if ($this->pdf->GetStringWidth($word) <= $width) {
return $word;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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).

Suggested change
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;

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants