From aba6cd07451add7c702c90eb88b2b7cd57a30efa Mon Sep 17 00:00:00 2001 From: Christian Schnegelberger Date: Mon, 6 Nov 2023 18:51:48 +0100 Subject: [PATCH 1/4] Check for empty in add_prefix() --- controller/admin_controller.php | 11 +++++++++-- language/en/acp_topic_prefixes.php | 1 + 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/controller/admin_controller.php b/controller/admin_controller.php index 2fc85f6..1f8130a 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -154,8 +154,15 @@ public function add_prefix() } $tag = $this->request->variable('prefix_tag', '', true); - $prefix = $this->manager->add_prefix($tag, $this->forum_id); - $this->log($prefix['prefix_tag'], 'ACP_LOG_PREFIX_ADDED'); + if ($tag != '') + { + $prefix = $this->manager->add_prefix($tag, $this->forum_id); + $this->log($prefix['prefix_tag'], 'ACP_LOG_PREFIX_ADDED'); + } + else + { + $this->trigger_message('TOPIC_PREFIXES_INPUT_EMPTY', E_USER_WARNING); + } } } diff --git a/language/en/acp_topic_prefixes.php b/language/en/acp_topic_prefixes.php index 524f26c..483c89a 100644 --- a/language/en/acp_topic_prefixes.php +++ b/language/en/acp_topic_prefixes.php @@ -40,4 +40,5 @@ 'TOPIC_PREFIXES_LOCK_FAILED_ACQUIRE' => 'Topic prefixes extension failed to acquire a lock on the table.', 'TOPIC_PREFIXES_INVALID_ITEM' => 'The requested topic prefix does not exist.', 'TOPIC_PREFIXES_INVALID_PARENT' => 'The requested topic prefix has no parent.', + 'TOPIC_PREFIXES_INPUT_EMPTY' => 'The input field for a new topic prefix must not be empty!', )); From a711f81a52f67536301fb34a7be70794ec35bceb Mon Sep 17 00:00:00 2001 From: Christian Schnegelberger Date: Tue, 7 Nov 2023 17:55:58 +0100 Subject: [PATCH 2/4] Trigger error when field empty, without else --- controller/admin_controller.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/controller/admin_controller.php b/controller/admin_controller.php index 1f8130a..a6df3dd 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -154,15 +154,14 @@ public function add_prefix() } $tag = $this->request->variable('prefix_tag', '', true); - if ($tag != '') - { - $prefix = $this->manager->add_prefix($tag, $this->forum_id); - $this->log($prefix['prefix_tag'], 'ACP_LOG_PREFIX_ADDED'); - } - else + + if ($tag === '') { $this->trigger_message('TOPIC_PREFIXES_INPUT_EMPTY', E_USER_WARNING); } + + $prefix = $this->manager->add_prefix($tag, $this->forum_id); + $this->log($prefix['prefix_tag'], 'ACP_LOG_PREFIX_ADDED'); } } From 11eaeafe1f8a6bfec7c5a61da5a7439fcf996ba0 Mon Sep 17 00:00:00 2001 From: Christian Schnegelberger Date: Sat, 9 Dec 2023 14:20:36 +0100 Subject: [PATCH 3/4] Add tests for empty field check --- tests/controller/add_fails_test.php | 68 +++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 tests/controller/add_fails_test.php diff --git a/tests/controller/add_fails_test.php b/tests/controller/add_fails_test.php new file mode 100644 index 0000000..2d51243 --- /dev/null +++ b/tests/controller/add_fails_test.php @@ -0,0 +1,68 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +namespace phpbb\topicprefixes\controller; + +/** + * We need to manually include the base file because auto loading won't work since the base + * uses the same namespace as the controller being tested (to override global scope functions). + */ +require_once __DIR__ . '/admin_controller_base.php'; + +class add_fails_test extends admin_controller_base +{ + /** + * Data for test_add_prefix + * + * @return array + */ + public function data_add_prefix() + { + return array( + array(true, true), + array(true, false), + array(false, false), + ); + } + + /** + * Test add_prefix() + * + * @dataProvider data_add_prefix + * @param $submit + * @param $valid_form + */ + + public function test_add_fails($input) + { + self::$valid_form = $valid_form; + + $this->request->expects(static::once()) + ->method('is_set_post') + ->willReturn(true); + $this->manager->expects(static::once()) + ->method('add_prefix') + ->willReturn(['prefix_tag' => $input]); + $this->log->expects($input ? static::once() : static::never()) + ->method('add'); + $this->db->expects($input ? static::once() : static::never()) + ->method('sql_fetchrow'); + + if ($input == '') + { + $exceptionName = version_compare(PHP_VERSION, '8.0', '<') ? \PHPUnit\Framework\Error\Error::class : \PHPUnit\Framework\Error\Warning::class; + $errno = version_compare(PHP_VERSION, '8.0', '<') ? E_USER_WARNING : E_WARNING; + $this->expectExceptionMessage('TOPIC_PREFIXES_INPUT_EMPTY'); + $this->expectException($exceptionName); + $this->expectExceptionCode($errno); + } + + $this->controller->add_prefix(); + } From 4e9b070d92849c78612ffcd22f721ea6b595a598 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Thu, 18 Jan 2024 08:38:12 -0800 Subject: [PATCH 4/4] Fix tests Signed-off-by: Matt Friedman --- controller/admin_controller.php | 2 +- prefixes/manager.php | 2 +- tests/controller/add_fails_test.php | 68 ---------------------------- tests/controller/add_prefix_test.php | 44 +++++++++++------- 4 files changed, 30 insertions(+), 86 deletions(-) delete mode 100644 tests/controller/add_fails_test.php diff --git a/controller/admin_controller.php b/controller/admin_controller.php index a6df3dd..5aa4578 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -159,7 +159,7 @@ public function add_prefix() { $this->trigger_message('TOPIC_PREFIXES_INPUT_EMPTY', E_USER_WARNING); } - + $prefix = $this->manager->add_prefix($tag, $this->forum_id); $this->log($prefix['prefix_tag'], 'ACP_LOG_PREFIX_ADDED'); } diff --git a/prefixes/manager.php b/prefixes/manager.php index 4b5902c..cb2b337 100644 --- a/prefixes/manager.php +++ b/prefixes/manager.php @@ -65,7 +65,7 @@ public function add_prefix($tag, $forum_id) { $data = [ 'prefix_tag' => $tag, - 'forum_id' => (int) $forum_id, + 'forum_id' => (int) $forum_id, 'prefix_enabled' => true, ]; diff --git a/tests/controller/add_fails_test.php b/tests/controller/add_fails_test.php deleted file mode 100644 index 2d51243..0000000 --- a/tests/controller/add_fails_test.php +++ /dev/null @@ -1,68 +0,0 @@ - - * @license GNU General Public License, version 2 (GPL-2.0) - * - */ - -namespace phpbb\topicprefixes\controller; - -/** - * We need to manually include the base file because auto loading won't work since the base - * uses the same namespace as the controller being tested (to override global scope functions). - */ -require_once __DIR__ . '/admin_controller_base.php'; - -class add_fails_test extends admin_controller_base -{ - /** - * Data for test_add_prefix - * - * @return array - */ - public function data_add_prefix() - { - return array( - array(true, true), - array(true, false), - array(false, false), - ); - } - - /** - * Test add_prefix() - * - * @dataProvider data_add_prefix - * @param $submit - * @param $valid_form - */ - - public function test_add_fails($input) - { - self::$valid_form = $valid_form; - - $this->request->expects(static::once()) - ->method('is_set_post') - ->willReturn(true); - $this->manager->expects(static::once()) - ->method('add_prefix') - ->willReturn(['prefix_tag' => $input]); - $this->log->expects($input ? static::once() : static::never()) - ->method('add'); - $this->db->expects($input ? static::once() : static::never()) - ->method('sql_fetchrow'); - - if ($input == '') - { - $exceptionName = version_compare(PHP_VERSION, '8.0', '<') ? \PHPUnit\Framework\Error\Error::class : \PHPUnit\Framework\Error\Warning::class; - $errno = version_compare(PHP_VERSION, '8.0', '<') ? E_USER_WARNING : E_WARNING; - $this->expectExceptionMessage('TOPIC_PREFIXES_INPUT_EMPTY'); - $this->expectException($exceptionName); - $this->expectExceptionCode($errno); - } - - $this->controller->add_prefix(); - } diff --git a/tests/controller/add_prefix_test.php b/tests/controller/add_prefix_test.php index 09978bd..dfcb33e 100644 --- a/tests/controller/add_prefix_test.php +++ b/tests/controller/add_prefix_test.php @@ -25,21 +25,25 @@ class add_prefix_test extends admin_controller_base */ public function data_add_prefix() { - return array( - array(true, true), - array(true, false), - array(false, false), - ); + return [ + ['topic_prefix1', true, true], // all should work fine + ['topic_prefix2', true, false], // form invalid error + ['topic_prefix3', false, false], // no action taken + ['', true, true], // prefix invalid error + ['', true, false], // form invalid error + ['', false, true], // no action taken + ]; } /** * Test add_prefix() * * @dataProvider data_add_prefix + * @param $prefix_id * @param $submit * @param $valid_form */ - public function test_add_prefix($submit, $valid_form) + public function test_add_prefix($prefix_id, $submit, $valid_form) { if ($submit) { @@ -51,21 +55,29 @@ public function test_add_prefix($submit, $valid_form) if (!$valid_form) { - // Throws E_WARNING in PHP 8.0+ and E_USER_WARNING in earlier versions - $exceptionName = version_compare(PHP_VERSION, '8.0', '<') ? \PHPUnit\Framework\Error\Error::class : \PHPUnit\Framework\Error\Warning::class; - $errno = version_compare(PHP_VERSION, '8.0', '<') ? E_USER_WARNING : E_WARNING; - $this->expectException($exceptionName); - $this->expectExceptionCode($errno); + $this->setExpectedTriggerError(E_USER_WARNING, $this->language->lang('FORM_INVALID')); } else { - $this->manager->expects(static::once()) + $valid_prefix = $prefix_id !== ''; + + $this->request->expects(static::once()) + ->method('variable') + ->willReturnMap([ + ['prefix_tag', '', true, \phpbb\request\request_interface::REQUEST, $prefix_id], + ]); + + if (!$valid_prefix) // Empty string prefixes should trigger error + { + $this->setExpectedTriggerError(E_USER_WARNING, $this->language->lang('TOPIC_PREFIXES_INPUT_EMPTY')); + } + $this->manager->expects($valid_prefix ? static::once() : static::never()) ->method('add_prefix') - ->willReturn(['prefix_tag' => 'topic_prefix']); - $this->log->expects(static::once()) + ->willReturn(['prefix_tag' => $prefix_id]); + $this->log->expects($valid_prefix ? static::once() : static::never()) ->method('add') - ->with('admin', static::anything(), static::anything(), 'ACP_LOG_PREFIX_ADDED', static::anything(), ['topic_prefix', 'Test Forum']); - $this->db->expects(static::once()) + ->with('admin', static::anything(), static::anything(), 'ACP_LOG_PREFIX_ADDED', static::anything(), [$prefix_id, 'Test Forum']); + $this->db->expects($valid_prefix ? static::once() : static::never()) ->method('sql_fetchrow') ->willReturn(['forum_name' => 'Test Forum']); }