diff --git a/reportbuilder/classes/local/entities/base.php b/reportbuilder/classes/local/entities/base.php index 6b14162378a..a32dfc02729 100644 --- a/reportbuilder/classes/local/entities/base.php +++ b/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(); } /** diff --git a/reportbuilder/classes/local/entities/course.php b/reportbuilder/classes/local/entities/course.php index c0824d702e7..5532f209f1f 100644 --- a/reportbuilder/classes/local/entities/course.php +++ b/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. * diff --git a/reportbuilder/classes/local/report/base.php b/reportbuilder/classes/local/report/base.php index c9c19922d40..8c4122573eb 100644 --- a/reportbuilder/classes/local/report/base.php +++ b/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; } diff --git a/reportbuilder/tests/local/entities/base_test.php b/reportbuilder/tests/local/entities/base_test.php index 603630162a6..a4110934997 100644 --- a/reportbuilder/tests/local/entities/base_test.php +++ b/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 */ diff --git a/reportbuilder/tests/local/report/base_test.php b/reportbuilder/tests/local/report/base_test.php index adc62f9e14d..5f04428a59f 100644 --- a/reportbuilder/tests/local/report/base_test.php +++ b/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 */ diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt index 6cd884281f6..dcf8e019dd0 100644 --- a/reportbuilder/upgrade.txt +++ b/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