MDL-62907 Logging: 'other' field option to use JSON format

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;

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'] = [];
         }

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

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

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';

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

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.
      */

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

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".
+  the other log stores such as "standard" and "database".