From 985ec1af13cbe8b4bdafe63effbc2c22b6d607a2 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Mon, 2 Jun 2025 12:12:04 -0700 Subject: [PATCH 1/3] Create text reparser task dynamically test --- config/services.yml | 13 +-- .../cron_text_reparser_factory_test.php | 110 ++++++++++++++++++ textreparser/cron_text_reparser_factory.php | 90 ++++++++++++++ 3 files changed, 205 insertions(+), 8 deletions(-) create mode 100644 tests/text_reparser/cron_text_reparser_factory_test.php create mode 100644 textreparser/cron_text_reparser_factory.php diff --git a/config/services.yml b/config/services.yml index 4f12488..5dd73e6 100644 --- a/config/services.yml +++ b/config/services.yml @@ -81,16 +81,13 @@ services: tags: - { name: text_reparser.plugin } + phpbb.pages.cron.task.text_reparser.factory: + class: phpbb\pages\textreparser\cron_text_reparser_factory + phpbb.pages.cron.task.text_reparser: class: phpbb\cron\task\text_reparser\reparser + factory: ['@phpbb.pages.cron.task.text_reparser.factory', 'create'] arguments: - - '@config' - - '@config_text' - - '@text_reparser.lock' - - '@text_reparser.manager' - - '@text_reparser_collection' - calls: - - [set_name, [cron.task.text_reparser.phpbb_pages]] - - [set_reparser, [phpbb.pages.text_reparser.page_text]] + - '@service_container' tags: - { name: cron.task } diff --git a/tests/text_reparser/cron_text_reparser_factory_test.php b/tests/text_reparser/cron_text_reparser_factory_test.php new file mode 100644 index 0000000..12dafc4 --- /dev/null +++ b/tests/text_reparser/cron_text_reparser_factory_test.php @@ -0,0 +1,110 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +namespace phpbb\pages\tests\text_reparser; + +use phpbb\pages\textreparser\cron_text_reparser_factory; +use Symfony\Component\DependencyInjection\ContainerInterface; + +class cron_text_reparser_factory_test extends \phpbb_test_case +{ + /** @var \PHPUnit\Framework\MockObject\MockObject|ContainerInterface */ + protected $container; + + /** @var cron_text_reparser_factory */ + protected $factory; + + protected function setUp(): void + { + $this->container = $this->createMock(ContainerInterface::class); + $this->factory = new cron_text_reparser_factory(); + } + + public function test_create_reparser() + { + // Mock the necessary services + $config = $this->createMock(\phpbb\config\config::class); + $config_text = $this->createMock(\phpbb\config\db_text::class); + $reparse_lock = $this->createMock(\phpbb\lock\db::class); + $reparser_manager = $this->createMock(\phpbb\textreparser\manager::class); + $reparsers = $this->createMock(\phpbb\di\service_collection::class); + + // Configure a container to return our mocked services + $this->container->method('has') + ->willReturnCallback(function($service) { + $valid_services = [ + 'config' => true, + 'config_text' => true, + 'text_reparser.lock' => true, + 'text_reparser.manager' => true, + 'text_reparser_collection' => true, + ]; + return $valid_services[$service] ?? false; + }); + + $this->container->method('get') + ->willReturnCallback(function($service) use ($config, $config_text, $reparse_lock, $reparser_manager, $reparsers) { + $services = [ + 'config' => $config, + 'config_text' => $config_text, + 'text_reparser.lock' => $reparse_lock, + 'text_reparser.manager' => $reparser_manager, + 'text_reparser_collection' => $reparsers, + ]; + return $services[$service] ?? null; + }); + + $reparser = $this->factory->create($this->container); + + // Assert the reparser was created successfully + $this->assertInstanceOf(\phpbb\cron\task\text_reparser\reparser::class, $reparser); + $this->assertEquals('cron.task.text_reparser.phpbb_pages', $reparser->get_name()); + } + + public function test_missing_required_service() + { + // Mock container to simulate missing service + $this->container->method('has') + ->willReturn(false); + + $this->container->method('get') + ->willReturn(null); + + $this->expectException(\ArgumentCountError::class); + $this->expectExceptionMessage('Too few arguments to function phpbb\cron\task\text_reparser\reparser::__construct(), 0 passed and exactly 5 expected'); + + $this->factory->create($this->container); + } + + public function test_service_name_mapping() + { + $reflection = new \ReflectionClass(cron_text_reparser_factory::class); + $method = $reflection->getMethod('get_service_name'); + $method->setAccessible(true); + + $expected_mappings = [ + 'config' => 'config', + 'config_text' => 'config_text', + 'reparse_lock' => 'text_reparser.lock', + 'reparser_manager' => 'text_reparser.manager', + 'reparsers' => 'text_reparser_collection', + 'unknown_param' => 'unknown_param', + ]; + + foreach ($expected_mappings as $param => $expected) + { + $this->assertEquals( + $expected, + $method->invoke($this->factory, $param), + "Service mapping failed for parameter '$param'" + ); + } + } +} diff --git a/textreparser/cron_text_reparser_factory.php b/textreparser/cron_text_reparser_factory.php new file mode 100644 index 0000000..04f1663 --- /dev/null +++ b/textreparser/cron_text_reparser_factory.php @@ -0,0 +1,90 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +namespace phpbb\pages\textreparser; + +/** + * Factory for creating cron text reparser task instances + * + * This factory handles the dynamic creation of text reparser tasks, + * ensuring compatibility with different phpBB versions by using reflection + * to determine and provide the correct constructor arguments. + */ +class cron_text_reparser_factory +{ + /** + * Creates a new instance of the text reparser cron task + * + * @param \Symfony\Component\DependencyInjection\ContainerInterface $container Service container + * @return \phpbb\cron\task\text_reparser\reparser Configured a text reparser cron task + * @throws \ReflectionException If the reparser class cannot be reflected + */ + public function create($container) + { + $args = $this->get_constructor_arguments($container); + + // Using ReflectionClass to instantiate with a variable number of arguments + $reflection = new \ReflectionClass(\phpbb\cron\task\text_reparser\reparser::class); + $reparser = $reflection->newInstanceArgs($args); + + $reparser->set_name('cron.task.text_reparser.phpbb_pages'); + $reparser->set_reparser('phpbb.pages.text_reparser.page_text'); + return $reparser; + } + + /** + * Gets the constructor arguments for the reparser based on reflection + * + * @param \Symfony\Component\DependencyInjection\ContainerInterface $container Service container + * @return array Array of constructor arguments + * @throws \ReflectionException If the reparser class cannot be reflected + */ + private function get_constructor_arguments($container) + { + $reflection = new \ReflectionClass(\phpbb\cron\task\text_reparser\reparser::class); + $constructor = $reflection->getConstructor(); + $params = $constructor->getParameters(); + + $arguments = array(); + foreach ($params as $param) + { + $service_name = $this->get_service_name($param->getName()); + if ($container->has($service_name)) + { + $arguments[] = $container->get($service_name); + } + else if ($param->isOptional()) + { + $arguments[] = $param->getDefaultValue(); + } + } + + return $arguments; + } + + /** + * Maps parameter names to their corresponding service IDs + * + * @param string $param_name Name of the parameter from the constructor + * @return string Service ID corresponding to the parameter + */ + private function get_service_name($param_name) + { + $serviceMap = [ + 'config' => 'config', + 'config_text' => 'config_text', + 'reparse_lock' => 'text_reparser.lock', + 'reparser_manager' => 'text_reparser.manager', + 'reparsers' => 'text_reparser_collection' + ]; + + return $serviceMap[$param_name] ?? $param_name; + } +} From ae62ad2896d5624c385715e0daf49f13822e1c7e Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Mon, 2 Jun 2025 12:12:28 -0700 Subject: [PATCH 2/3] use trigger_event instead of dispatch --- controller/admin_controller.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/admin_controller.php b/controller/admin_controller.php index c9765b2..0676e26 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -336,7 +336,7 @@ protected function add_edit_page_data($entity) * @event phpbb.pages.acp_add_edit_page * @since 1.0.0-RC1 */ - $this->dispatcher->dispatch('phpbb.pages.acp_add_edit_page'); + $this->dispatcher->trigger_event('phpbb.pages.acp_add_edit_page'); // Set template vars for Page Template select menu $this->create_page_template_options($entity->get_template()); From c604e81a3b19d36e5925f7e45c522da6340930f5 Mon Sep 17 00:00:00 2001 From: Matt Friedman Date: Tue, 3 Jun 2025 09:55:29 -0700 Subject: [PATCH 3/3] Add a functional test for cron reparser --- tests/functional/cron_reparser_test.php | 92 +++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 tests/functional/cron_reparser_test.php diff --git a/tests/functional/cron_reparser_test.php b/tests/functional/cron_reparser_test.php new file mode 100644 index 0000000..6fcf553 --- /dev/null +++ b/tests/functional/cron_reparser_test.php @@ -0,0 +1,92 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + */ + +namespace functional; + +use phpbb\pages\tests\functional\pages_functional_base; + +/** + * @group functional + */ +class cron_reparser_test extends pages_functional_base +{ + /** + * Test the cron reparser functionality + */ + public function test_cron_reparser() + { + $this->login(); + $this->admin_login(); + + // Store some of our data in variables + $page_title = 'Cron Reparser Test Page'; + $page_content = '[b]This is a functional test page for the cron task reparser.[/b]'; + + // Create a test page + $route = $this->create_page($page_title, $page_content); + + // Go to the test page + $crawler = self::request('GET', "app.php/$route?sid=$this->sid"); + $this->assertStringContainsString($page_title, $crawler->filter('h2')->text()); + + // Assert no reparsers have run yet + $this->assertEmpty($this->get_reparser_resume()); + + // Run the cron task to reparse pages + self::request('GET', "app.php/cron/cron.task.text_reparser.phpbb_pages", [], false); + + // Try to ensure that the cron can actually run before we start to wait for it + sleep(1); + $cron_lock = new \phpbb\lock\db( + 'cron_lock', + new \phpbb\config\db( + $this->db, + new \phpbb\cache\driver\dummy(), + 'phpbb_config' + ), + $this->db + ); + + // Add timeout to prevent infinite loop + $timeout = time() + 30; // 30-second timeout + while (!$cron_lock->acquire()) + { + if (time() > $timeout) + { + $this->fail('Cron lock could not be acquired within 30 seconds'); + } + usleep(100000); // Sleep for 100 ms between attempts + } + $cron_lock->release(); + + // Assert there's now a record of reparsing pages in the database + $this->assertEquals( + ['phpbb.pages.text_reparser.page_text'], + array_keys(unserialize($this->get_reparser_resume(), ['allowed_classes' => false])) + ); + } + + /** + * Get the reparser resume data from the database + * + * @return string|null The config value or null if not found + */ + private function get_reparser_resume() + { + $sql = "SELECT config_value + FROM " . CONFIG_TEXT_TABLE . " + WHERE config_name = 'reparser_resume'"; + $result = $this->db->sql_query($sql); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + return $row['config_value'] ?? null; + } +}