diff --git a/config/services.yml b/config/services.yml index dbcdbb07..1421d77b 100644 --- a/config/services.yml +++ b/config/services.yml @@ -18,7 +18,6 @@ services: class: phpbb\boardrules\controller\admin_controller arguments: - '@config' - - '@service_container' - '@controller.helper' - '@language' - '@language.loader' @@ -44,9 +43,8 @@ services: - '%core.root_path%' - '%core.php_ext%' - phpbb.boardrules.entity: - class: phpbb\boardrules\entity\rule - shared: false # service MUST not be shared for this to work! + phpbb.boardrules.entity_factory: + class: phpbb\boardrules\entity\factory arguments: - '@dbal.conn' - '%phpbb.boardrules.tables.boardrules%' @@ -54,10 +52,12 @@ services: phpbb.boardrules.operator: class: phpbb\boardrules\operators\rule arguments: - - '@service_container' + - '@phpbb.boardrules.entity_factory' + - '@dbal.conn' - '@phpbb.boardrules.nestedset_rules' - '@phpbb.boardrules.ruleset_operator' - '@phpbb.boardrules.table_lock' + - '%phpbb.boardrules.tables.boardrules%' phpbb.boardrules.ruleset_operator: class: phpbb\boardrules\operators\ruleset diff --git a/controller/admin_controller.php b/controller/admin_controller.php index d2bb3faf..58f6ab94 100644 --- a/controller/admin_controller.php +++ b/controller/admin_controller.php @@ -10,8 +10,6 @@ namespace phpbb\boardrules\controller; -use Symfony\Component\DependencyInjection\ContainerInterface; - /** * Admin controller */ @@ -20,9 +18,6 @@ class admin_controller implements admin_interface /** @var \phpbb\config\config */ protected $config; - /** @var ContainerInterface */ - protected $container; - /** @var \phpbb\controller\helper */ protected $controller_helper; @@ -66,7 +61,6 @@ class admin_controller implements admin_interface * Constructor * * @param \phpbb\config\config $config Config object - * @param ContainerInterface $container Service container interface * @param \phpbb\controller\helper $controller_helper Controller helper object * @param \phpbb\language\language $lang Language object * @param \phpbb\language\language_file_loader $language_loader Language file loader @@ -81,10 +75,9 @@ class admin_controller implements admin_interface * @param string $php_ext phpEx * @access public */ - public function __construct(\phpbb\config\config $config, ContainerInterface $container, \phpbb\controller\helper $controller_helper, \phpbb\language\language $lang, \phpbb\language\language_file_loader $language_loader, \phpbb\log\log $log, \phpbb\notification\manager $notification_manager, \phpbb\request\request $request, \phpbb\boardrules\operators\rule $rule_operator, \phpbb\boardrules\operators\ruleset $ruleset_operator, \phpbb\template\template $template, \phpbb\user $user, $root_path, $php_ext) + public function __construct(\phpbb\config\config $config, \phpbb\controller\helper $controller_helper, \phpbb\language\language $lang, \phpbb\language\language_file_loader $language_loader, \phpbb\log\log $log, \phpbb\notification\manager $notification_manager, \phpbb\request\request $request, \phpbb\boardrules\operators\rule $rule_operator, \phpbb\boardrules\operators\ruleset $ruleset_operator, \phpbb\template\template $template, \phpbb\user $user, $root_path, $php_ext) { $this->config = $config; - $this->container = $container; $this->controller_helper = $controller_helper; $this->lang = $lang; $this->language_loader = $language_loader; @@ -234,7 +227,6 @@ public function display_language_dashboard() * @param int $parent_id Category to display rules from; default: 0 * @return void * @access public - * @throws \phpbb\boardrules\exception\base If stored rule data is invalid */ public function display_rules($language, $parent_id = 0) { @@ -250,8 +242,18 @@ public function display_rules($language, $parent_id = 0) trigger_error($this->lang->lang('ACP_BOARDRULES_INVALID_LANGUAGE') . adm_back_link($this->u_action), E_USER_WARNING); } - // Grab all the rules in the current user's language - $entities = $this->rule_operator->get_rules($language, $parent_id); + try + { + // Load both result sets before assigning template data, so a malformed + // stored rule cannot leave a partially rendered ACP page. + $entities = $this->rule_operator->get_rules($language, $parent_id); + $parent_entities = $this->rule_operator->get_rule_parents($language, $parent_id); + } + catch (\phpbb\boardrules\exception\base $e) + { + $this->display_rule_error($e); + return; + } // Initialize a variable to hold the right_id value $last_right_id = 0; @@ -282,11 +284,8 @@ public function display_rules($language, $parent_id = 0) $last_right_id = $entity->get_right_id(); } - // Prepare rule breadcrumb path navigation - $entities = $this->rule_operator->get_rule_parents($language, $parent_id); - // Process each rule entity for breadcrumb display - foreach ($entities as $entity) + foreach ($parent_entities as $entity) { // Set output block vars for display in the template $this->template->assign_block_vars('breadcrumb', array( @@ -514,7 +513,6 @@ protected function get_ruleset_return_url($language, $return_to) * @param int $parent_id Category to display rules from; default: 0 * @return void * @access public - * @throws \phpbb\boardrules\exception\base If stored rule data is invalid */ public function add_rule($language, $parent_id = 0) { @@ -523,7 +521,7 @@ public function add_rule($language, $parent_id = 0) // Initiate a rule entity /* @var $entity \phpbb\boardrules\entity\rule */ - $entity = $this->container->get('phpbb.boardrules.entity'); + $entity = $this->rule_operator->create_rule(); // Collect the form data $data = array( @@ -538,7 +536,15 @@ public function add_rule($language, $parent_id = 0) ); // Process the new rule - $this->add_edit_rule_data($entity, $data); + try + { + $this->add_edit_rule_data($entity, $data); + } + catch (\phpbb\boardrules\exception\base $e) + { + $this->display_rule_error($e); + return; + } // Set output vars for display in the template $this->template->assign_vars(array( @@ -555,14 +561,21 @@ public function add_rule($language, $parent_id = 0) * @param int $rule_id The rule identifier to edit * @return void * @access public - * @throws \phpbb\boardrules\exception\base If stored rule data is invalid */ public function edit_rule($rule_id) { // Add form key add_form_key('add_edit_rule'); - $entity = $this->load_rule($rule_id); + try + { + $entity = $this->load_rule($rule_id); + } + catch (\phpbb\boardrules\exception\base $e) + { + $this->display_rule_error($e); + return; + } // Collect the form data $data = array( @@ -577,7 +590,15 @@ public function edit_rule($rule_id) ); // Process the edited rule - $this->add_edit_rule_data($entity, $data); + try + { + $this->add_edit_rule_data($entity, $data); + } + catch (\phpbb\boardrules\exception\base $e) + { + $this->display_rule_error($e); + return; + } // Set output vars for display in the template $this->template->assign_vars(array( @@ -683,14 +704,7 @@ protected function add_edit_rule_data($entity, $data) if ($entity->get_id()) { // Save the edited rule entity to the database - try - { - $entity->save(); - } - catch (\phpbb\boardrules\exception\out_of_bounds $e) - { - trigger_error($e->get_message($this->lang) . adm_back_link($this->u_action), E_USER_WARNING); - } + $entity = $this->rule_operator->save_rule($entity); // Change rule parent if (isset($data['rule_parent_id']) && ($entity->get_parent_id() !== (int) $data['rule_parent_id'])) @@ -699,11 +713,7 @@ protected function add_edit_rule_data($entity, $data) { $this->rule_operator->change_parent($entity->get_id(), $data['rule_parent_id']); } - catch (\phpbb\boardrules\exception\out_of_bounds $e) - { - trigger_error($e->get_message($this->lang) . adm_back_link($this->u_action), E_USER_WARNING); - } - catch (\Exception $e) + catch (\InvalidArgumentException|\RuntimeException $e) { trigger_error($this->lang->lang($e->getMessage()) . adm_back_link($this->u_action), E_USER_WARNING); } @@ -719,10 +729,6 @@ protected function add_edit_rule_data($entity, $data) { $this->rule_operator->add_rule($entity, $data['rule_language'], $data['rule_parent_id']); } - catch (\phpbb\boardrules\exception\out_of_bounds $e) - { - trigger_error($e->get_message($this->lang) . adm_back_link($this->u_action), E_USER_WARNING); - } catch (\InvalidArgumentException|\RuntimeException $e) { trigger_error($this->lang->lang($e->getMessage()) . adm_back_link($this->u_action), E_USER_WARNING); @@ -779,7 +785,15 @@ protected function add_edit_rule_data($entity, $data) */ public function delete_rule($rule_id) { - $entity = $this->load_rule($rule_id); + try + { + $entity = $this->load_rule($rule_id); + } + catch (\phpbb\boardrules\exception\base $e) + { + $this->display_rule_error($e); + return; + } // Use a confirmation box routine when deleting a rule if (confirm_box(true)) @@ -789,13 +803,15 @@ public function delete_rule($rule_id) { $this->rule_operator->delete_rule($rule_id); } - catch (\phpbb\boardrules\exception\out_of_bounds $e) + catch (\phpbb\boardrules\exception\base $e) { - trigger_error($e->get_message($this->lang) . adm_back_link($this->u_action), E_USER_WARNING); + $this->display_rule_error($e); + return; } catch (\Exception $e) { trigger_error($this->lang->lang($e->getMessage()) . adm_back_link($this->u_action), E_USER_WARNING); + return; } // Show user confirmation of the deleted rule and provide link back to the previous page @@ -842,13 +858,15 @@ public function move_rule($rule_id, $direction, $amount = 1) { $moved = $this->rule_operator->move($rule_id, $direction, $amount); } - catch (\phpbb\boardrules\exception\out_of_bounds $e) + catch (\phpbb\boardrules\exception\base $e) { - trigger_error($e->get_message($this->lang) . adm_back_link($this->u_action), E_USER_WARNING); + $this->display_rule_error($e); + return; } catch (\Exception $e) { trigger_error($this->lang->lang($e->getMessage()) . adm_back_link($this->u_action), E_USER_WARNING); + return; } // Send a JSON response if an AJAX request was used @@ -858,7 +876,15 @@ public function move_rule($rule_id, $direction, $amount = 1) $json_response->send(array('success' => $moved)); } - $entity = $this->load_rule($rule_id); + try + { + $entity = $this->load_rule($rule_id); + } + catch (\phpbb\boardrules\exception\base $e) + { + $this->display_rule_error($e); + return; + } // Use a redirect to reload the current page redirect("{$this->u_action}&language={$entity->get_language()}&parent_id={$entity->get_parent_id()}"); @@ -915,21 +941,26 @@ public function set_page_url($u_action) } /** - * Load a rule or display a recoverable ACP error when it no longer exists. + * Load a rule. * * @param int $rule_id Rule identifier * @return \phpbb\boardrules\entity\rule_interface + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid */ protected function load_rule($rule_id) { - try - { - return $this->container->get('phpbb.boardrules.entity')->load($rule_id); - } - catch (\phpbb\boardrules\exception\out_of_bounds $e) - { - trigger_error($e->get_message($this->lang) . adm_back_link($this->u_action), E_USER_WARNING); - } + return $this->rule_operator->get_rule($rule_id); + } + + /** + * Display a translated entity or operator failure in the ACP. + * + * @param \phpbb\boardrules\exception\base $exception + * @return void + */ + protected function display_rule_error(\phpbb\boardrules\exception\base $exception) + { + trigger_error($exception->get_message($this->lang) . adm_back_link($this->u_action), E_USER_WARNING); } /** diff --git a/controller/admin_interface.php b/controller/admin_interface.php index 9071ec57..ecb4e5fc 100644 --- a/controller/admin_interface.php +++ b/controller/admin_interface.php @@ -13,7 +13,7 @@ /** * Interface for our admin controller * -* This describes all of the methods we'll use for the admin front-end of this extension +* This describes all the methods we'll use for the admin front-end of this extension */ interface admin_interface { @@ -59,7 +59,6 @@ public function set_ruleset_published($language, $published, $return_to = ''); * @param int $parent_id Category to display rules from; default: 0 * @return void * @access public - * @throws \phpbb\boardrules\exception\base If stored rule data is invalid */ public function display_rules($language, $parent_id = 0); @@ -78,7 +77,6 @@ public function save_ruleset_intro($language); * @param int $parent_id Category to display rules from; default: 0 * @return void * @access public - * @throws \phpbb\boardrules\exception\base If stored rule data is invalid */ public function add_rule($language, $parent_id = 0); @@ -88,7 +86,6 @@ public function add_rule($language, $parent_id = 0); * @param int $rule_id The rule identifier to edit * @return void * @access public - * @throws \phpbb\boardrules\exception\base If the rule does not exist or stored rule data is invalid */ public function edit_rule($rule_id); @@ -98,7 +95,6 @@ public function edit_rule($rule_id); * @param int $rule_id The rule identifier to delete * @return void * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds If the rule does not exist */ public function delete_rule($rule_id); @@ -110,7 +106,6 @@ public function delete_rule($rule_id); * @param int $amount The number of places to move the rule * @return void * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds If the rule does not exist after moving */ public function move_rule($rule_id, $direction, $amount = 1); diff --git a/controller/main_controller.php b/controller/main_controller.php index 17724a8b..5739205b 100644 --- a/controller/main_controller.php +++ b/controller/main_controller.php @@ -69,14 +69,14 @@ public function __construct(\phpbb\config\config $config, \phpbb\controller\help * * @return \Symfony\Component\HttpFoundation\Response A Symfony Response object * @access public - * @throws \phpbb\boardrules\exception\base If stored rule data is invalid + * @throws \phpbb\exception\http_exception If stored rule data is invalid */ public function display() { // When board rules are disabled, redirect users back to the forum index if (empty($this->config['boardrules_enable'])) { - redirect(append_sid("{$this->root_path}index.{$this->php_ext}")); + redirect(append_sid("{$this->root_path}index.$this->php_ext")); } // Add boardrules controller language file @@ -91,14 +91,21 @@ public function display() // Grab all published rules in the current user's language $used_language = $this->lang->get_used_language(); $display_language = $used_language; - $entities = $this->ruleset_operator->is_published($used_language) ? $this->rule_operator->get_rules($used_language) : array(); + try + { + $entities = $this->ruleset_operator->is_published($used_language) ? $this->rule_operator->get_rules($used_language) : array(); - // If no rules were found, it may be because no rules exist in the current user's - // language, so let's look for rules in the board's default language as a fallback. - if (empty($entities) && $used_language !== $this->config['default_lang'] && $this->ruleset_operator->is_published($this->config['default_lang'])) + // If no rules were found, it may be because no rules exist in the current user's + // language, so let's look for rules in the board's default language as a fallback. + if (empty($entities) && $used_language !== $this->config['default_lang'] && $this->ruleset_operator->is_published($this->config['default_lang'])) + { + $display_language = $this->config['default_lang']; + $entities = $this->rule_operator->get_rules($this->config['default_lang']); + } + } + catch (\phpbb\boardrules\exception\base $e) { - $display_language = $this->config['default_lang']; - $entities = $this->rule_operator->get_rules($this->config['default_lang']); + throw new \phpbb\exception\http_exception(500, 'GENERAL_ERROR', array(), $e); } /* @var $entity \phpbb\boardrules\entity\rule */ diff --git a/controller/main_interface.php b/controller/main_interface.php index 44c242ec..b8bb1cd1 100644 --- a/controller/main_interface.php +++ b/controller/main_interface.php @@ -22,7 +22,7 @@ interface main_interface * * @return \Symfony\Component\HttpFoundation\Response A Symfony Response object * @access public - * @throws \phpbb\boardrules\exception\base If stored rule data is invalid + * @throws \phpbb\exception\http_exception If stored rule data is invalid */ public function display(); } diff --git a/entity/factory.php b/entity/factory.php new file mode 100644 index 00000000..18e250ab --- /dev/null +++ b/entity/factory.php @@ -0,0 +1,42 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +*/ + +namespace phpbb\boardrules\entity; + +/** + * Factory for rule entities. + */ +class factory +{ + /** @var \phpbb\db\driver\driver_interface */ + protected $db; + + /** @var string */ + protected $boardrules_table; + + /** + * Constructor. + */ + public function __construct(\phpbb\db\driver\driver_interface $db, $boardrules_table) + { + $this->db = $db; + $this->boardrules_table = $boardrules_table; + } + + /** + * Create a fresh rule entity. + * + * @return rule_interface + */ + public function create() + { + return new rule($this->db, $this->boardrules_table); + } +} diff --git a/entity/rule.php b/entity/rule.php index 2979cea7..c993ec26 100644 --- a/entity/rule.php +++ b/entity/rule.php @@ -33,7 +33,14 @@ class rule implements rule_interface * rule_message_bbcode_options * @access protected */ - protected $data; + protected $data = array(); + + /** + * Storage-form data captured when this entity was hydrated. + * + * @var array + */ + protected $original_data = array(); /** @var \phpbb\db\driver\driver_interface */ protected $db; @@ -58,50 +65,21 @@ public function __construct(\phpbb\db\driver\driver_interface $db, $boardrules_t $this->boardrules_table = $boardrules_table; } - /** - * Load the data from the database for this rule - * - * @param int $id Rule identifier - * @return rule_interface $this object for chaining calls; load()->set()->save() - * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds - */ - public function load($id) - { - $sql = 'SELECT * - FROM ' . $this->boardrules_table . ' - WHERE rule_id = ' . (int) $id; - $result = $this->db->sql_query($sql); - $this->data = $this->db->sql_fetchrow($result); - $this->db->sql_freeresult($result); - - if ($this->data === false) - { - // A rule does not exist - throw new \phpbb\boardrules\exception\out_of_bounds('rule_id'); - } - - return $this; - } - /** * Import data for this rule * * Used when the data is already loaded externally. * Any existing data on this rule is over-written. - * Required fields and basic data types are validated. Values already loaded + * Required fields are checked and storage types are normalized. Values already loaded * from storage are not passed through write-time transformations again. * * @param array $data Data array, typically from the database - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\base */ public function import($data) { - // Clear out any saved data - $this->data = array(); - // All of our fields $fields = array( // column => data type (see settype()) @@ -121,7 +99,9 @@ public function import($data) 'rule_message_bbcode_options' => 'integer', ); - // Go through the basic fields and set them to our data array + $hydrated = array(); + + // Cast storage values without invoking write-time setters. foreach ($fields as $field => $type) { // If the data wasn't sent to us, throw an exception @@ -130,21 +110,10 @@ public function import($data) throw new \phpbb\boardrules\exception\invalid_argument(array($field, 'FIELD_MISSING')); } - // If the type is a method on this class, call it - if (method_exists($this, $type)) - { - $this->$type($data[$field]); - } - else - { - // settype passes values by reference - $value = $data[$field]; - - // We're using settype to enforce data types - settype($value, $type); - - $this->data[$field] = $value; - } + // settype passes values by reference + $value = $data[$field]; + settype($value, $type); + $hydrated[$field] = $value; } // Some fields must be unsigned (>= 0) @@ -158,85 +127,38 @@ public function import($data) foreach ($validate_unsigned as $field) { - // If the data is less than 0, it's not unsigned and we'll throw an exception - if ($this->data[$field] < 0) + // If the data is less than 0, it's not unsigned, and we'll throw an exception + if ($hydrated[$field] < 0) { throw new \phpbb\boardrules\exception\out_of_bounds($field); } } + // Replace state only after the entire row has passed hydration checks. + $this->data = $hydrated; + $this->original_data = $hydrated; + return $this; } /** - * Insert the rule for the first time - * - * Will throw an exception if the rule was already inserted (call save() instead) - * - * @param string $language The language iso - * @return rule_interface $this object for chaining calls; load()->set()->save() - * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds - */ - public function insert($language) + * Export current storage-form data. + * + * @return array + */ + public function get_data() { - if (!empty($this->data['rule_id'])) - { - // The rule already exists - throw new \phpbb\boardrules\exception\out_of_bounds('rule_id'); - } - - // Resets values required for the nested set system - $this->data['rule_parent_id'] = 0; - $this->data['rule_left_id'] = 0; - $this->data['rule_right_id'] = 0; - $this->data['rule_parents'] = ''; - - // Make extra sure there is no rule_id set - unset($this->data['rule_id']); - - // Add the language identifier to the data array - $this->data['rule_language'] = $language; - - // Insert the rule data to the database - $sql = 'INSERT INTO ' . $this->boardrules_table . ' ' . $this->db->sql_build_array('INSERT', $this->data); - $this->db->sql_query($sql); - - // Set the rule_id using the id created by the SQL insert - $this->data['rule_id'] = (int) $this->db->sql_last_inserted_id(); - - return $this; + return $this->data; } /** - * Save the current settings to the database - * - * This must be called before closing or any changes will not be saved! - * If adding a rule (saving for the first time), you must call insert() or an exception will be thrown - * - * @return rule_interface $this object for chaining calls; load()->set()->save() - * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds - */ - public function save() + * Export storage-form fields changed since hydration. + * + * @return array + */ + public function get_changes() { - if (empty($this->data['rule_id'])) - { - // The rule does not exist - throw new \phpbb\boardrules\exception\out_of_bounds('rule_id'); - } - - // Copy the data array, filtering out the rule_id identifier - // so we do not attempt to update the row's identity column. - $sql_array = array_diff_key($this->data, array('rule_id' => null)); - - // Update the page data in the database - $sql = 'UPDATE ' . $this->boardrules_table . ' - SET ' . $this->db->sql_build_array('UPDATE', $sql_array) . ' - WHERE rule_id = ' . $this->get_id(); - $this->db->sql_query($sql); - - return $this; + return array_diff_assoc($this->data, $this->original_data); } /** @@ -265,7 +187,7 @@ public function get_title() * Set title * * @param string $title - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\unexpected_value */ @@ -333,7 +255,7 @@ public function get_message_for_display($censor_text = true) * Set message * * @param string $message - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function set_message($message) @@ -365,7 +287,7 @@ public function message_bbcode_enabled() /** * Enable bbcode on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_enable_bbcode() @@ -378,7 +300,7 @@ public function message_enable_bbcode() /** * Disable bbcode on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_disable_bbcode() @@ -402,7 +324,7 @@ public function message_magic_url_enabled() /** * Enable magic url on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_enable_magic_url() @@ -415,7 +337,7 @@ public function message_enable_magic_url() /** * Disable magic url on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_disable_magic_url() @@ -439,7 +361,7 @@ public function message_smilies_enabled() /** * Enable smilies on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_enable_smilies() @@ -452,7 +374,7 @@ public function message_enable_smilies() /** * Disable smilies on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_disable_smilies() @@ -477,7 +399,7 @@ public function get_anchor() * Set anchor * * @param string $anchor Anchor text - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\unexpected_value */ @@ -549,7 +471,7 @@ public function get_language() * Set the language iso * * @param string $language language iso - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\unexpected_value */ @@ -640,7 +562,7 @@ protected function set_message_option($option_value, $negate = false, $reparse_m $this->data['rule_message_bbcode_options'] -= $option_value; } - // Re-parse the message + // Reparse the message if ($reparse_message && !empty($this->data['rule_message'])) { $message = $this->data['rule_message']; diff --git a/entity/rule_interface.php b/entity/rule_interface.php index d687bf69..5be0fe23 100644 --- a/entity/rule_interface.php +++ b/entity/rule_interface.php @@ -13,57 +13,38 @@ /** * Interface for a single rule * -* This describes all of the methods we'll have for a single rule +* This describes all the methods we'll have for a single rule */ interface rule_interface { - /** - * Load the data from the database for this rule - * - * @param int $id Rule identifier - * @return rule_interface $this object for chaining calls; load()->set()->save() - * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds - */ - public function load($id); - /** * Import data for this rule * * Used when the data is already loaded externally. * Any existing data on this rule is over-written. - * All data is validated and an exception is thrown if any data is invalid. + * Required fields are checked and storage types are normalized. Values already loaded + * from storage are not passed through write-time transformations again. * * @param array $data Data array, typically from the database - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\base */ public function import($data); /** - * Insert the rule for the first time - * - * Will throw an exception if the rule was already inserted (call save() instead) - * - * @param string $language The language iso - * @return rule_interface $this object for chaining calls; load()->set()->save() - * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds - */ - public function insert($language); + * Export current storage-form data. + * + * @return array + */ + public function get_data(); /** - * Save the current settings to the database - * - * This must be called before closing or any changes will not be saved! - * If adding a rule (saving for the first time), you must call insert() or an exception will be thrown - * - * @return rule_interface $this object for chaining calls; load()->set()->save() - * @access public - * @throws \phpbb\boardrules\exception\out_of_bounds - */ - public function save(); + * Export storage-form fields changed since hydration. + * + * @return array + */ + public function get_changes(); /** * Get id @@ -85,7 +66,7 @@ public function get_title(); * Set title * * @param string $title - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\unexpected_value */ @@ -112,7 +93,7 @@ public function get_message_for_display($censor_text = true); * Set message * * @param string $message - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function set_message($message); @@ -128,7 +109,7 @@ public function message_bbcode_enabled(); /** * Enable bbcode on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_enable_bbcode(); @@ -136,7 +117,7 @@ public function message_enable_bbcode(); /** * Disable bbcode on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_disable_bbcode(); @@ -152,7 +133,7 @@ public function message_magic_url_enabled(); /** * Enable magic url on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_enable_magic_url(); @@ -160,7 +141,7 @@ public function message_enable_magic_url(); /** * Disable magic url on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_disable_magic_url(); @@ -176,7 +157,7 @@ public function message_smilies_enabled(); /** * Enable smilies on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_enable_smilies(); @@ -184,7 +165,7 @@ public function message_enable_smilies(); /** * Disable smilies on the message * - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public */ public function message_disable_smilies(); @@ -201,7 +182,7 @@ public function get_anchor(); * Set anchor * * @param string $anchor Anchor text - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\unexpected_value */ @@ -219,7 +200,7 @@ public function get_language(); * Set the language iso * * @param string $language language iso - * @return rule_interface $this object for chaining calls; load()->set()->save() + * @return rule_interface $this object for chaining calls * @access public * @throws \phpbb\boardrules\exception\unexpected_value If the language is not installed */ diff --git a/exception/base.php b/exception/base.php index bd872423..a6d20361 100644 --- a/exception/base.php +++ b/exception/base.php @@ -135,19 +135,7 @@ protected function translate_portions(\phpbb\language\language $lang, $message_p */ public function add_lang(\phpbb\language\language $lang) { - static $is_loaded = false; - - // We only need to load the language file once - if ($is_loaded) - { - return; - } - - // Add our language file $lang->add_lang('exceptions', 'phpbb/boardrules'); - - // So the language file is only loaded once - $is_loaded = true; } /** diff --git a/operators/rule.php b/operators/rule.php index cda7482b..62b2d601 100644 --- a/operators/rule.php +++ b/operators/rule.php @@ -10,15 +10,19 @@ namespace phpbb\boardrules\operators; -use Symfony\Component\DependencyInjection\ContainerInterface; - /** * Operator for a set of rules */ class rule implements rule_interface { - /** @var ContainerInterface */ - protected $container; + /** @var \phpbb\boardrules\entity\factory */ + protected $entity_factory; + + /** @var \phpbb\db\driver\driver_interface */ + protected $db; + + /** @var string */ + protected $boardrules_table; /** * Nestedset for board rules @@ -36,18 +40,56 @@ class rule implements rule_interface /** * Constructor * - * @param ContainerInterface $container Service container interface + * @param \phpbb\boardrules\entity\factory $entity_factory Rule entity factory + * @param \phpbb\db\driver\driver_interface $db Database connection * @param \phpbb\boardrules\operators\nestedset_rules $nestedset_rules Nestedset object for tree functionality * @param \phpbb\boardrules\operators\ruleset_interface $ruleset_operator Ruleset operator object * @param \phpbb\lock\db $lock Shared Board Rules tree lock + * @param string $boardrules_table Board Rules table name * @access public */ - public function __construct(ContainerInterface $container, \phpbb\boardrules\operators\nestedset_rules $nestedset_rules, \phpbb\boardrules\operators\ruleset_interface $ruleset_operator, \phpbb\lock\db $lock) + public function __construct(\phpbb\boardrules\entity\factory $entity_factory, \phpbb\db\driver\driver_interface $db, \phpbb\boardrules\operators\nestedset_rules $nestedset_rules, \phpbb\boardrules\operators\ruleset_interface $ruleset_operator, \phpbb\lock\db $lock, $boardrules_table) { - $this->container = $container; + $this->entity_factory = $entity_factory; + $this->db = $db; $this->nestedset_rules = $nestedset_rules; $this->ruleset_operator = $ruleset_operator; $this->lock = $lock; + $this->boardrules_table = $boardrules_table; + } + + /** + * Create an empty rule entity. + * + * @return \phpbb\boardrules\entity\rule_interface + */ + public function create_rule() + { + return $this->entity_factory->create(); + } + + /** + * Get one rule by identifier. + * + * @param int $rule_id Rule identifier + * @return \phpbb\boardrules\entity\rule_interface + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid + */ + public function get_rule($rule_id) + { + $sql = 'SELECT * + FROM ' . $this->boardrules_table . ' + WHERE rule_id = ' . (int) $rule_id; + $result = $this->db->sql_query($sql); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + if ($row === false) + { + throw new \phpbb\boardrules\exception\out_of_bounds('rule_id'); + } + + return $this->create_rule()->import($row); } /** @@ -71,8 +113,7 @@ public function get_rules($language, $parent_id = 0) // Import each rule into an entity, and store them in an array foreach ($rowset as $row) { - $entities[] = $this->container->get('phpbb.boardrules.entity') - ->import($row); + $entities[] = $this->create_rule()->import($row); } // Return all rule entities @@ -89,10 +130,15 @@ public function get_rules($language, $parent_id = 0) * @access public * @throws \InvalidArgumentException If the language is not installed * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If the entity or stored data is invalid */ public function add_rule($entity, $language, $parent_id = 0) { + if ($entity->get_id()) + { + throw new \phpbb\boardrules\exception\out_of_bounds('rule_id'); + } + if (!$this->lock->acquire()) { throw new \RuntimeException('RULES_NESTEDSET_LOCK_FAILED_ACQUIRE'); @@ -108,11 +154,16 @@ public function add_rule($entity, $language, $parent_id = 0) // An empty ruleset must enter draft before its first rule is visible. $this->ruleset_operator->draft_if_empty($language); - // Insert the rule data to the database for the given language selection - $entity->insert($language); + $data = array_diff_key($entity->get_data(), array('rule_id' => null)); + $data['rule_parent_id'] = 0; + $data['rule_left_id'] = 0; + $data['rule_right_id'] = 0; + $data['rule_parents'] = ''; + $data['rule_language'] = (string) $language; - // Get the newly inserted rule's identifier - $rule_id = $entity->get_id(); + $sql = 'INSERT INTO ' . $this->boardrules_table . ' ' . $this->db->sql_build_array('INSERT', $data); + $this->db->sql_query($sql); + $rule_id = (int) $this->db->sql_last_inserted_id(); // Update the tree for the rule in the database $this->nestedset_rules @@ -125,8 +176,7 @@ public function add_rule($entity, $language, $parent_id = 0) $this->nestedset_rules->change_parent($rule_id, $parent_id); } - // Reload the data to return a fresh rule entity - return $entity->load($rule_id); + return $this->get_rule($rule_id); } finally { @@ -134,6 +184,33 @@ public function add_rule($entity, $language, $parent_id = 0) } } + /** + * Persist changes to an existing rule. + * + * @param \phpbb\boardrules\entity\rule_interface $entity Rule entity + * @return \phpbb\boardrules\entity\rule_interface Persisted rule entity + * @throws \phpbb\boardrules\exception\base If the entity is missing or persisted data is invalid + */ + public function save_rule($entity) + { + $rule_id = $entity->get_id(); + if (!$rule_id) + { + throw new \phpbb\boardrules\exception\out_of_bounds('rule_id'); + } + + $changes = array_diff_key($entity->get_changes(), array('rule_id' => null)); + if (!empty($changes)) + { + $sql = 'UPDATE ' . $this->boardrules_table . ' + SET ' . $this->db->sql_build_array('UPDATE', $changes) . ' + WHERE rule_id = ' . $rule_id; + $this->db->sql_query($sql); + } + + return $this->get_rule($rule_id); + } + /** * Delete a rule * @@ -141,7 +218,7 @@ public function add_rule($entity, $language, $parent_id = 0) * @return void * @access public * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid */ public function delete_rule($rule_id) { @@ -170,7 +247,7 @@ public function delete_rule($rule_id) * @return bool True if the rule moved, false if it was already at the boundary * @access public * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid */ public function move($rule_id, $direction = 'up', $amount = 1) { @@ -200,7 +277,7 @@ public function move($rule_id, $direction = 'up', $amount = 1) * @return void * @access public * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If either rule is missing or stored data is invalid */ public function change_parent($rule_id, $new_parent_id) { @@ -233,13 +310,11 @@ public function change_parent($rule_id, $new_parent_id) * * @param int $rule_id Rule identifier * @return string Language ISO code - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid */ protected function get_rule_language($rule_id) { - return $this->container->get('phpbb.boardrules.entity') - ->load($rule_id) - ->get_language(); + return $this->get_rule($rule_id)->get_language(); } /** @@ -263,8 +338,7 @@ public function get_rule_parents($language, $parent_id) // Import each rule into an entity, and store them in an array foreach ($rowset as $row) { - $entities[] = $this->container->get('phpbb.boardrules.entity') - ->import($row); + $entities[] = $this->create_rule()->import($row); } return $entities; } diff --git a/operators/rule_interface.php b/operators/rule_interface.php index ca99b540..c93d1ced 100644 --- a/operators/rule_interface.php +++ b/operators/rule_interface.php @@ -17,6 +17,22 @@ */ interface rule_interface { + /** + * Create an empty rule entity. + * + * @return \phpbb\boardrules\entity\rule_interface + */ + public function create_rule(); + + /** + * Get one rule by identifier. + * + * @param int $rule_id Rule identifier + * @return \phpbb\boardrules\entity\rule_interface + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid + */ + public function get_rule($rule_id); + /** * Get the rules * @@ -38,10 +54,19 @@ public function get_rules($language, $parent_id = 0); * @access public * @throws \InvalidArgumentException If the language is not installed * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds If the entity was already inserted + * @throws \phpbb\boardrules\exception\base If the entity or stored data is invalid */ public function add_rule($entity, $language, $parent_id = 0); + /** + * Persist changes to an existing rule. + * + * @param \phpbb\boardrules\entity\rule_interface $entity Rule entity + * @return \phpbb\boardrules\entity\rule_interface Persisted rule entity + * @throws \phpbb\boardrules\exception\base If the entity is missing or persisted data is invalid + */ + public function save_rule($entity); + /** * Delete a rule * @@ -49,7 +74,7 @@ public function add_rule($entity, $language, $parent_id = 0); * @return void * @access public * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid */ public function delete_rule($rule_id); @@ -62,7 +87,7 @@ public function delete_rule($rule_id); * @return bool True if the rule moved, false if it was already at the boundary * @access public * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If the rule is missing or stored data is invalid */ public function move($rule_id, $direction, $amount = 1); @@ -74,7 +99,7 @@ public function move($rule_id, $direction, $amount = 1); * @return void * @access public * @throws \RuntimeException If the nested-set lock cannot be acquired - * @throws \phpbb\boardrules\exception\out_of_bounds + * @throws \phpbb\boardrules\exception\base If either rule is missing or stored data is invalid */ public function change_parent($rule_id, $new_parent_id); diff --git a/tests/controller/admin_controller_test.php b/tests/controller/admin_controller_test.php index 7cf7ff48..b1059012 100644 --- a/tests/controller/admin_controller_test.php +++ b/tests/controller/admin_controller_test.php @@ -115,12 +115,7 @@ protected function setUp(): void $user->style['style_path'] = 'prosilver'; $user->ip = '127.0.0.1'; - $container = $this->createMock(\Symfony\Component\DependencyInjection\ContainerInterface::class); - $container->method('get') - ->with('phpbb.boardrules.entity') - ->willReturnCallback(function () { - return new \phpbb\boardrules\entity\rule($this->db, 'phpbb_boardrules'); - }); + $entity_factory = new \phpbb\boardrules\entity\factory($this->db, 'phpbb_boardrules'); $lock = new \phpbb\lock\db('boardrules.table_lock.boardrules_table', $this->config, $this->db); $nestedset = new \phpbb\boardrules\operators\nestedset_rules($this->db, $lock, 'phpbb_boardrules'); @@ -130,7 +125,14 @@ protected function setUp(): void 'phpbb_boardrules', 'phpbb_boardrules_rulesets' ); - $this->rule_operator = new \phpbb\boardrules\operators\rule($container, $nestedset, $this->ruleset_operator, $lock); + $this->rule_operator = new \phpbb\boardrules\operators\rule( + $entity_factory, + $this->db, + $nestedset, + $this->ruleset_operator, + $lock, + 'phpbb_boardrules' + ); $this->request = $this->createMock(\phpbb\request\request::class); $this->request->method('variable') @@ -165,7 +167,6 @@ protected function setUp(): void $this->controller = new admin_controller( $this->config, - $container, $helper, $language, $language_loader, @@ -327,6 +328,18 @@ public function test_display_rules_uses_real_tree_and_skips_nested_children(): v ), admin_test_state::$form_key_suffixes); } + public function test_display_rules_reports_hydration_failure(): void + { + $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) + ->disableOriginalConstructor() + ->getMock(); + $operator->method('get_rules')->willThrowException(new \phpbb\boardrules\exception\invalid_argument(array('rule_title', 'FIELD_MISSING'))); + $this->replace_controller_service('rule_operator', $operator); + $this->setExpectedTriggerError(E_USER_WARNING, 'EXCEPTION_INVALID_ARGUMENT'); + + $this->controller->display_rules('en'); + } + public function test_display_rules_reports_available_default_fallback(): void { $this->controller->display_rules('fr'); @@ -640,6 +653,18 @@ public function test_edit_rule_reports_missing_rule(): void $this->controller->edit_rule(999); } + public function test_edit_rule_reports_invalid_stored_rule(): void + { + $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) + ->disableOriginalConstructor() + ->getMock(); + $operator->method('get_rule')->willThrowException(new \phpbb\boardrules\exception\invalid_argument(array('rule_title', 'FIELD_MISSING'))); + $this->replace_controller_service('rule_operator', $operator); + $this->setExpectedTriggerError(E_USER_WARNING, 'EXCEPTION_INVALID_ARGUMENT'); + + $this->controller->edit_rule(2); + } + public function test_edit_rule_initial_form_accepts_unchanged_legacy_anchor(): void { $this->db->sql_query("UPDATE phpbb_boardrules @@ -686,22 +711,42 @@ public function test_edit_rule_submit_saves_changes(): void public function test_edit_rule_reports_stale_entity_save(): void { $entity = $this->mock_entity(2); - $entity->method('save')->willThrowException(new \phpbb\boardrules\exception\out_of_bounds('rule_id')); - $this->replace_controller_service('container', $this->entity_container($entity)); + $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) + ->disableOriginalConstructor() + ->getMock(); + $operator->method('get_rule')->willReturn($entity); + $operator->method('save_rule')->willThrowException(new \phpbb\boardrules\exception\out_of_bounds('rule_id')); + $this->replace_controller_service('rule_operator', $operator); $this->post['submit'] = true; $this->setExpectedTriggerError(E_USER_WARNING, 'EXCEPTION_OUT_OF_BOUNDS'); $this->controller->edit_rule(2); } + public function test_edit_rule_reports_invalid_persisted_entity(): void + { + $entity = $this->mock_entity(2); + $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) + ->disableOriginalConstructor() + ->getMock(); + $operator->method('get_rule')->willReturn($entity); + $operator->method('save_rule')->willThrowException(new \phpbb\boardrules\exception\invalid_argument(array('rule_title', 'FIELD_MISSING'))); + $this->replace_controller_service('rule_operator', $operator); + $this->post['submit'] = true; + $this->setExpectedTriggerError(E_USER_WARNING, 'EXCEPTION_INVALID_ARGUMENT'); + + $this->controller->edit_rule(2); + } + public function test_edit_rule_reports_parent_change_failure(): void { $entity = $this->mock_entity(2); $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) ->disableOriginalConstructor() ->getMock(); + $operator->method('get_rule')->willReturn($entity); + $operator->method('save_rule')->willReturn($entity); $operator->method('change_parent')->willThrowException(new \RuntimeException('PARENT_CHANGE_FAILED')); - $this->replace_controller_service('container', $this->entity_container($entity)); $this->replace_controller_service('rule_operator', $operator); $this->post['submit'] = true; $this->variables['rule_parent'] = 3; @@ -716,8 +761,9 @@ public function test_edit_rule_translates_parent_change_bounds_failure(): void $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) ->disableOriginalConstructor() ->getMock(); + $operator->method('get_rule')->willReturn($entity); + $operator->method('save_rule')->willReturn($entity); $operator->method('change_parent')->willThrowException(new \phpbb\boardrules\exception\out_of_bounds('new_parent_id')); - $this->replace_controller_service('container', $this->entity_container($entity)); $this->replace_controller_service('rule_operator', $operator); $this->post['submit'] = true; $this->variables['rule_parent'] = 3; @@ -735,8 +781,8 @@ public function test_add_rule_reports_operator_errors($exception, $expected): vo $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) ->disableOriginalConstructor() ->getMock(); + $operator->method('create_rule')->willReturn($entity); $operator->method('add_rule')->willThrowException($exception); - $this->replace_controller_service('container', $this->entity_container($entity)); $this->replace_controller_service('rule_operator', $operator); $this->post['submit'] = true; $this->variables['rule_title'] = 'Valid title'; @@ -749,6 +795,7 @@ public static function add_rule_operator_error_data(): array { return array( 'entity bounds failure' => array(new \phpbb\boardrules\exception\out_of_bounds('rule_id'), 'EXCEPTION_OUT_OF_BOUNDS'), + 'entity hydration failure' => array(new \phpbb\boardrules\exception\invalid_argument(array('rule_title', 'FIELD_MISSING')), 'EXCEPTION_INVALID_ARGUMENT'), 'nested-set lock failure' => array(new \RuntimeException('RULES_NESTEDSET_LOCK_FAILED_ACQUIRE'), 'RULES_NESTEDSET_LOCK_FAILED_ACQUIRE'), ); } @@ -817,15 +864,11 @@ public function test_move_rule_redirects_to_own_parent(): void public function test_move_rule_reports_rule_removed_after_move(): void { - $entity = $this->getMockBuilder(\phpbb\boardrules\entity\rule::class) - ->disableOriginalConstructor() - ->getMock(); - $entity->method('load')->willThrowException(new \phpbb\boardrules\exception\out_of_bounds('rule_id')); $operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) ->disableOriginalConstructor() ->getMock(); $operator->method('move')->willReturn(true); - $this->replace_controller_service('container', $this->entity_container($entity)); + $operator->method('get_rule')->willThrowException(new \phpbb\boardrules\exception\out_of_bounds('rule_id')); $this->replace_controller_service('rule_operator', $operator); $this->setExpectedTriggerError(E_USER_WARNING, 'EXCEPTION_OUT_OF_BOUNDS'); @@ -907,7 +950,7 @@ public function test_parent_menu_resets_indentation_after_completed_category(): 'rule_message_bbcode_options' => 0, ); $this->db->sql_query('INSERT INTO phpbb_boardrules ' . $this->db->sql_build_array('INSERT', $sql_ary)); - $entity = (new \phpbb\boardrules\entity\rule($this->db, 'phpbb_boardrules'))->load(2); + $entity = $this->rule_operator->get_rule(2); $this->invoke_protected('build_parent_select_menu', array($entity, 1)); @@ -920,7 +963,6 @@ protected function mock_entity($id) $entity = $this->getMockBuilder(\phpbb\boardrules\entity\rule::class) ->disableOriginalConstructor() ->getMock(); - $entity->method('load')->willReturnSelf(); $entity->method('get_id')->willReturn($id); $entity->method('get_language')->willReturn('en'); $entity->method('get_parent_id')->willReturn(0); @@ -936,13 +978,6 @@ protected function mock_entity($id) return $entity; } - protected function entity_container($entity) - { - $container = $this->createMock(\Symfony\Component\DependencyInjection\ContainerInterface::class); - $container->method('get')->with('phpbb.boardrules.entity')->willReturn($entity); - return $container; - } - protected function replace_controller_service($property, $value): void { $reflection = new \ReflectionProperty($this->controller, $property); diff --git a/tests/controller/main_controller_test.php b/tests/controller/main_controller_test.php index e9471e15..b61d1e2b 100644 --- a/tests/controller/main_controller_test.php +++ b/tests/controller/main_controller_test.php @@ -14,6 +14,58 @@ class main_controller_test extends \phpbb_test_case { + public function test_display_converts_invalid_stored_rule_to_http_error(): void + { + global $phpbb_root_path, $phpEx; + + $config = new \phpbb\config\config(array( + 'boardrules_enable' => 1, + 'boardrules_list_style' => '', + 'default_lang' => 'en', + )); + $lang = $this->getMockBuilder(\phpbb\language\language::class) + ->disableOriginalConstructor() + ->getMock(); + $lang->method('get_used_language')->willReturn('en'); + + $rule_operator = $this->getMockBuilder(\phpbb\boardrules\operators\rule::class) + ->disableOriginalConstructor() + ->getMock(); + $domain_exception = new \phpbb\boardrules\exception\invalid_argument(array('rule_title', 'FIELD_MISSING')); + $rule_operator->method('get_rules')->willThrowException($domain_exception); + + $ruleset_operator = $this->getMockBuilder(\phpbb\boardrules\operators\ruleset::class) + ->disableOriginalConstructor() + ->getMock(); + $ruleset_operator->method('is_published')->willReturn(true); + + $helper = $this->getMockBuilder(\phpbb\controller\helper::class) + ->disableOriginalConstructor() + ->getMock(); + $template = $this->createMock(\phpbb\template\template::class); + $controller = new \phpbb\boardrules\controller\main_controller( + $config, + $helper, + $lang, + $rule_operator, + $ruleset_operator, + $template, + $phpbb_root_path, + $phpEx + ); + + try + { + $controller->display(); + self::fail('Expected invalid stored rule to produce an HTTP error.'); + } + catch (\phpbb\exception\http_exception $e) + { + self::assertSame(500, $e->getStatusCode()); + self::assertSame($domain_exception, $e->getPrevious()); + } + } + /** * Test data for the test_display() function * diff --git a/tests/entity/rule_entity_anchor_test.php b/tests/entity/rule_entity_anchor_test.php index ab4617c8..1874e4a1 100644 --- a/tests/entity/rule_entity_anchor_test.php +++ b/tests/entity/rule_entity_anchor_test.php @@ -174,7 +174,7 @@ public function test_unique_anchor($id, $language, $anchor, $expected) // Load the rule from the db if it exists if (null !== $id) { - $entity->load($id); + $entity->import($this->get_import_data()[$id]); } // Set the anchor for the given language @@ -251,7 +251,7 @@ public function test_unique_anchor_fails($id, $language, $anchor) // Load the rule from the db if it exists if (null !== $id) { - $entity->load($id); + $entity->import($this->get_import_data()[$id]); } // Set the anchor for the given language diff --git a/tests/entity/rule_entity_import_test.php b/tests/entity/rule_entity_import_test.php index c40bf038..d9f31abd 100644 --- a/tests/entity/rule_entity_import_test.php +++ b/tests/entity/rule_entity_import_test.php @@ -79,6 +79,32 @@ public function test_import_accepts_existing_unicode_title() $entity->import($data); self::assertSame($data['rule_title'], $entity->get_title()); + self::assertSame($data['rule_title'], $entity->get_data()['rule_title']); + self::assertSame(array(), $entity->get_changes()); + + $entity->set_title('Changed title'); + self::assertSame(array('rule_title' => 'Changed title'), $entity->get_changes()); + } + + /** + * Failed hydration does not leave partially replaced entity state. + */ + public function test_import_is_atomic() + { + $data = $this->get_import_data()[1]; + $entity = $this->get_rule_entity()->import($data); + unset($data['rule_anchor']); + + try + { + $entity->import($data); + self::fail('Expected invalid_argument exception was not thrown.'); + } + catch (\phpbb\boardrules\exception\invalid_argument $e) + { + self::assertSame(1, $entity->get_id()); + self::assertSame('anchor1', $entity->get_anchor()); + } } /** @@ -117,11 +143,6 @@ public static function import_test_fail_data() 'rule_message_bbcode_options' => -1, )); -// // Too long (no longer tested inside in the import method) -// $data[] = array_merge($import_data[1], array( -// 'rule_anchor' => str_repeat('a', 256), -// )); - // Go through every field and unset it while submitting everything else foreach ($import_data[1] as $field => $value) { diff --git a/tests/entity/rule_entity_insert_test.php b/tests/entity/rule_entity_insert_test.php deleted file mode 100644 index d29a6fd9..00000000 --- a/tests/entity/rule_entity_insert_test.php +++ /dev/null @@ -1,71 +0,0 @@ - -* @license GNU General Public License, version 2 (GPL-2.0) -* -*/ - -namespace phpbb\boardrules\tests\entity; - -/** -* Tests related to insert on rule entity -*/ -class rule_entity_insert_test extends rule_entity_base -{ - /** - * Test inserting new rule data - */ - public function test_insert() - { - // This is needed to set up the s9e text formatter services - // This can lead to a test failure if PCRE is old. - $this->get_test_case_helpers()->set_s9e_services(); - - // Set a language variable - $language = 'en'; - - // Setup the entity class - $entity = $this->get_rule_entity(); - - // Insert a table row - $result = $entity - ->set_anchor('inserted_anchor') - ->set_title('inserted_title') - ->set_message('inserted_message') - ->insert($language); - - // Assert the returned value is what we expect - self::assertInstanceOf('\phpbb\boardrules\entity\rule', $result); - - // Assert that a rule_id of value 5 was created - self::assertEquals(5, $result->get_id()); - } - - /** - * Try inserting a rule that already exists into the database - * Entities with an existing rule_id will fail to insert - * - */ - public function test_insert_fails() - { - $this->expectException(\phpbb\boardrules\exception\out_of_bounds::class); - - // Set a language variable - $language = 'en'; - - // Load some import test data - $import_data = $this->get_import_data(); - - // Setup the entity class - $entity = $this->get_rule_entity(); - - // Import an existing rule entity - $entity->import($import_data[1]); - - // Try to insert the existing rule entity - $entity->insert($language); - } -} diff --git a/tests/entity/rule_entity_load_test.php b/tests/entity/rule_entity_load_test.php deleted file mode 100644 index 00d30e3f..00000000 --- a/tests/entity/rule_entity_load_test.php +++ /dev/null @@ -1,134 +0,0 @@ - -* @license GNU General Public License, version 2 (GPL-2.0) -* -*/ - -namespace phpbb\boardrules\tests\entity; - -/** -* Tests related to load on rule entity -*/ -class rule_entity_load_test extends rule_entity_base -{ - /** - * Test data for the test_load() function - * - * @return array Array of test data - */ - public static function load_test_data() - { - return array( - // id to search, data which should match - array( - 1, - array( - 'rule_id' => 1, - 'rule_language' => 'en', - 'rule_left_id' => 1, - 'rule_right_id' => 2, - 'rule_parent_id' => 0, - 'rule_anchor' => 'anchor_1', - 'rule_title' => 'title_1', - 'rule_message' => 'message_1', - ), - ), - array( - 2, - array( - 'rule_id' => 2, - 'rule_language' => 'en', - 'rule_left_id' => 3, - 'rule_right_id' => 4, - 'rule_parent_id' => 0, - 'rule_anchor' => 'anchor_2', - 'rule_title' => 'title_2', - 'rule_message' => 'message_2', - ), - ), - array( - 3, - array( - 'rule_id' => 3, - 'rule_language' => 'en', - 'rule_left_id' => 5, - 'rule_right_id' => 6, - 'rule_parent_id' => 0, - 'rule_anchor' => 'anchor_3', - 'rule_title' => 'title_3', - 'rule_message' => 'message_3', - ), - ), - ); - } - - /** - * Test loading rules from the database - * - * @dataProvider load_test_data - */ - public function test_load($id, $data) - { - // Setup the entity class - $entity = $this->get_rule_entity(); - - // Set the data - $result = $entity->load($id); - - // Assert the returned value is what we expect - self::assertInstanceOf('\phpbb\boardrules\entity\rule', $result); - - // Map the fields to the getters - $map = array( - 'rule_id' => 'get_id', - 'rule_language' => 'get_language', - 'rule_left_id' => 'get_left_id', - 'rule_right_id' => 'get_right_id', - 'rule_parent_id'=> 'get_parent_id', - 'rule_anchor' => 'get_anchor', - 'rule_title' => 'get_title', - 'rule_message' => 'get_message_for_edit', - ); - - // Go through each field in the data and make sure the function returns - // what we saved - foreach ($map as $field => $function) - { - self::assertEquals($data[$field], $entity->$function()); - } - } - - /** - * Test data for the test_load_fails() function - * - * @return array Array of test data - */ - public static function load_fails_test_data() - { - return array( - // id to search - array(0), - array(10), - ); - } - - /** - * Test loading (non-existant) rules from the database - * - * @dataProvider load_fails_test_data - */ - public function test_load_fails($id) - { - $this->expectException(\phpbb\boardrules\exception\out_of_bounds::class); - - // Setup the entity class - $entity = $this->get_rule_entity(); - - // Load the entity - $entity->load($id); - } -} diff --git a/tests/entity/rule_entity_save_test.php b/tests/entity/rule_entity_save_test.php deleted file mode 100644 index 595094f6..00000000 --- a/tests/entity/rule_entity_save_test.php +++ /dev/null @@ -1,117 +0,0 @@ - -* @license GNU General Public License, version 2 (GPL-2.0) -* -*/ - -namespace phpbb\boardrules\tests\entity; - -/** -* Tests related to save on rule entity -*/ -class rule_entity_save_test extends rule_entity_base -{ - /** - * Test data for the test_save() function - * - * @return array Array of test data - */ - public static function save_test_data() - { - return array( - array( - 1, - array( - 'rule_id' => 1, - 'rule_anchor' => 'new_anchor_1', - 'rule_title' => 'new_title_1', - ), - ), - array( - 2, - array( - 'rule_id' => 2, - 'rule_anchor' => 'new_anchor_2', - 'rule_title' => 'new_title_2', - ), - ), - ); - } - - /** - * Test saving data - * - * @dataProvider save_test_data - */ - public function test_save($id, $expected) - { - // Setup the entity class - $entity = $this->get_rule_entity(); - - // Load the data - $result = $entity->load($id); - - // Assert the returned value is what we expect - self::assertInstanceOf('\phpbb\boardrules\entity\rule', $result); - - // Set some new data - $entity - ->set_anchor($expected['rule_anchor']) - ->set_title($expected['rule_title']) - ->save(); - - // Re-load the data from the database - $result = $entity->load($id); - - // Assert expected matches actual - self::assertEquals($expected['rule_id'], $result->get_id()); - self::assertEquals($expected['rule_anchor'], $result->get_anchor()); - self::assertEquals($expected['rule_title'], $result->get_title()); - } - - public function test_four_byte_title_characters_are_encoded_and_other_unicode_is_preserved() - { - $entity = $this->get_rule_entity(); - $entity->load(1) - ->set_anchor('emoji-title') - ->set_title('Emoji 😀 中文 Кириллица title') - ->save(); - - $result = $this->db->sql_query('SELECT rule_anchor, rule_title - FROM phpbb_boardrules - WHERE rule_id = 1'); - $row = $this->db->sql_fetchrow($result); - $this->db->sql_freeresult($result); - - self::assertSame('emoji-title', $row['rule_anchor']); - self::assertSame( - strpos($this->db->get_sql_layer(), 'mssql') === 0 - ? 'Emoji 😀 中文 Кириллица title' - : 'Emoji 😀 中文 Кириллица title', - $row['rule_title'] - ); - - $entity->load(1); - self::assertSame('emoji-title', $entity->get_anchor()); - self::assertSame('Emoji 😀 中文 Кириллица title', $entity->get_title()); - } - - /** - * Test saving to (non-existant) rules from the database - * - */ - public function test_save_fails() - { - $this->expectException(\phpbb\boardrules\exception\out_of_bounds::class); - - // Setup the entity class - $entity = $this->get_rule_entity(); - - // Save the entity with no rule ID set - $entity->save(); - } -} diff --git a/tests/operators/rule_operator_add_rule_test.php b/tests/operators/rule_operator_add_rule_test.php index 41857d33..d8ebbf87 100644 --- a/tests/operators/rule_operator_add_rule_test.php +++ b/tests/operators/rule_operator_add_rule_test.php @@ -56,6 +56,20 @@ public function test_add_rule() self::assertSame('0', (string) $this->config['nestedset_rules_lock']); } + /** + * Test adding an existing rule fails before side effects. + */ + public function test_add_rule_rejects_existing_rule() + { + $entity = $this->get_rule_operator()->get_rule(1); + $this->ruleset_operator->expects(self::never())->method('draft_if_empty'); + + $this->expectException(\phpbb\boardrules\exception\out_of_bounds::class); + $this->expectExceptionMessage('rule_id'); + + $this->get_rule_operator()->add_rule($entity, 'en'); + } + /** * Test adding a rule rejects a parent from another language. */ @@ -78,7 +92,6 @@ public function test_add_rule_rejects_parent_from_another_language() $german_parent_id = (int) $this->db->sql_nextid(); $entity = $this->createMock(\phpbb\boardrules\entity\rule_interface::class); - $entity->expects(self::never())->method('insert'); $this->ruleset_operator->expects(self::never())->method('draft_if_empty'); try diff --git a/tests/operators/rule_operator_base.php b/tests/operators/rule_operator_base.php index ae25ab51..9448217d 100644 --- a/tests/operators/rule_operator_base.php +++ b/tests/operators/rule_operator_base.php @@ -28,8 +28,8 @@ protected static function setup_extensions() /** @var \phpbb\config\config */ protected $config; - /** @var \PHPUnit\Framework\MockObject\MockObject|\Symfony\Component\DependencyInjection\ContainerInterface */ - protected $container; + /** @var \phpbb\boardrules\entity\factory */ + protected $entity_factory; /** @var \phpbb\db\driver\driver_interface */ protected $db; @@ -55,17 +55,7 @@ protected function setUp(): void global $config; $this->db = $this->new_dbal(); - $db = $this->db; - - // mock container for the entity service - $this->container = $this->getMockBuilder('\Symfony\Component\DependencyInjection\ContainerInterface') - ->getMock(); - $this->container - ->method('get') - ->with('phpbb.boardrules.entity') - ->willReturnCallback(function () use ($db) { - return new \phpbb\boardrules\entity\rule($db, 'phpbb_boardrules'); - }); + $this->entity_factory = new \phpbb\boardrules\entity\factory($this->db, 'phpbb_boardrules'); $config = $this->config = new \phpbb\config\config(array('nestedset_rules_lock' => 0)); @@ -81,6 +71,13 @@ protected function setUp(): void */ protected function get_rule_operator() { - return new \phpbb\boardrules\operators\rule($this->container, $this->nestedset_rules, $this->ruleset_operator, $this->lock); + return new \phpbb\boardrules\operators\rule( + $this->entity_factory, + $this->db, + $this->nestedset_rules, + $this->ruleset_operator, + $this->lock, + 'phpbb_boardrules' + ); } } diff --git a/tests/operators/rule_operator_save_rule_test.php b/tests/operators/rule_operator_save_rule_test.php new file mode 100644 index 00000000..4879bbbc --- /dev/null +++ b/tests/operators/rule_operator_save_rule_test.php @@ -0,0 +1,76 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +*/ + +namespace phpbb\boardrules\tests\operators; + +class rule_operator_save_rule_test extends rule_operator_base +{ + public function test_get_and_save_rule() + { + $operator = $this->get_rule_operator(); + $entity = $operator->get_rule(1); + $entity->set_title('Changed title'); + + $saved = $operator->save_rule($entity); + + self::assertSame('Changed title', $saved->get_title()); + self::assertSame(array(), $saved->get_changes()); + self::assertSame('Changed title', $operator->get_rule(1)->get_title()); + } + + public function test_save_rule_rejects_new_entity() + { + $this->expectException(\phpbb\boardrules\exception\out_of_bounds::class); + $this->expectExceptionMessage('rule_id'); + + $this->get_rule_operator()->save_rule($this->entity_factory->create()); + } + + public function test_get_rule_rejects_unknown_id() + { + $this->expectException(\phpbb\boardrules\exception\out_of_bounds::class); + $this->expectExceptionMessage('rule_id'); + + $this->get_rule_operator()->get_rule(100); + } + + public function test_save_unicode_rule_title() + { + $operator = $this->get_rule_operator(); + $entity = $operator->get_rule(1); + $entity + ->set_anchor('emoji-title') + ->set_title('Emoji 😀 中文 Кириллица title'); + + $saved = $operator->save_rule($entity); + + $result = $this->db->sql_query('SELECT rule_anchor, rule_title + FROM phpbb_boardrules + WHERE rule_id = 1'); + $row = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + self::assertSame('emoji-title', $row['rule_anchor']); + self::assertSame( + strpos($this->db->get_sql_layer(), 'mssql') === 0 + ? 'Emoji 😀 中文 Кириллица title' + : 'Emoji 😀 中文 Кириллица title', + $row['rule_title'] + ); + self::assertSame('Emoji 😀 中文 Кириллица title', $saved->get_title()); + } + + public function test_create_rule_returns_fresh_entities() + { + $operator = $this->get_rule_operator(); + + self::assertNotSame($operator->create_rule(), $operator->create_rule()); + } +}