Ensure new rulesets start as draft and not immediately published

[iMattPro]

No description provided.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.72%. Comparing base (2841aed) to head (98fd516).

Files with missing lines	Patch %	Lines
controller/admin_controller.php	95.45%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #377      +/-   ##
============================================
- Coverage     99.80%   99.72%   -0.09%     
- Complexity      328      340      +12     
============================================
  Files            16       16              
  Lines          1045     1086      +41     
============================================
+ Hits           1043     1083      +40     
- Misses            2        3       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Updates Board Rules exception handling, initializes new rulesets as drafts, and expands related wiring and tests.

Changes:

• Standardizes Board Rules runtime exceptions.
• Adds draft-state initialization for rulesets.
• Updates dependencies, interfaces, and regression tests.

Open findings:

• exception/base.php passes constructor arguments in the wrong order, causing exception construction failures.
• controller/admin_controller.php does not normalize native runtime exceptions before narrowed catches at lines 699, 784, and 832.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Summary
tests/system/exception_test.php	Tests exception inheritance and string output.
tests/operators/ruleset_operator_test.php	Tests draft and exception behavior.
tests/operators/rule_operator_base.php	Adds the ruleset operator mock.
tests/operators/rule_operator_add_rule_test.php	Tests draft initialization during rule insertion.
tests/migrations/v310_upgrade_test.php	Tests legacy publication defaults.
tests/controller/admin_controller_test.php	Updates controller exception tests.
operators/ruleset.php	Adds draft initialization and runtime exceptions.
operators/ruleset_interface.php	Documents the updated ruleset API.
operators/rule.php	Integrates ruleset draft handling.
operators/rule_interface.php	Updates exception documentation.
exception/base.php	Updates the base exception hierarchy and constructor.
controller/admin_controller.php	Updates runtime exception handling.
config/services.yml	Wires the ruleset operator dependency.

Suppressed comments (2)

controller/admin_controller.php:784

• rule::delete_rule() still delegates to nestedset_rules->delete() without converting the inherited native \RuntimeException used for lock-acquisition failures into \phpbb\exception\runtime_exception. Since this handler was narrowed from \Exception, a delete during lock contention now escapes the ACP instead of producing the expected warning. Normalize the exception in the rule operator (or otherwise preserve handling for native runtime exceptions) before narrowing this catch.

			catch (\phpbb\exception\runtime_exception $e)

controller/admin_controller.php:832

• rule::move() still delegates to nestedset_rules->move() without converting the inherited native \RuntimeException used for lock-acquisition failures into \phpbb\exception\runtime_exception. With this handler narrowed from \Exception, a move during lock contention now bypasses the ACP warning path and bubbles out. Normalize the exception in the rule operator (or otherwise preserve handling for native runtime exceptions) before narrowing this catch.

		catch (\phpbb\exception\runtime_exception $e)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

operators/rule.php:104

• The new rule-operator test only verifies that a mock method is called, while the ruleset tests call draft_if_empty() directly. No test adds the first rule through a rule wired to the concrete ruleset operator and verifies that rules_published becomes 0; a regression in call ordering or service wiring would therefore pass. Add an integration/functional case for adding a first rule to an empty installed language.

			// An empty ruleset must enter draft before its first rule is visible.
			$this->ruleset_operator->draft_if_empty($language);

operators/rule.php:104

• This call unconditionally drafts the language, but the admin controller explicitly forbids the board's default language from being drafted (controller/admin_controller.php:458-461). If an administrator deletes the last default-language rule and adds a replacement, this sets rules_published to 0; the new rule then disappears from the public page because the fallback only applies to non-default languages. Skip the automatic draft transition for the default language or enforce that invariant in the ruleset operator.

			// An empty ruleset must enter draft before its first rule is visible.
			$this->ruleset_operator->draft_if_empty($language);

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (3)

operators/rule.php:104

• This call is unconditional, so when the default-language ruleset is empty (for example after its rules are deleted), adding its first rule writes rules_published = 0. The controller explicitly forbids drafting default_lang at controller/admin_controller.php:458-460, and the public controller does not load default rules while that state is false (controller/main_controller.php:99-106), so the newly added default rule disappears from the public page until a separate publish action. Either exempt the default language here or enforce the same default-language invariant in the ruleset operator.

			$this->ruleset_operator->draft_if_empty($language);

tests/migrations/v310_upgrade_test.php:30

• This does not exercise the migration: ruleset.xml already declares phpbb_boardrules_rulesets (lines 38-41), and this test only constructs the operator; it never runs m18_ruleset_status. As a result, it can verify the missing-row fallback but cannot detect a migration that fails to create the table or its columns. Use a pre-migration fixture and the migration test harness (or rename this as an operator compatibility test).

		// Models a 3.0.1 board after schema migration: existing rules have no
		// publication row because that table did not exist in the old release.
		return $this->createXMLDataSet(__DIR__ . '/../operators/fixtures/ruleset.xml');

tests/operators/rule_operator_add_rule_test.php:46

• This test only verifies that add_rule() calls a mock method; it never adds the first rule through the concrete ruleset operator and checks the persisted publication state. The existing functional create test adds to populated en, so the core regression could pass while the real ruleset service is miswired or fails to save rules_published = 0. Please add an integration/functional case for adding the first rule to an empty language.

		$this->ruleset_operator->expects(self::once())
			->method('draft_if_empty')
			->with($language);