From 8844cb8286206a7422ab67dfc9487e27134d5a9c Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Mon, 4 May 2020 13:24:31 +0100 Subject: [PATCH] MDL-68500 dataformat: re-factor download/export methods into new class. --- admin/user/user_bulk_download.php | 5 +- dataformat/upgrade.txt | 3 + lib/classes/dataformat.php | 151 ++++++++++++++++++++++++++++++ lib/dataformatlib.php | 126 +------------------------ lib/tests/dataformat_test.php | 16 ++-- lib/upgrade.txt | 1 + mod/forum/export.php | 4 +- user/action_redir.php | 4 +- 8 files changed, 169 insertions(+), 141 deletions(-) create mode 100644 lib/classes/dataformat.php diff --git a/admin/user/user_bulk_download.php b/admin/user/user_bulk_download.php index babede0d9bd..4e5e8d37ae9 100644 --- a/admin/user/user_bulk_download.php +++ b/admin/user/user_bulk_download.php @@ -25,7 +25,6 @@ define('NO_OUTPUT_BUFFERING', true); require_once('../../config.php'); require_once($CFG->libdir.'/adminlib.php'); -require_once($CFG->libdir.'/dataformatlib.php'); require_once($CFG->dirroot.'/user/profile/lib.php'); $dataformat = optional_param('dataformat', '', PARAM_ALPHA); @@ -69,9 +68,9 @@ if ($dataformat) { $downloadusers = new ArrayObject($SESSION->bulk_users); $iterator = $downloadusers->getIterator(); - download_as_dataformat($filename, $dataformat, $fields, $iterator, function($userid) use ($extrafields, $fields) { + \core\dataformat::download_data($filename, $dataformat, $fields, $iterator, function($userid) use ($extrafields, $fields) { global $DB; - $row = array(); + if (!$user = $DB->get_record('user', array('id' => $userid))) { return null; } diff --git a/dataformat/upgrade.txt b/dataformat/upgrade.txt index 9ec18bb6f19..cf4d09edc67 100644 --- a/dataformat/upgrade.txt +++ b/dataformat/upgrade.txt @@ -6,6 +6,9 @@ information provided here is intended especially for developers. file. They can be overridden in extending classes to define how files should be created: - start_output_to_file() - close_output_to_file() +* Calls to the following dataformat plugin methods have been removed: + - write_header() + - write_footer() === 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.php b/lib/classes/dataformat.php new file mode 100644 index 00000000000..42ae23d7d66 --- /dev/null +++ b/lib/classes/dataformat.php @@ -0,0 +1,151 @@ +. + +/** + * Class containing utility methods for dataformats + * + * @package core + * @copyright 2020 Moodle Pty Ltd + * @author 2020 Paul Holden + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @license Moodle Workplace License, distribution is restricted, contact support@moodle.com + */ + +namespace core; + +use coding_exception; +use core_php_time_limit; + +/** + * Dataformat utility class + * + * @package core + * @copyright 2020 Moodle Pty Ltd + * @author 2020 Paul Holden + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @license Moodle Workplace License, distribution is restricted, contact support@moodle.com + */ +class dataformat { + + /** + * Return an instead of a dataformat writer from given dataformat type + * + * @param string $dataformat + * @return dataformat\base + * @throws coding_exception + */ + protected static function get_format_instance(string $dataformat): \core\dataformat\base { + $classname = 'dataformat_' . $dataformat . '\writer'; + if (!class_exists($classname)) { + throw new coding_exception('Invalid dataformat', $dataformat); + } + + return new $classname(); + } + + /** + * Sends a formatted data file to the browser + * + * @param string $filename + * @param string $dataformat + * @param array $columns + * @param Iterable $iterator + * @param callable|null $callback + * @throws coding_exception + */ + public static function download_data(string $filename, string $dataformat, array $columns, Iterable $iterator, + callable $callback = null): void { + + if (ob_get_length()) { + throw new coding_exception('Output can not be buffered before calling download_data()'); + } + + $format = self::get_format_instance($dataformat); + + // The data format export could take a while to generate. + core_php_time_limit::raise(); + + // Close the session so that the users other tabs in the same session are not blocked. + \core\session\manager::write_close(); + + // If this file was requested from a form, then mark download as complete (before sending headers). + \core_form\util::form_download_complete(); + + $format->set_filename($filename); + $format->send_http_headers(); + + $format->start_output(); + $format->start_sheet($columns); + + $rownum = 0; + foreach ($iterator as $row) { + if (is_callable($callback)) { + $row = $callback($row); + } + if ($row === null) { + continue; + } + $format->write_record($row, $rownum++); + } + + $format->close_sheet($columns); + $format->close_output(); + } + + /** + * Writes a formatted data file with specified filename + * + * @param string $filename + * @param string $dataformat + * @param array $columns + * @param Iterable $iterator + * @param callable|null $callback + * @return string Complete path to the file on disk + */ + public static function write_data(string $filename, string $dataformat, array $columns, Iterable $iterator, + callable $callback = null): string { + + $format = self::get_format_instance($dataformat); + + // The data format export could take a while to generate. + core_php_time_limit::raise(); + + // Close the session so that the users other tabs in the same session are not blocked. + \core\session\manager::write_close(); + + $filepath = make_request_directory() . '/' . $filename . $format->get_extension(); + $format->set_filepath($filepath); + + $format->start_output_to_file(); + $format->start_sheet($columns); + + $rownum = 0; + foreach ($iterator as $row) { + if (is_callable($callback)) { + $row = $callback($row); + } + if ($row === null) { + continue; + } + $format->write_record($row, $rownum++); + } + + $format->close_sheet($columns); + $format->close_output_to_file(); + + return $filepath; + } +} diff --git a/lib/dataformatlib.php b/lib/dataformatlib.php index dac5c4a8b3d..0fe4b360736 100644 --- a/lib/dataformatlib.php +++ b/lib/dataformatlib.php @@ -35,129 +35,11 @@ * @param Iterator $iterator An iterator over the records, usually a RecordSet * @param callable $callback An option function applied to each record before writing * @throws coding_exception - */ -function download_as_dataformat($filename, $dataformat, $columns, $iterator, $callback = null) { - - if (ob_get_length()) { - throw new coding_exception("Output can not be buffered before calling download_as_dataformat"); - } - - $classname = 'dataformat_' . $dataformat . '\writer'; - if (!class_exists($classname)) { - throw new coding_exception("Unable to locate dataformat/$dataformat/classes/writer.php"); - } - - // The data format export could take a while to generate... - core_php_time_limit::raise(); - - // Close the session so that the users other tabs in the same session are not blocked. - \core\session\manager::write_close(); - - // If this file was requested from a form, then mark download as complete (before sending headers). - \core_form\util::form_download_complete(); - - /** @var \core\dataformat\base $format */ - $format = new $classname; - - $format->set_filename($filename); - $format->send_http_headers(); - - // This exists to support all dataformats - see MDL-56046. - if (!method_exists($format, 'start_output') && method_exists($format, 'write_header')) { - debugging('The function write_header() does not support multiple sheets. In order to support multiple sheets you ' . - 'must implement start_output() and start_sheet() and remove write_header() in your dataformat.', DEBUG_DEVELOPER); - $format->write_header($columns); - } else { - $format->start_output(); - $format->start_sheet($columns); - } - - $c = 0; - foreach ($iterator as $row) { - if ($callback) { - $row = $callback($row); - } - if ($row === null) { - continue; - } - $format->write_record($row, $c++); - } - - // This exists to support all dataformats - see MDL-56046. - if (!method_exists($format, 'close_output') && method_exists($format, 'write_footer')) { - debugging('The function write_footer() does not support multiple sheets. In order to support multiple sheets you ' . - 'must implement close_sheet() and close_output() and remove write_footer() in your dataformat.', DEBUG_DEVELOPER); - $format->write_footer($columns); - } else { - $format->close_sheet($columns); - $format->close_output(); - } -} - -/** - * Writes a formatted data file to specified path * - * @package core - * @subpackage dataformat - * - * @param string $filename The base filename without an extension - * @param string $dataformat A dataformat name - * @param array $columns An ordered map of column keys and labels - * @param Iterator $iterator An iterator over the records, usually a RecordSet - * @param callable $callback An option function applied to each record before writing - * @return string Complete path to written file - * @throws coding_exception + * @deprecated since Moodle 3.9 - MDL-68500 please use \core\dataformat::download_data */ -function write_file_as_dataformat(string $filename, string $dataformat, array $columns, $iterator, - callable $callback = null): string { - - $classname = 'dataformat_' . $dataformat . '\writer'; - if (!class_exists($classname)) { - throw new coding_exception("Unable to locate dataformat/$dataformat/classes/writer.php"); - } - - // The data format export could take a while to generate. - core_php_time_limit::raise(); - - // Close the session so that the users other tabs in the same session are not blocked. - \core\session\manager::write_close(); - - /** @var \core\dataformat\base $format */ - $format = new $classname; - - $filepath = make_request_directory() . '/' . $filename . $format->get_extension(); - $format->set_filepath($filepath); - - // This exists to support all dataformats - see MDL-56046. - if (!method_exists($format, 'start_output_to_file') && method_exists($format, 'write_header')) { - debugging('The function write_header() does not support multiple sheets. In order to support multiple sheets you must ' . - 'implement start_output_to_file() and start_sheet() and remove write_header() in your dataformat.', DEBUG_DEVELOPER); - $format->write_header($columns); - } else { - $format->start_output_to_file(); - $format->start_sheet($columns); - } - - $c = 0; - foreach ($iterator as $row) { - if ($callback) { - $row = $callback($row); - } - if ($row === null) { - continue; - } - $format->write_record($row, $c++); - } - - // This exists to support all dataformats - see MDL-56046. - if (!method_exists($format, 'close_output_to_file') && method_exists($format, 'write_footer')) { - debugging('The function write_footer() does not support multiple sheets. In order to support multiple sheets you must ' . - 'implement close_sheet() and close_output_to_file() and remove write_footer() in your dataformat.', DEBUG_DEVELOPER); - $format->write_footer($columns); - } else { - $format->close_sheet($columns); - $format->close_output_to_file(); - } +function download_as_dataformat($filename, $dataformat, $columns, $iterator, $callback = null) { + debugging('download_as_dataformat() is deprecated, please use \core\dataformat::download_data() instead', DEBUG_DEVELOPER); - return $filepath; + \core\dataformat::download_data($filename, $dataformat, $columns, $iterator, $callback); } diff --git a/lib/tests/dataformat_test.php b/lib/tests/dataformat_test.php index 3e274bda5a6..083b0c04aac 100644 --- a/lib/tests/dataformat_test.php +++ b/lib/tests/dataformat_test.php @@ -25,11 +25,7 @@ namespace core; use core_component; - -defined('MOODLE_INTERNAL') || die(); - -global $CFG; -require_once("{$CFG->libdir}/dataformatlib.php"); +use core\dataformat; /** * Dataformat tests @@ -41,11 +37,11 @@ require_once("{$CFG->libdir}/dataformatlib.php"); class dataformat_testcase extends \advanced_testcase { /** - * Data provider for {@see test_write_file_as_dataformat} + * Data provider for {@see test_write_data) * * @return array */ - public function write_file_as_dataformat_provider(): array { + public function write_data_provider(): array { $data = []; $dataformats = core_component::get_plugin_list('dataformat'); @@ -62,9 +58,9 @@ class dataformat_testcase extends \advanced_testcase { * @param string $dataformat * @return void * - * @dataProvider write_file_as_dataformat_provider + * @dataProvider write_data_provider */ - public function test_write_file_as_dataformat(string $dataformat): void { + public function test_write_data(string $dataformat): void { $columns = ['fruit', 'colour', 'animal']; $rows = [ ['banana', 'yellow', 'monkey'], @@ -73,7 +69,7 @@ class dataformat_testcase extends \advanced_testcase { ]; // Export to file. Assert that the exported file exists and is non-zero in size. - $exportfile = write_file_as_dataformat('My export', $dataformat, $columns, $rows); + $exportfile = dataformat::write_data('My export', $dataformat, $columns, $rows); $this->assertFileExists($exportfile); $this->assertGreaterThan(0, filesize($exportfile)); } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index eb04ddcbad5..084fe762bbf 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -47,6 +47,7 @@ information provided here is intended especially for developers. db/services.php. Note - this also requires $CFG->enable_read_only_sessions to be set to true. * database_manager::check_database_schema() now checks for missing and extra indexes. * Implement a more direct xsendfile_file() method for an alternative_file_system_class +* The download_as_dataformat() method has been deprecated. Please use \core\dataformat::download_data() instead === 3.8 === * Add CLI option to notify all cron tasks to stop: admin/cli/cron.php --stop diff --git a/mod/forum/export.php b/mod/forum/export.php index 360c1d533ca..14509f50ccd 100644 --- a/mod/forum/export.php +++ b/mod/forum/export.php @@ -25,7 +25,6 @@ define('NO_OUTPUT_BUFFERING', true); require_once(__DIR__ . '/../../config.php'); require_once($CFG->libdir . '/adminlib.php'); -require_once($CFG->libdir . '/dataformatlib.php'); require_once($CFG->dirroot . '/calendar/externallib.php'); $forumid = required_param('id', PARAM_INT); @@ -127,9 +126,8 @@ if ($form->is_cancelled()) { $exportdata = new ArrayObject($datamapper->to_legacy_objects($posts)); $iterator = $exportdata->getIterator(); - require_once($CFG->libdir . '/dataformatlib.php'); $filename = clean_filename('discussion'); - download_as_dataformat( + \core\dataformat::download_data( $filename, $dataformat, $fields, diff --git a/user/action_redir.php b/user/action_redir.php index 46babe7ccd6..72eb3057391 100644 --- a/user/action_redir.php +++ b/user/action_redir.php @@ -82,8 +82,6 @@ if ($formaction == 'bulkchange.php') { $plugins = core_plugin_manager::instance()->get_plugins_of_type('dataformat'); if (isset($plugins[$dataformat])) { if ($plugins[$dataformat]->is_enabled()) { - require_once($CFG->dirroot . '/lib/dataformatlib.php'); - if (empty($userids)) { redirect($returnurl, get_string('noselectedusers', 'bulkusers')); } @@ -111,7 +109,7 @@ if ($formaction == 'bulkchange.php') { WHERE u.id $insql"; $rs = $DB->get_recordset_sql($sql, $inparams); - download_as_dataformat('courseid_' . $course->id . '_participants', $dataformat, $columnnames, $rs); + \core\dataformat::download_data('courseid_' . $course->id . '_participants', $dataformat, $columnnames, $rs); $rs->close(); } } -- 2.43.0