From c65d069e1dfad0db836bd862a6e764d85000c699 Mon Sep 17 00:00:00 2001
From: Mark Johnson <mark.johnson@catalyst-eu.net>
Date: Thu, 5 Dec 2024 09:30:56 +0000
Subject: [PATCH] MDL-83859 core_question: Update filter condition API

This corrects some definitions of the methods in the base condition
class to make things more obvious to developers implementing new
filters.

Previously if your filter wanted to use the default
`core/datafilter/filtertype` class, you still had to implement
`get_filter_class` to return `null`, since it was declared as abstract.
This change defines it as returning `null` by default, so this is no
longer necessary.

Also, this removes the default definitions for `get_condition_key` and
`build_query_from_functions`, and declares them abstract. Currently it
is necessary to override these to implement a functional filter so it
doesn't make sense to have a useless default definition.

This will not cause any breakages with existing filters. All filters
must already be defining the methods that are now abstract, otherwise
they will not function. Any filter that is now overriding
`get_filter_class` to return `null` will continue to work as before,
even though this is no longer necessary.
---
 .upgradenotes/MDL-83859-2024120511024767.yml | 20 ++++++++++++++++
 question/classes/local/bank/condition.php    | 25 +++++++++++---------
 2 files changed, 34 insertions(+), 11 deletions(-)
 create mode 100644 .upgradenotes/MDL-83859-2024120511024767.yml

diff --git a/.upgradenotes/MDL-83859-2024120511024767.yml b/.upgradenotes/MDL-83859-2024120511024767.yml
new file mode 100644
index 0000000000000..7bc3bb9a7d97a
--- /dev/null
+++ b/.upgradenotes/MDL-83859-2024120511024767.yml
@@ -0,0 +1,20 @@
+issueNumber: MDL-83859
+notes:
+  core_question:
+    - message: >
+        The definition of the abstract `core_question\local\bank\condition`
+        class has changed to make it clearer which methods are required 
+        in child classes.
+
+        The `get_filter_class` method is no longer declared as abstract, and
+        will return `null` by default to use the base 
+        `core/datafilter/filtertype` class. If you have defined this method
+        to return `null` in your own class, it will continue to work, but
+        it is no longer necessary.
+
+        `build_query_from_filter` and `get_condition_key` are now declared as
+        abstract, since all filter condition classes must define these 
+        (as well as existing abstract methods) to function. Again, exsiting
+        child classes will continue to work if they did before, as they 
+        already needed these methods.
+      type: changed
diff --git a/question/classes/local/bank/condition.php b/question/classes/local/bank/condition.php
index c337613fda702..bf16f999a0d48 100644
--- a/question/classes/local/bank/condition.php
+++ b/question/classes/local/bank/condition.php
@@ -27,7 +27,6 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 abstract class condition {
-
     /** @var int The default filter type */
     const JOINTYPE_DEFAULT = datafilter::JOINTYPE_ANY;
 
@@ -48,11 +47,16 @@ abstract class condition {
     abstract public function get_title();
 
     /**
-     * Return filter class associated with this condition
+     * Return the Javascript filter class to provide the UI for this condition.
+     *
+     * If left as null, this will use the default core/datafilter/filtertype class. Otherwise, override it to return
+     * the full path to the Javascript module path for the class.
      *
      * @return string filter class
      */
-    abstract public function get_filter_class();
+    public function get_filter_class() {
+        return null;
+    }
 
     /**
      * Extract the required filter from the provided question bank view.
@@ -140,9 +144,7 @@ public function get_condition_class() {
      *
      * @return string
      */
-    public static function get_condition_key() {
-        return '';
-    }
+    abstract public static function get_condition_key();
 
     /**
      * Return an SQL fragment to be ANDed into the WHERE clause to filter which questions are shown.
@@ -211,12 +213,13 @@ public static function get_filter_from_list(array $filters): ?array {
     }
 
     /**
-     * Build query from filter value
+     * Return the SQL WHERE condition and parameters to be ANDed with other filter conditions.
+     *
+     * The $filter parameter recieves an array with a 'values' key, containing an array of the filter values selected,
+     * and a 'jointype' key containing the selected join type.
      *
      * @param array $filter filter properties
-     * @return array where sql and params
+     * @return array ['SQL where condition', ['param1' => 'value1', 'param2' => 'value2', ...]]
      */
-    public static function build_query_from_filter(array $filter): array {
-        return ['', []];
-    }
+    abstract public static function build_query_from_filter(array $filter);
 }