From 118a1094998852f26d6925763af3b8b5820a1fd3 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Wed, 18 Dec 2019 21:38:40 +0000 Subject: [PATCH 1/3] MDL-67547 dataformat: allow plugins to declare support for HTML. --- dataformat/html/classes/writer.php | 11 +++++++++++ dataformat/json/classes/writer.php | 2 +- dataformat/pdf/classes/writer.php | 23 +++++++++++++++++------ lib/classes/dataformat/base.php | 21 +++++++++++++++++++++ lib/classes/dataformat/spout_base.php | 4 ++-- lib/tablelib.php | 11 ++++++++++- 6 files changed, 62 insertions(+), 10 deletions(-) diff --git a/dataformat/html/classes/writer.php b/dataformat/html/classes/writer.php index a7b60d4ea0e..49ed857d0fe 100644 --- a/dataformat/html/classes/writer.php +++ b/dataformat/html/classes/writer.php @@ -92,6 +92,15 @@ table { echo \html_writer::end_tag('tr'); } + /** + * Method to define whether the dataformat supports export of HTML + * + * @return bool + */ + public function supports_html(): bool { + return true; + } + /** * Write a single record * @@ -99,6 +108,8 @@ table { * @param int $rownum */ public function write_record($record, $rownum) { + $record = $this->format_record($record); + echo \html_writer::start_tag('tr'); foreach ($record as $cell) { echo \html_writer::tag('td', $cell); diff --git a/dataformat/json/classes/writer.php b/dataformat/json/classes/writer.php index 599f3fee809..effd83ecaa9 100644 --- a/dataformat/json/classes/writer.php +++ b/dataformat/json/classes/writer.php @@ -80,7 +80,7 @@ class writer extends \core\dataformat\base { echo ","; } - echo json_encode($record, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT); + echo json_encode($this->format_record($record), JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT); $this->sheetdatadded = true; } diff --git a/dataformat/pdf/classes/writer.php b/dataformat/pdf/classes/writer.php index 1fce4399d16..9768ffd0c4a 100644 --- a/dataformat/pdf/classes/writer.php +++ b/dataformat/pdf/classes/writer.php @@ -93,14 +93,25 @@ class writer extends \core\dataformat\base { $this->print_heading(); } + /** + * Method to define whether the dataformat supports export of HTML + * + * @return bool + */ + public function supports_html(): bool { + return true; + } + + /** + * Write a single record + * + * @param array $record + * @param int $rownum + */ public function write_record($record, $rownum) { $rowheight = 0; - // If $record is an object convert it to an array. - if (is_object($record)) { - $record = (array)$record; - } - + $record = $this->format_record($record); foreach ($record as $cell) { $rowheight = max($rowheight, $this->pdf->getStringHeight($this->colwidth, $cell, false, true, '', 1)); } @@ -123,7 +134,7 @@ class writer extends \core\dataformat\base { // Determine whether we're at the last element of the record. $nextposition = ($lastkey === $key) ? 1 : 0; // Write the element. - $this->pdf->Multicell($this->colwidth, $rowheight, $cell, 1, 'L', false, $nextposition); + $this->pdf->writeHTMLCell($this->colwidth, $rowheight, '', '', $cell, 1, $nextposition, false, true, 'L'); } } diff --git a/lib/classes/dataformat/base.php b/lib/classes/dataformat/base.php index a0dc3686fa6..9fb443b3200 100644 --- a/lib/classes/dataformat/base.php +++ b/lib/classes/dataformat/base.php @@ -145,6 +145,27 @@ abstract class base { // Override me if needed. } + /** + * Method to define whether the dataformat supports export of HTML + * + * @return bool + */ + public function supports_html(): bool { + return false; + } + + /** + * Apply formatting to the cells of a given record + * + * @param array|\stdClass $record + * @return array + */ + protected function format_record($record): array { + $record = (array)$record; + + return $record; + } + /** * Write a single record * diff --git a/lib/classes/dataformat/spout_base.php b/lib/classes/dataformat/spout_base.php index 02a827f60f9..4c582707ec3 100644 --- a/lib/classes/dataformat/spout_base.php +++ b/lib/classes/dataformat/spout_base.php @@ -116,11 +116,11 @@ abstract class spout_base extends \core\dataformat\base { /** * Write a single record * - * @param object $record + * @param array $record * @param int $rownum */ public function write_record($record, $rownum) { - $row = \Box\Spout\Writer\Common\Creator\WriterEntityFactory::createRowFromArray((array)$record); + $row = \Box\Spout\Writer\Common\Creator\WriterEntityFactory::createRowFromArray($this->format_record($record)); $this->writer->addRow($row); } diff --git a/lib/tablelib.php b/lib/tablelib.php index 55a83c2c719..ff6a1ac018a 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -2012,7 +2012,7 @@ class table_default_export_format_parent { */ class table_dataformat_export_format extends table_default_export_format_parent { - /** @var $dataformat */ + /** @var \core\dataformat\base $dataformat */ protected $dataformat; /** @var $rownum */ @@ -2047,6 +2047,15 @@ class table_dataformat_export_format extends table_default_export_format_parent \core\session\manager::write_close(); } + /** + * Whether the current dataformat supports export of HTML + * + * @return bool + */ + public function supports_html(): bool { + return $this->dataformat->supports_html(); + } + /** * Start document * From 233a51ad21ea7806b442c329814df5242b70b2e1 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Mon, 27 Apr 2020 10:13:30 +0100 Subject: [PATCH 2/3] MDL-67547 dataformat_pdf: method to convert images to supported format. For Dataformats that support exporting HTML content, provide an API for converting images within that content to something suitable for the format. This fixes an issue with the PDF writer when it encountered a pluginfile.php image, which it tried to request via HTTP without an active session. This resulted in a 303 header returned by Moodle instead of the actual image, causing an exception in the underlying TCPDF library. --- dataformat/pdf/classes/writer.php | 27 +++++++++- dataformat/pdf/tests/writer_test.php | 74 ++++++++++++++++++++++++++++ dataformat/upgrade.txt | 7 +++ lib/classes/dataformat/base.php | 55 +++++++++++++++++++++ 4 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 dataformat/pdf/tests/writer_test.php diff --git a/dataformat/pdf/classes/writer.php b/dataformat/pdf/classes/writer.php index 9768ffd0c4a..2203af5697d 100644 --- a/dataformat/pdf/classes/writer.php +++ b/dataformat/pdf/classes/writer.php @@ -102,6 +102,23 @@ class writer extends \core\dataformat\base { return true; } + /** + * When exporting images, we need to return their Base64 encoded content. Otherwise TCPDF will create a HTTP + * request for them, which will lead to the login page (i.e. not the image it expects) and throw an exception + * + * Note: ideally we would copy the file to a temp location and return it's path, but a bug in TCPDF currently + * prevents that + * + * @param \stored_file $file + * @return string|null + */ + protected function export_html_image_source(\stored_file $file): ?string { + // Set upper dimensions for embedded images. + $resizedimage = $file->resize_image(400, 300); + + return '@' . base64_encode($resizedimage); + } + /** * Write a single record * @@ -113,7 +130,15 @@ class writer extends \core\dataformat\base { $record = $this->format_record($record); foreach ($record as $cell) { - $rowheight = max($rowheight, $this->pdf->getStringHeight($this->colwidth, $cell, false, true, '', 1)); + // We need to calculate the row height (accounting for any content). Unfortunately TCPDF doesn't provide an easy + // method to do that, so we create a second PDF inside a transaction, add cell content and use the largest cell by + // height. Solution similar to that at https://stackoverflow.com/a/1943096. + $pdf2 = clone $this->pdf; + $pdf2->startTransaction(); + $pdf2->AddPage('L'); + $pdf2->writeHTMLCell($this->colwidth, 0, '', '', $cell, 1, 1, false, true, 'L'); + $rowheight = max($rowheight, $pdf2->getY() - $pdf2->getMargins()['top']); + $pdf2->rollbackTransaction(); } $margins = $this->pdf->getMargins(); diff --git a/dataformat/pdf/tests/writer_test.php b/dataformat/pdf/tests/writer_test.php new file mode 100644 index 00000000000..e1218a0639a --- /dev/null +++ b/dataformat/pdf/tests/writer_test.php @@ -0,0 +1,74 @@ +. + +/** + * Tests for the dataformat_pdf writer + * + * @package dataformat_pdf + * @copyright 2020 Paul Holden + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace dataformat_pdf; + +use core\dataformat; +use context_system; +use html_writer; +use moodle_url; + +/** + * Writer tests + * + * @package dataformat_pdf + * @copyright 2020 Paul Holden + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class writer_testcase extends \advanced_testcase { + + /** + * Test writing data whose content contains an image with pluginfile.php source + */ + public function test_write_data_with_pluginfile_image(): void { + global $CFG; + + $this->resetAfterTest(true); + + $imagefixture = "{$CFG->dirroot}/lib/filestorage/tests/fixtures/testimage.jpg"; + $image = get_file_storage()->create_file_from_pathname([ + 'contextid' => context_system::instance()->id, + 'component' => 'dataformat_pdf', + 'filearea' => 'test', + 'itemid' => 0, + 'filepath' => '/', + 'filename' => basename($imagefixture), + + ], $imagefixture); + + $imageurl = moodle_url::make_pluginfile_url($image->get_contextid(), $image->get_component(), $image->get_filearea(), + $image->get_itemid(), $image->get_filepath(), $image->get_filename()); + + // Insert out test image into the data so it is exported. + $columns = ['animal', 'image']; + $row = ['cat', html_writer::img($imageurl->out(), 'My image')]; + + // Export to file. Assert that the exported file exists. + $exportfile = dataformat::write_data('My export', 'pdf', $columns, [$row]); + $this->assertFileExists($exportfile); + + // The exported file should be a reasonable size (~275kb). + $this->assertGreaterThan(270000, filesize($exportfile)); + } +} diff --git a/dataformat/upgrade.txt b/dataformat/upgrade.txt index cf4d09edc67..89eaa5bce0c 100644 --- a/dataformat/upgrade.txt +++ b/dataformat/upgrade.txt @@ -9,6 +9,13 @@ information provided here is intended especially for developers. * Calls to the following dataformat plugin methods have been removed: - write_header() - write_footer() +* The following methods have been added to the base class to allow instances to define support for exporting + HTML content, with additional support for defining how images should be embedded: + - supports_html() + - export_html_image_source() +* Dataformat writers should also call the following method to ensure data is properly formatted before being + written, which takes into account prior methods defining support for HTML: + - format_record() === 3.4 === * In order to allow multiple sheets in an exported file the functions write_header() and write_footer() have diff --git a/lib/classes/dataformat/base.php b/lib/classes/dataformat/base.php index 9fb443b3200..454ed721757 100644 --- a/lib/classes/dataformat/base.php +++ b/lib/classes/dataformat/base.php @@ -163,9 +163,64 @@ abstract class base { protected function format_record($record): array { $record = (array)$record; + // If the dataformat supports export of HTML, we need to allow them to manage embedded images. + if ($this->supports_html()) { + $record = array_map([$this, 'replace_pluginfile_images'], $record); + } + return $record; } + /** + * Given a stored_file, return a suitable source attribute for an img element in the export (or null to use the original) + * + * @param \stored_file $file + * @return string|null + */ + protected function export_html_image_source(\stored_file $file): ?string { + return null; + } + + /** + * We need to locate all img tags within a given cell that match pluginfile URL's. Partly so the exported file will show + * the image without requiring the user is logged in; and also to prevent some of the dataformats requesting the file + * themselves, which is likely to fail due to them not having an active session + * + * @param string|null $content + * @return string + */ + protected function replace_pluginfile_images(?string $content): string { + $content = (string)$content; + + // Examine content to see if it contains any HTML image tags. + return preg_replace_callback('/(?
]+src=")(?[^"]*)(?".*>)/i', function(array $matches) {
+            $source = $matches['source'];
+
+            // Now check if the image source looks like a pluginfile URL.
+            if (preg_match('/pluginfile.php\/(?\d+)\/(?[^\/]+)\/(?[^\/]+)\/(?:(?\d+)\/)?' .
+                    '(?.*)/u', $source, $args)) {
+
+                $context = $args['context'];
+                $component = clean_param($args['component'], PARAM_COMPONENT);
+                $filearea = clean_param($args['filearea'], PARAM_AREA);
+                $itemid = $args['itemid'] ?: 0;
+                $path = clean_param(urldecode($args['path']), PARAM_PATH);
+
+                // Try and get the matching file from storage, allow the dataformat to define the replacement source.
+                $fullpath = "/{$context}/{$component}/{$filearea}/{$itemid}/{$path}";
+                if ($file = get_file_storage()->get_file_by_hash(sha1($fullpath))) {
+                    $exportsource = $this->export_html_image_source($file);
+
+                    if ($exportsource) {
+                        $source = $exportsource;
+                    }
+                }
+            }
+
+            return $matches['pre'] . $source . $matches['post'];
+        }, $content);
+    }
+
     /**
      * Write a single record
      *

From f8f5a2f6dac8dd0bc133bcd1a28e02e75c2daee1 Mon Sep 17 00:00:00 2001
From: Paul Holden 
Date: Tue, 5 May 2020 11:25:43 +0100
Subject: [PATCH 3/3] MDL-67547 mod_forum: convert pluginfile URLs prior to
 export.

---
 mod/forum/export.php | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/mod/forum/export.php b/mod/forum/export.php
index 14509f50ccd..9aee3f18809 100644
--- a/mod/forum/export.php
+++ b/mod/forum/export.php
@@ -120,7 +120,7 @@ if ($form->is_cancelled()) {
                 'messageformat', 'messagetrust', 'attachment', 'totalscore', 'mailnow', 'deleted', 'privatereplyto',
                 'privatereplytofullname', 'wordcount', 'charcount'];
 
-    $canviewfullname = has_capability('moodle/site:viewfullnames', $forum->get_context());
+    $canviewfullname = has_capability('moodle/site:viewfullnames', $context);
 
     $datamapper = $legacydatamapperfactory->get_post_data_mapper();
     $exportdata = new ArrayObject($datamapper->to_legacy_objects($posts));
@@ -132,7 +132,7 @@ if ($form->is_cancelled()) {
         $dataformat,
         $fields,
         $iterator,
-        function($exportdata) use ($fields, $striphtml, $humandates, $canviewfullname) {
+        function($exportdata) use ($fields, $striphtml, $humandates, $canviewfullname, $context) {
             $data = new stdClass();
 
             foreach ($fields as $field) {
@@ -149,6 +149,11 @@ if ($form->is_cancelled()) {
                     $data->privatereplytofullname = fullname($user, $canviewfullname);
                 }
 
+                if ($field == 'message') {
+                    $data->message = file_rewrite_pluginfile_urls($data->message, 'pluginfile.php', $context->id, 'mod_forum',
+                        'post', $data->id);
+                }
+
                 // Convert any boolean fields to their integer equivalent for output.
                 if (is_bool($data->$field)) {
                     $data->$field = (int) $data->$field;
@@ -156,11 +161,6 @@ if ($form->is_cancelled()) {
             }
 
             if ($striphtml) {
-                // The following call to html_to_text uses the option that strips out
-                // all URLs, but format_text complains if it finds @@PLUGINFILE@@ tokens.
-                // So, we need to replace @@PLUGINFILE@@ with a real URL, but it doesn't
-                // matter what. We use http://example.com/.
-                $data->message = str_replace('@@PLUGINFILE@@/', 'http://example.com/', $data->message);
                 $data->message = html_to_text(format_text($data->message, $data->messageformat), 0, false);
                 $data->messageformat = FORMAT_PLAIN;
             }