Merge branch 'MDL-69688' of https://github.com/stronk7/moodle

lib/phpunit/classes/advanced_testcase.php

@@ -390,7 +390,8 @@ abstract class advanced_testcase extends base_testcase {
     }
 
     /**
-     * Assert that an event is not using event->contxet.
+     * Assert that various event methods are not using event->context
+     *
      * While restoring context might not be valid and it should not be used by event url
      * or description methods.
      *
@@ -410,7 +411,7 @@ abstract class advanced_testcase extends base_testcase {
         $event->get_url();
         $event->get_description();
 
-        // Restore event->context.
+        // Restore event->context (note that this is unreachable when the event uses context). But ok for correct events.
         phpunit_event_mock::testable_set_event_context($event, $eventcontext);
     }
 

lib/phpunit/tests/advanced_test.php

@@ -339,6 +339,54 @@ class advanced_test extends \advanced_testcase {
         }
     }
 
+    /**
+     * Test the assertEventContextNotUsed() assertion.
+     *
+     * Verify that events using the event context in some of their
+     * methods are detected properly (will throw a warning if they are).
+     *
+     * To do so, we'll be using some fixture events (context_used_in_event_xxxx),
+     * that, on purpose, use the event context (incorrectly) in their methods.
+     *
+     * Note that because we are using imported fixture classes, and because we
+     * are testing for warnings, better we run the tests in a separate process.
+     *
+     * @param string $fixture The fixture class to use.
+     * @param bool $phpwarn Whether a PHP warning is expected.
+     *
+     * @runInSeparateProcess
+     * @dataProvider assert_event_context_not_used_provider
+     * @covers ::assertEventContextNotUsed
+     */
+    public function test_assert_event_context_not_used($fixture, $phpwarn): void {
+        require(__DIR__ . '/fixtures/event_fixtures.php');
+        // Create an event that uses the event context in its get_url() and get_description() methods.
+        $event = $fixture::create([
+            'other' => [
+                'sample' => 1,
+                'xx' => 10,
+            ],
+        ]);
+
+        if ($phpwarn) {
+            $this->expectWarning();
+        }
+        $this->assertEventContextNotUsed($event);
+    }
+
+    /**
+     * Data provider for test_assert_event_context_not_used().
+     *
+     * @return array
+     */
+    public static function assert_event_context_not_used_provider(): array {
+        return [
+            'correct' => ['\core\event\context_used_in_event_correct', false],
+            'wrong_get_url' => ['\core\event\context_used_in_event_get_url', true],
+            'wrong_get_description' => ['\core\event\context_used_in_event_get_description', true],
+        ];
+    }
+
     public function test_message_processors_reset() {
         global $DB;
 

lib/phpunit/tests/fixtures/event_fixtures.php

@@ -0,0 +1,80 @@
+<?php
+// This file is part of Moodle - https://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.
+
+/**
+ * Fixtures for advanced_testcase tests.
+ *
+ * @package   core
+ * @category  event
+ * @copyright 2024 onwards Eloy Lafuente (stronk7) {@link https://stronk7.com}
+ * @license   https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+namespace core\event;
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * Event to test that \advanced_testcase::assertEventContextNotUsed() passes ok when no context is used.
+ */
+class context_used_in_event_correct extends \core\event\base {
+
+    protected function init() {
+        $this->data['crud'] = 'u';
+        $this->data['edulevel'] = self::LEVEL_PARTICIPATING;
+        $this->context = \context_system::instance();
+    }
+
+    public function get_url() {
+        return new \moodle_url('/somepath/somefile.php'); // No context used.
+    }
+
+    public function get_description() {
+        return 'Description'; // No context used.
+    }
+}
+
+/**
+ * Event to test that \advanced_testcase::assertEventContextNotUsed() detects context usage on get_url().
+ */
+class context_used_in_event_get_url extends \core\event\base {
+
+    protected function init() {
+        $this->data['crud'] = 'u';
+        $this->data['edulevel'] = self::LEVEL_PARTICIPATING;
+        $this->context = \context_system::instance();
+    }
+
+    public function get_url() {
+        return new \moodle_url('/somepath/somefile.php', ['id' => $this->context->instanceid]); // Causes a PHP Warning.
+    }
+}
+
+/**
+ * Event to test that \advanced_testcase::assertEventContextNotUsed() detects context usage on get_description().
+ */
+class context_used_in_event_get_description extends \core\event\base {
+
+    protected function init() {
+        $this->data['crud'] = 'u';
+        $this->data['edulevel'] = self::LEVEL_PARTICIPATING;
+        $this->context = \context_system::instance();
+    }
+
+    public function get_description() {
+        return $this->context->instanceid . " Description"; // Causes a PHP Warning.
+    }
+}

lib/tests/event/base_test.php

@@ -853,27 +853,6 @@ class base_test extends \advanced_testcase {
         $this->assertSame($event->get_data(), $data);
     }
 
-    public function test_context_not_used() {
-        // TODO: MDL-69688 - This test is far away from my understanding. It throws a
-        // "Trying to get property 'instanceid' of non-object" notice, so
-        // it's not clear for me what the test is doing. This was detected
-        // when preparing tests for PHPUnit 8 (MDL-67673) and, at the end
-        // all that was done is to move the annotation (deprecated) to
-        // explicit expectation. Still try commenting it out and you'll see
-        // the notice.
-        if (PHP_VERSION_ID >= 80000) {
-            $this->expectWarning();
-        } else {
-            $this->expectNotice();
-        }
-        $event = \core_tests\event\context_used_in_event::create(array('other' => array('sample' => 1, 'xx' => 10)));
-        $this->assertEventContextNotUsed($event);
-
-        $eventcontext = phpunit_event_mock::testable_get_event_context($event);
-        phpunit_event_mock::testable_set_event_context($event, null);
-        $this->assertEventContextNotUsed($event);
-    }
-
     /**
      * Test that all observer information is returned correctly.
      */

lib/tests/fixtures/event_fixtures.php

@@ -244,25 +244,6 @@ class course_module_viewed_noinit extends \core\event\course_module_viewed {
 class grade_report_viewed extends \core\event\grade_report_viewed {
 }
 
-/**
- * Event to test context used in event functions
- */
-class context_used_in_event extends \core\event\base {
-    public function get_description() {
-        return $this->context->instanceid . " Description";
-    }
-
-    protected function init() {
-        $this->data['crud'] = 'u';
-        $this->data['edulevel'] = self::LEVEL_PARTICIPATING;
-        $this->context = \context_system::instance();
-    }
-
-    public function get_url() {
-        return new \moodle_url('/somepath/somefile.php', array('id' => $this->context->instanceid));
-    }
-}
-
 /**
  * This is an explanation of the event.
  *      - I'm making a point here.