From 75dc1756bb3686574535534246697ba3693f67af Mon Sep 17 00:00:00 2001 From: sam marshall Date: Thu, 12 Jul 2018 18:44:43 +0100 Subject: [PATCH] MDL-62907 Logging: 'other' field option to use JSON format --- .../log/classes/helper/buffered_writer.php | 9 ++- .../tool/log/classes/local/privacy/helper.php | 2 +- .../log/store/standard/classes/log/store.php | 24 ++++++- admin/tool/log/store/standard/db/upgrade.php | 7 ++ .../standard/lang/en/logstore_standard.php | 2 + admin/tool/log/store/standard/settings.php | 4 ++ .../log/store/standard/tests/store_test.php | 65 ++++++++++++++++++- admin/tool/log/store/standard/version.php | 2 +- admin/tool/log/upgrade.txt | 9 ++- 9 files changed, 116 insertions(+), 8 deletions(-) diff --git a/admin/tool/log/classes/helper/buffered_writer.php b/admin/tool/log/classes/helper/buffered_writer.php index 96cfd0cb7d2..e88a68368f2 100644 --- a/admin/tool/log/classes/helper/buffered_writer.php +++ b/admin/tool/log/classes/helper/buffered_writer.php @@ -43,6 +43,9 @@ trait buffered_writer { /** @var int $count Counter. */ protected $count = 0; + /** @var bool If true, writes JSON instead of PHP serialized data for 'other' field */ + protected $jsonformat = false; + /** * Should the event be ignored (== not logged)? * @param \core\event\base $event @@ -69,7 +72,11 @@ trait buffered_writer { // at the same time this lowers memory use because // snapshots and custom objects may be garbage collected. $entry = $event->get_data(); - $entry['other'] = serialize($entry['other']); + if ($this->jsonformat) { + $entry['other'] = json_encode($entry['other']); + } else { + $entry['other'] = serialize($entry['other']); + } $entry['origin'] = $PAGE->requestorigin; $entry['ip'] = $PAGE->requestip; $entry['realuserid'] = \core\session\manager::is_loggedinas() ? $GLOBALS['USER']->realuser : null; diff --git a/admin/tool/log/classes/local/privacy/helper.php b/admin/tool/log/classes/local/privacy/helper.php index 8a057cf2a6f..15ff9e3664f 100644 --- a/admin/tool/log/classes/local/privacy/helper.php +++ b/admin/tool/log/classes/local/privacy/helper.php @@ -49,7 +49,7 @@ class helper { $extra = ['origin' => $data->origin, 'ip' => $data->ip, 'realuserid' => $data->realuserid]; $data = (array) $data; $id = $data['id']; - $data['other'] = unserialize($data['other']); + $data['other'] = \logstore_standard\log\store::decode_other($data['other']); if ($data['other'] === false) { $data['other'] = []; } diff --git a/admin/tool/log/store/standard/classes/log/store.php b/admin/tool/log/store/standard/classes/log/store.php index fe93ff58fbc..b69659e05f4 100644 --- a/admin/tool/log/store/standard/classes/log/store.php +++ b/admin/tool/log/store/standard/classes/log/store.php @@ -38,6 +38,9 @@ class store implements \tool_log\log\writer, \core\log\sql_internal_table_reader $this->helper_setup($manager); // Log everything before setting is saved for the first time. $this->logguests = $this->get_config('logguests', 1); + // JSON writing defaults to false (table format compatibility with older versions). + // Note: This variable is defined in the buffered_writer trait. + $this->jsonformat = (bool)$this->get_config('jsonformat', false); } /** @@ -109,6 +112,25 @@ class store implements \tool_log\log\writer, \core\log\sql_internal_table_reader return new \core\dml\recordset_walk($recordset, array($this, 'get_log_event')); } + /** + * Function decodes the other field into an array using either PHP serialisation or JSON. + * + * Note that this does not rely on the config setting, it supports both formats, so you can + * use it for data before/after making a change to the config setting. + * + * The return value is usually an array but it can also be null or a boolean or something. + * + * @param string $other Other value + * @return mixed Decoded value + */ + public static function decode_other(string $other) { + if ($other === 'N;' || preg_match('~^.:~', $other)) { + return unserialize($other); + } else { + return json_decode($other, true); + } + } + /** * Returns an event from the log data. * @@ -120,7 +142,7 @@ class store implements \tool_log\log\writer, \core\log\sql_internal_table_reader $extra = array('origin' => $data->origin, 'ip' => $data->ip, 'realuserid' => $data->realuserid); $data = (array)$data; $id = $data['id']; - $data['other'] = unserialize($data['other']); + $data['other'] = self::decode_other($data['other']); if ($data['other'] === false) { $data['other'] = array(); } diff --git a/admin/tool/log/store/standard/db/upgrade.php b/admin/tool/log/store/standard/db/upgrade.php index d6cd26d4232..e1ac5a2ddc1 100644 --- a/admin/tool/log/store/standard/db/upgrade.php +++ b/admin/tool/log/store/standard/db/upgrade.php @@ -39,5 +39,12 @@ function xmldb_logstore_standard_upgrade($oldversion) { // Automatically generated Moodle v3.6.0 release upgrade line. // Put any upgrade step following this. + if ($oldversion < 2019032800) { + // For existing installations, set the new jsonformat option to off (no behaviour change). + // New installations default to on. + set_config('jsonformat', 0, 'logstore_standard'); + upgrade_plugin_savepoint(true, 2019032800, 'logstore', 'standard'); + } + return true; } diff --git a/admin/tool/log/store/standard/lang/en/logstore_standard.php b/admin/tool/log/store/standard/lang/en/logstore_standard.php index 14a1d2b6459..12a4c422482 100644 --- a/admin/tool/log/store/standard/lang/en/logstore_standard.php +++ b/admin/tool/log/store/standard/lang/en/logstore_standard.php @@ -23,6 +23,8 @@ */ $string['buffersize'] = 'Write buffer size'; +$string['jsonformat'] = 'JSON format'; +$string['jsonformat_desc'] = 'Use standard JSON format instead of PHP serialised data in the \'other\' database field.'; $string['pluginname'] = 'Standard log'; $string['pluginname_desc'] = 'A log plugin stores log entries in a Moodle database table.'; $string['privacy:metadata:log'] = 'A collection of past events'; diff --git a/admin/tool/log/store/standard/settings.php b/admin/tool/log/store/standard/settings.php index daa8131781d..fc4a894abd3 100644 --- a/admin/tool/log/store/standard/settings.php +++ b/admin/tool/log/store/standard/settings.php @@ -30,6 +30,10 @@ if ($hassiteconfig) { new lang_string('logguests', 'core_admin'), new lang_string('logguests_help', 'core_admin'), 1)); + $settings->add(new admin_setting_configcheckbox('logstore_standard/jsonformat', + new lang_string('jsonformat', 'logstore_standard'), + new lang_string('jsonformat_desc', 'logstore_standard'), 1)); + $options = array( 0 => new lang_string('neverdeletelogs'), 1000 => new lang_string('numdays', '', 1000), diff --git a/admin/tool/log/store/standard/tests/store_test.php b/admin/tool/log/store/standard/tests/store_test.php index def6684cf09..f2d05319e8b 100644 --- a/admin/tool/log/store/standard/tests/store_test.php +++ b/admin/tool/log/store/standard/tests/store_test.php @@ -33,11 +33,21 @@ class logstore_standard_store_testcase extends advanced_testcase { */ private $wedisabledgc = false; - public function test_log_writing() { + /** + * Tests log writing. + * + * @param bool $jsonformat True to test with JSON format + * @dataProvider test_log_writing_provider + * @throws moodle_exception + */ + public function test_log_writing(bool $jsonformat) { global $DB; $this->resetAfterTest(); $this->preventResetByRollback(); // Logging waits till the transaction gets committed. + // Apply JSON format system setting. + set_config('jsonformat', $jsonformat ? 1 : 0, 'logstore_standard'); + $this->setAdminUser(); $user1 = $this->getDataGenerator()->create_user(); $user2 = $this->getDataGenerator()->create_user(); @@ -82,7 +92,11 @@ class logstore_standard_store_testcase extends advanced_testcase { $log1 = reset($logs); unset($log1->id); - $log1->other = unserialize($log1->other); + if ($jsonformat) { + $log1->other = json_decode($log1->other, true); + } else { + $log1->other = unserialize($log1->other); + } $log1 = (array)$log1; $data = $event1->get_data(); $data['origin'] = 'cli'; @@ -112,7 +126,11 @@ class logstore_standard_store_testcase extends advanced_testcase { $log3 = array_shift($logs); unset($log3->id); - $log3->other = unserialize($log3->other); + if ($jsonformat) { + $log3->other = json_decode($log3->other, true); + } else { + $log3->other = unserialize($log3->other); + } $log3 = (array)$log3; $data = $event2->get_data(); $data['origin'] = 'restore'; @@ -200,6 +218,19 @@ class logstore_standard_store_testcase extends advanced_testcase { get_log_manager(true); } + /** + * Returns different JSON format settings so the test can be run with JSON format either on or + * off. + * + * @return [bool] Array of true/false + */ + public static function test_log_writing_provider(): array { + return [ + [false], + [true] + ]; + } + /** * Test logmanager::get_supported_reports returns all reports that require this store. */ @@ -332,6 +363,34 @@ class logstore_standard_store_testcase extends advanced_testcase { $this->assertEquals(1, $DB->count_records('logstore_standard_log')); } + /** + * Tests the decode_other function can cope with both JSON and PHP serialized format. + * + * @param mixed $value Value to encode and decode + * @dataProvider test_decode_other_provider + */ + public function test_decode_other($value) { + $this->assertEquals($value, \logstore_standard\log\store::decode_other(serialize($value))); + $this->assertEquals($value, \logstore_standard\log\store::decode_other(json_encode($value))); + } + + /** + * List of possible values for 'other' field. + * + * I took these types from our logs based on the different first character of PHP serialized + * data - my query found only these types. The normal case is an array. + * + * @return array Array of parameters + */ + public function test_decode_other_provider(): array { + return [ + [['info' => 'd2819896', 'logurl' => 'discuss.php?d=2819896']], + [null], + ['just a string'], + [32768] + ]; + } + /** * Disable the garbage collector if it's enabled to ensure we don't adjust memory statistics. */ diff --git a/admin/tool/log/store/standard/version.php b/admin/tool/log/store/standard/version.php index 89ab35af76a..2a6b63dacff 100644 --- a/admin/tool/log/store/standard/version.php +++ b/admin/tool/log/store/standard/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2018120300; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2019032800; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2018112800; // Requires this Moodle version. $plugin->component = 'logstore_standard'; // Full name of the plugin (used for diagnostics). diff --git a/admin/tool/log/upgrade.txt b/admin/tool/log/upgrade.txt index e1b65e3f388..abaf3e27327 100644 --- a/admin/tool/log/upgrade.txt +++ b/admin/tool/log/upgrade.txt @@ -2,7 +2,14 @@ This files describes API changes in /admin/tool/log - plugins, information provided here is intended especially for developers. +=== 3.7 === + +* The new jsonformat option, which defaults to 'on' for a new install (and 'off' for existing installs) means that + the 'other' event field is now stored in JSON format instead of PHP serialize format in the database. The system + can read data in both formats but if any third-party software directly accesses the database field, it may need + to be modified (or require users to turn off jsonformat). + === 3.6 === * The legacy log store is in its first stage of deprecation and is due for removal in Moodle 4.0. Please use one of - the other log stores such as "standard" and "database". \ No newline at end of file + the other log stores such as "standard" and "database".