Merge branch 'MDL-62907-master' of https://github.com/sammarshallou/moodle

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/helper/reader.php

@@ -62,6 +62,25 @@ trait reader {
         return $this->store;
     }
 
+    /**
+     * 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);
+        }
+    }
+
     /**
      * Adds ID column to $sort to make sure events from one request
      * within 1 second are returned in the same order.

admin/tool/log/classes/local/privacy/helper.php

@@ -37,6 +37,7 @@ use core_privacy\local\request\transform;
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class helper {
+    use \tool_log\helper\reader;
 
     /**
      * Returns an event from a standard record.
@@ -49,7 +50,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'] = self::decode_other($data['other']);
         if ($data['other'] === false) {
             $data['other'] = [];
         }

admin/tool/log/store/database/classes/log/store.php

@@ -57,6 +57,9 @@ class store implements \tool_log\log\writer, \core\log\sql_reader {
         $levels = $this->get_config('includelevels', '');
         $this->includeactions = $actions === '' ? array() : explode(',', $actions);
         $this->includelevels = $levels === '' ? array() : explode(',', $levels);
+        // 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);
     }
 
     /**
@@ -223,7 +226,7 @@ class store implements \tool_log\log\writer, \core\log\sql_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/database/db/upgrade.php

@@ -39,5 +39,12 @@ function xmldb_logstore_database_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_database');
+        upgrade_plugin_savepoint(true, 2019032800, 'logstore', 'database');
+    }
+
     return true;
 }

admin/tool/log/store/database/lang/en/logstore_database.php

@@ -39,6 +39,8 @@ $string['includeactions'] = 'Include actions of these types';
 $string['includelevels'] = 'Include actions with these educational levels';
 $string['filters'] = 'Filter logs';
 $string['filters_help'] = 'Enable filters that exclude some actions from being logged.';
+$string['jsonformat'] = 'JSON format';
+$string['jsonformat_desc'] = 'Use standard JSON format instead of PHP serialised data in the \'other\' database field.';
 $string['logguests'] = 'Log guest actions';
 $string['other'] = 'Other';
 $string['participating'] = 'Participating';

admin/tool/log/store/database/settings.php

@@ -59,6 +59,10 @@ if ($hassiteconfig) {
     $settings->add(new admin_setting_configtext('logstore_database/buffersize', get_string('buffersize',
         'logstore_database'), get_string('buffersize_help', 'logstore_database'), 50));
 
+    $settings->add(new admin_setting_configcheckbox('logstore_database/jsonformat',
+            new lang_string('jsonformat', 'logstore_database'),
+            new lang_string('jsonformat_desc', 'logstore_database'), 1));
+
     // Filters.
     $settings->add(new admin_setting_heading('filters', get_string('filters', 'logstore_database'), get_string('filters_help',
         'logstore_database')));

admin/tool/log/store/database/tests/store_test.php

@@ -28,11 +28,21 @@ require_once(__DIR__ . '/fixtures/event.php');
 require_once(__DIR__ . '/fixtures/store.php');
 
 class logstore_database_store_testcase extends advanced_testcase {
-    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, $CFG;
         $this->resetAfterTest();
         $this->preventResetByRollback(); // Logging waits till the transaction gets committed.
 
+        // Apply JSON format system setting.
+        set_config('jsonformat', $jsonformat ? 1 : 0, 'logstore_database');
+
         $dbman = $DB->get_manager();
         $this->assertTrue($dbman->table_exists('logstore_standard_log'));
         $DB->delete_records('logstore_standard_log');
@@ -118,7 +128,11 @@ class logstore_database_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';
@@ -145,7 +159,11 @@ class logstore_database_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'] = 'cli';
@@ -229,6 +247,19 @@ class logstore_database_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 method is_event_ignored.
      */

admin/tool/log/store/database/version.php

@@ -17,13 +17,13 @@
 /**
  * External database log store.
  *
- * @package    logstore_standard
+ * @package    logstore_database
  * @copyright  2013 Petr Skoda {@link http://skodak.org}
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 
 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_database'; // Full name of the plugin (used for diagnostics).

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);
     }
 
     /**
@@ -120,7 +123,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".