MDL-77614 reportbuilder: throw exception if entity name already exists.

reportbuilder/classes/local/entities/base.php

@@ -33,7 +33,7 @@ use core_reportbuilder\local\report\filter;
 abstract class base {
 
     /** @var string $entityname Internal reference to name of entity */
-    private $entityname = '';
+    private $entityname = null;
 
     /** @var lang_string $entitytitle Used as a title for the entity in reports */
     private $entitytitle = null;
@@ -87,7 +87,7 @@ abstract class base {
      *
      * @return string
      */
-    protected function get_default_entity_name(): string {
+    private function get_default_entity_name(): string {
         $namespace = explode('\\', get_called_class());
 
         return end($namespace);
@@ -98,13 +98,8 @@ abstract class base {
      *
      * @param string $entityname
      * @return self
-     * @throws coding_exception
      */
     final public function set_entity_name(string $entityname): self {
-        if ($entityname === '' || $entityname !== clean_param($entityname, PARAM_ALPHANUMEXT)) {
-            throw new coding_exception('Entity name must be comprised of alphanumeric character, underscore or dash');
-        }
-
         $this->entityname = $entityname;
         return $this;
     }
@@ -115,7 +110,7 @@ abstract class base {
      * @return string
      */
     final public function get_entity_name(): string {
-        return $this->entityname ?: $this->get_default_entity_name();
+        return $this->entityname ?? $this->get_default_entity_name();
     }
 
     /**

reportbuilder/classes/local/entities/course.php

@@ -63,15 +63,6 @@ class course extends base {
         ];
     }
 
-    /**
-     * The default machine-readable name for this entity that will be used in the internal names of the columns/filters.
-     *
-     * @return string
-     */
-    protected function get_default_entity_name(): string {
-        return 'course';
-    }
-
     /**
      * The default title for this entity in the list of columns/filters in the report builder.
      *

reportbuilder/classes/local/report/base.php

@@ -292,10 +292,14 @@ abstract class base {
      * @throws coding_exception
      */
     final protected function annotate_entity(string $name, lang_string $title): void {
-        if (empty($name) || $name !== clean_param($name, PARAM_ALPHANUMEXT)) {
+        if ($name === '' || $name !== clean_param($name, PARAM_ALPHANUMEXT)) {
             throw new coding_exception('Entity name must be comprised of alphanumeric character, underscore or dash');
         }
 
+        if (array_key_exists($name, $this->entitytitles)) {
+            throw new coding_exception('Duplicate entity name', $name);
+        }
+
         $this->entitytitles[$name] = $title;
     }
 

reportbuilder/tests/local/entities/base_test.php

@@ -126,17 +126,6 @@ class base_test extends advanced_testcase {
         $this->assertEquals('newentityname', $entity->get_entity_name());
     }
 
-    /**
-     * Test invalid entity name
-     */
-    public function test_set_entity_name_invalid(): void {
-        $entity = new base_test_entity();
-
-        $this->expectException(coding_exception::class);
-        $this->expectExceptionMessage('Entity name must be comprised of alphanumeric character, underscore or dash');
-        $entity->set_entity_name('');
-    }
-
     /**
      * Test entity title
      */

reportbuilder/tests/local/report/base_test.php

@@ -19,9 +19,12 @@ declare(strict_types=1);
 namespace core_reportbuilder\local\report;
 
 use advanced_testcase;
+use coding_exception;
 use context_system;
 use core_reportbuilder\system_report_available;
 use core_reportbuilder\system_report_factory;
+use lang_string;
+use ReflectionClass;
 
 /**
  * Unit tests for report base class
@@ -111,6 +114,56 @@ class base_test extends advanced_testcase {
         $this->assertEquals($contextcourse, $systemreport2->get_context());
     }
 
+    /**
+     * Test entity annotation
+     */
+    public function test_annotate_entity(): void {
+        $this->resetAfterTest();
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+
+        $method = (new ReflectionClass($systemreport))->getMethod('annotate_entity');
+        $method->setAccessible(true);
+
+        $method->invoke($systemreport, 'test', new lang_string('yes'));
+        $this->assertEquals(new lang_string('yes'), $systemreport->get_entity_title('test'));
+    }
+
+    /**
+     * Test entity annotation for invalid entity name
+     */
+    public function test_annotate_entity_invalid(): void {
+        $this->resetAfterTest();
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+
+        $method = (new ReflectionClass($systemreport))->getMethod('annotate_entity');
+        $method->setAccessible(true);
+
+        $this->expectException(coding_exception::class);
+        $this->expectExceptionMessage('Entity name must be comprised of alphanumeric character, underscore or dash');
+        $method->invoke($systemreport, '', new lang_string('yes'));
+    }
+
+    /**
+     * Test entity annotation for duplicated entity name
+     */
+    public function test_annotate_entity_duplicate(): void {
+        $this->resetAfterTest();
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+
+        $method = (new ReflectionClass($systemreport))->getMethod('annotate_entity');
+        $method->setAccessible(true);
+
+        $method->invoke($systemreport, 'test', new lang_string('yes'));
+
+        // Adding a second time with the same name should trigger exception.
+        $this->expectException(coding_exception::class);
+        $this->expectExceptionMessage('Duplicate entity name (test)');
+        $method->invoke($systemreport, 'test', new lang_string('no'));
+    }
+
     /**
      * Test for get_column
      */

reportbuilder/upgrade.txt

@@ -1,6 +1,11 @@
 This file describes API changes in /reportbuilder/*
 Information provided here is intended especially for developers.
 
+=== 4.3 ===
+
+* Trying to add/annotate duplicate entity names to a report will now throw a coding exception
+* The `get_default_entity_name` method of the base entity class is now private, and shouldn't be overridden in extending classes
+
 === 4.2 ===
 
 * New method `set_checkbox_toggleall` in system report class to allow reports to easily create checkbox toggle columns