From b62b5879af34da8f02a4a0522ec3d8576ea2c163 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Wed, 24 May 2017 23:11:38 +0800 Subject: [PATCH 1/5] MDL-56046 core_dataformat: added functions to support multiple sheets Also removed write_header() and write_footer(). The reason for this is because in core we want to know if the format being used supports multiple sheets. To do this, and ensure we do not break third-party dataformat plugins, we are using method_exist(). If write_header() and write_footer() remain in the base class then this will always be true. --- lib/classes/dataformat/base.php | 23 +++++++++++++++++------ lib/classes/dataformat/spout_base.php | 11 ++++------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/classes/dataformat/base.php b/lib/classes/dataformat/base.php index d9fe32da04d..d00e46f537c 100644 --- a/lib/classes/dataformat/base.php +++ b/lib/classes/dataformat/base.php @@ -76,8 +76,6 @@ abstract class base { * Output file headers to initialise the download of the file. */ public function send_http_headers() { - global $CFG; - if (defined('BEHAT_SITE_RUNNING')) { // For text based formats - we cannot test the output with behat if we force a file download. return; @@ -98,11 +96,18 @@ abstract class base { } /** - * Write the start of the format + * Write the start of the file. + */ + public function start_output() { + // Override me if needed. + } + + /** + * Write the start of the sheet we will be adding data to. * * @param array $columns */ - public function write_header($columns) { + public function start_sheet($columns) { // Override me if needed. } @@ -115,12 +120,18 @@ abstract class base { abstract public function write_record($record, $rownum); /** - * Write the end of the format + * Write the end of the sheet containing the data. * * @param array $columns */ - public function write_footer($columns) { + public function close_sheet($columns) { // Override me if needed. } + /** + * Write the end of the file. + */ + public function close_output() { + // Override me if needed. + } } diff --git a/lib/classes/dataformat/spout_base.php b/lib/classes/dataformat/spout_base.php index 9208e605b3f..6e622ea8beb 100644 --- a/lib/classes/dataformat/spout_base.php +++ b/lib/classes/dataformat/spout_base.php @@ -75,11 +75,11 @@ abstract class spout_base extends \core\dataformat\base { } /** - * Write the start of the format + * Write the start of the sheet we will be adding data to. * * @param array $columns */ - public function write_header($columns) { + public function start_sheet($columns) { $this->writer->addRow(array_values((array)$columns)); } @@ -94,13 +94,10 @@ abstract class spout_base extends \core\dataformat\base { } /** - * Write the end of the format - * - * @param array $columns + * Write the end of the file. */ - public function write_footer($columns) { + public function close_output() { $this->writer->close(); $this->writer = null; } - } From ce3beb267cac905ee7ab31af353a0e6a6adc3967 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Wed, 24 May 2017 23:12:31 +0800 Subject: [PATCH 2/5] MDL-56046 core: convert download_as_dataformat to use new API --- lib/dataformatlib.php | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/dataformatlib.php b/lib/dataformatlib.php index 36b51731002..44893cedf6f 100644 --- a/lib/dataformatlib.php +++ b/lib/dataformatlib.php @@ -56,7 +56,15 @@ function download_as_dataformat($filename, $dataformat, $columns, $iterator, $ca $format->set_filename($filename); $format->send_http_headers(); - $format->write_header($columns); + // This exists to support all dataformats - see MDL-56046. + if (method_exists($format, 'write_header')) { + error_log('The function write_header() does not support multiple tables. In order to support multiple tables you ' . + 'must implement start_output() and start_sheet() and remove write_header() in your dataformat.'); + $format->write_header($columns); + } else { + $format->start_output(); + $format->start_sheet($columns); + } $c = 0; foreach ($iterator as $row) { if ($callback) { @@ -67,6 +75,14 @@ function download_as_dataformat($filename, $dataformat, $columns, $iterator, $ca } $format->write_record($row, $c++); } - $format->write_footer($columns); + // This exists to support all dataformats - see MDL-56046. + if (method_exists($format, 'write_footer')) { + error_log('The function write_footer() does not support multiple tables. In order to support multiple tables you ' . + 'must implement close_sheet() and close_output() and remove write_footer() in your dataformat.'); + $format->write_footer($columns); + } else { + $format->close_sheet($columns); + $format->close_output(); + } } From eb26be5b39b1b0a1a5eacd36a5eabce0324a0daf Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Wed, 24 May 2017 22:08:13 +0800 Subject: [PATCH 3/5] MDL-56046 dataformat_*: convert core plugins --- dataformat/html/classes/writer.php | 31 ++++++++++++++++-------- dataformat/json/classes/writer.php | 38 ++++++++++++++++++++++++++---- 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/dataformat/html/classes/writer.php b/dataformat/html/classes/writer.php index 90ca3688a77..0eaa5559725 100644 --- a/dataformat/html/classes/writer.php +++ b/dataformat/html/classes/writer.php @@ -42,11 +42,9 @@ class writer extends \core\dataformat\base { public $extension = ".html"; /** - * Write the start of the format - * - * @param array $columns + * Write the start of the output */ - public function write_header($columns) { + public function start_output() { echo ""; echo \html_writer::tag('title', $this->filename); echo " - - -"; +"; + } + + /** + * Write the start of the sheet we will be adding data to. + * + * @param array $columns + */ + public function start_sheet($columns) { + echo "
"; echo \html_writer::start_tag('tr'); foreach ($columns as $k => $v) { echo \html_writer::tag('th', $v); @@ -100,12 +105,18 @@ table { } /** - * Write the end of the format + * Write the end of the sheet containing the data. * * @param array $columns */ - public function write_footer($columns) { - echo "
"; + public function close_sheet($columns) { + echo ""; } + /** + * Write the end of the sheet containing the data. + */ + public function close_output() { + echo ""; + } } diff --git a/dataformat/json/classes/writer.php b/dataformat/json/classes/writer.php index 84f3cc67ccc..fe741a6ae93 100644 --- a/dataformat/json/classes/writer.php +++ b/dataformat/json/classes/writer.php @@ -41,12 +41,31 @@ class writer extends \core\dataformat\base { /** @var $extension */ public $extension = ".json"; + /** @var $hasstarted */ + public $sheetstarted = false; + + /** @var $sheetdatadded */ + public $sheetdatadded = false; + /** - * Write the start of the format + * Write the start of the file. + */ + public function start_output() { + echo "["; + } + + /** + * Write the start of the sheet we will be adding data to. * * @param array $columns */ - public function write_header($columns) { + public function start_sheet($columns) { + if ($this->sheetstarted) { + echo ","; + } else { + $this->sheetstarted = true; + } + $this->sheetdatadded = false; echo "["; } @@ -57,19 +76,28 @@ class writer extends \core\dataformat\base { * @param int $rownum */ public function write_record($record, $rownum) { - if ($rownum) { + if ($this->sheetdatadded) { echo ","; } + echo json_encode($record); + + $this->sheetdatadded = true; } /** - * Write the end of the format + * Write the end of the sheet containing the data. * * @param array $columns */ - public function write_footer($columns) { + public function close_sheet($columns) { echo "]"; } + /** + * Write the end of the file. + */ + public function close_output() { + echo "]"; + } } From d4606c1b244c72d2f5d804eb2e64dbdede5f849f Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Mon, 26 Jun 2017 13:45:59 +0800 Subject: [PATCH 4/5] MDL-56046 dataformat: added related information to upgrade.txt --- dataformat/upgrade.txt | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/dataformat/upgrade.txt b/dataformat/upgrade.txt index bfd29d1c945..620e83a2da2 100644 --- a/dataformat/upgrade.txt +++ b/dataformat/upgrade.txt @@ -1,7 +1,14 @@ This files describes API changes in /dataformat/ download system, information provided here is intended especially for developers. +=== 3.4 === + +* In order to allow multiple sheets in an exported file the functions write_header() and write_footer() have + been removed from core dataformats plugins and have been replaced. + - write_header() has been split into the two functions start_output() and start_sheet(). + - write_footer() has been split into the two functions close_output() and close_sheet(). + For backwards compatibility write_header() and write_footer() will continue to work but if used will + trigger the function error_log(). + === 3.1 === * Added new plugin system with low memory support for csv, ods, xls and json - - From faf45f9c4771656e90f17edfee30b2c482143159 Mon Sep 17 00:00:00 2001 From: Mark Nelson Date: Mon, 19 Jun 2017 19:17:27 +0800 Subject: [PATCH 5/5] MDL-56046 core: enable multiple sheets for flexible table --- lib/classes/dataformat/spout_base.php | 22 ++++++++++------ lib/tablelib.php | 36 +++++++++++++++++++++------ 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/lib/classes/dataformat/spout_base.php b/lib/classes/dataformat/spout_base.php index 6e622ea8beb..6b0c10b34b1 100644 --- a/lib/classes/dataformat/spout_base.php +++ b/lib/classes/dataformat/spout_base.php @@ -44,6 +44,9 @@ abstract class spout_base extends \core\dataformat\base { /** @var $sheettitle */ protected $sheettitle; + /** @var $renamecurrentsheet */ + protected $renamecurrentsheet = false; + /** * Output file headers to initialise the download of the file. */ @@ -54,10 +57,9 @@ abstract class spout_base extends \core\dataformat\base { } $filename = $this->filename . $this->get_extension(); $this->writer->openToBrowser($filename); - if ($this->sheettitle && $this->writer instanceof \Box\Spout\Writer\AbstractMultiSheetsWriter) { - $sheet = $this->writer->getCurrentSheet(); - $sheet->setName($this->sheettitle); - } + + // By default one sheet is always created, but we want to rename it when we call start_sheet(). + $this->renamecurrentsheet = true; } /** @@ -68,9 +70,6 @@ abstract class spout_base extends \core\dataformat\base { * @param string $title */ public function set_sheettitle($title) { - if (!$title) { - return; - } $this->sheettitle = $title; } @@ -80,6 +79,15 @@ abstract class spout_base extends \core\dataformat\base { * @param array $columns */ public function start_sheet($columns) { + if ($this->sheettitle && $this->writer instanceof \Box\Spout\Writer\AbstractMultiSheetsWriter) { + if ($this->renamecurrentsheet) { + $sheet = $this->writer->getCurrentSheet(); + $this->renamecurrentsheet = false; + } else { + $sheet = $this->writer->addNewSheetAndMakeItCurrent(); + } + $sheet->setName($this->sheettitle); + } $this->writer->addRow(array_values((array)$columns)); } diff --git a/lib/tablelib.php b/lib/tablelib.php index 4dd84f8de56..701267d4dfc 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -125,6 +125,12 @@ class flexible_table { */ private $prefs = array(); + /** @var $sheettitle */ + protected $sheettitle; + + /** @var $filename */ + protected $filename; + /** * Constructor * @param string $uniqueid all tables have to have a unique id, this is used @@ -180,7 +186,7 @@ class flexible_table { } else if (is_null($this->exportclass) && !empty($this->download)) { $this->exportclass = new table_dataformat_export_format($this, $this->download); if (!$this->exportclass->document_started()) { - $this->exportclass->start_document($this->filename); + $this->exportclass->start_document($this->filename, $this->sheettitle); } } return $this->exportclass; @@ -1741,11 +1747,14 @@ class table_dataformat_export_format extends table_default_export_format_parent * Start document * * @param string $filename + * @param string $sheettitle */ - public function start_document($filename) { - $this->filename = $filename; + public function start_document($filename, $sheettitle) { $this->documentstarted = true; $this->dataformat->set_filename($filename); + $this->dataformat->send_http_headers(); + $this->dataformat->set_sheettitle($sheettitle); + $this->dataformat->start_output(); } /** @@ -1755,7 +1764,6 @@ class table_dataformat_export_format extends table_default_export_format_parent */ public function start_table($sheettitle) { $this->dataformat->set_sheettitle($sheettitle); - $this->dataformat->send_http_headers(); } /** @@ -1765,7 +1773,13 @@ class table_dataformat_export_format extends table_default_export_format_parent */ public function output_headers($headers) { $this->columns = $headers; - $this->dataformat->write_header($headers); + if (method_exists($this->dataformat, 'write_header')) { + error_log('The function write_header() does not support multiple tables. In order to support multiple tables you ' . + 'must implement start_output() and start_sheet() and remove write_header() in your dataformat.'); + $this->dataformat->write_header($headers); + } else { + $this->dataformat->start_sheet($headers); + } } /** @@ -1782,15 +1796,21 @@ class table_dataformat_export_format extends table_default_export_format_parent * Finish export */ public function finish_table() { - $this->dataformat->write_footer($this->columns); + if (method_exists($this->dataformat, 'write_footer')) { + error_log('The function write_footer() does not support multiple tables. In order to support multiple tables you ' . + 'must implement close_sheet() and close_output() and remove write_footer() in your dataformat.'); + $this->dataformat->write_footer($this->columns); + } else { + $this->dataformat->close_sheet($this->columns); + } } /** * Finish download */ public function finish_document() { - exit; + $this->dataformat->close_output(); + exit(); } - }