From 511bb9eae9d8ccd420c050ff324848721586e975 Mon Sep 17 00:00:00 2001 From: Deltik Date: Fri, 9 Feb 2018 04:37:48 -0600 Subject: [PATCH 1/3] Implemented custom sorting for e_tree_model In #3025, e_tree_model sorting was reimplemented in pure PHP, but this broke custom sorting (like `?field=cat_name&asc=desc`). This commit introduces a hack that simulates a subset of MySQL/MariaDB ORDER BY clauses, which should be sufficient for all known custom sorting that can be requested. Tree formatting is always preserved, but custom sorting will apply for all items at a certain depth under the same parent. This commit also contains some minor formatting fixes and makes a minor change to some regex to make use of non-capturing groups. Fixes: #3029 --- e107_handlers/admin_ui.php | 4 +- e107_handlers/model_class.php | 91 ++++++++++++++++++++++++++++++----- 2 files changed, 80 insertions(+), 15 deletions(-) mode change 100644 => 100755 e107_handlers/admin_ui.php mode change 100644 => 100755 e107_handlers/model_class.php diff --git a/e107_handlers/admin_ui.php b/e107_handlers/admin_ui.php old mode 100644 new mode 100755 index bc1e2965f..68b36158f --- a/e107_handlers/admin_ui.php +++ b/e107_handlers/admin_ui.php @@ -6188,7 +6188,7 @@ class e_admin_ui extends e_admin_controller_ui $this->_tree_model = new e_admin_tree_model(); $this->_tree_model->setModelTable($this->table) ->setFieldIdName($this->pid) - ->setUrl($this->url) + ->setUrl($this->url) ->setMessageStackName('admin_ui_tree_'.$this->table) ->setParams(array('model_class' => 'e_admin_model', 'model_message_stack' => 'admin_ui_model_'.$this->table , @@ -6916,7 +6916,7 @@ class e_admin_form_ui extends e_form - // FIXME - use e_form::batchoptions(), nice way of buildig batch dropdown - news administration show_batch_options() + // FIXME - use e_form::batchoptions(), nice way of building batch dropdown - news administration show_batch_options() /** * @param array $options array of flags for copy, delete, url, featurebox, batch diff --git a/e107_handlers/model_class.php b/e107_handlers/model_class.php old mode 100644 new mode 100755 index 35601befb..f0ab6f502 --- a/e107_handlers/model_class.php +++ b/e107_handlers/model_class.php @@ -3339,13 +3339,16 @@ class e_tree_model extends e_front_model // Workaround: Parse and modify db_query param for simulated pagination $this->prepareSimulatedPagination(); + // Workaround: Parse and modify db_query param for simulated custom ordering + $this->prepareSimulatedCustomOrdering(); if($sql->gen($this->getParam('db_query'), $this->getParam('db_debug') ? true : false)) { $rows = self::flatTreeFromArray($sql->rows(), $this->getParam('primary_field'), $this->getParam('sort_parent'), - $this->getParam('sort_field') + $this->getParam('sort_field'), + $this->getParam('sort_order') ); // Simulated pagination @@ -3389,13 +3392,15 @@ class e_tree_model extends e_front_model * @param array $rows Relational array with a parent field and a sort order field * @param string $primary_field The field name of the primary key (matches children to parents) * @param string $sort_parent The field name whose value is the parent ID - * @param string $sort_field The field name whose value is the sort order in the current tree node + * @param mixed $sort_field The field name (string) or field names (array) whose value + * is or values are the sort order in the current tree node + * @param int $sort_order Desired sorting direction: 1 if ascending, -1 if descending * @return array Input array sorted depth-first as if it were a tree */ - private static function flatTreeFromArray($rows, $primary_field, $sort_parent, $sort_field) + private static function flatTreeFromArray($rows, $primary_field, $sort_parent, $sort_field, $sort_order) { $rows_tree = self::arrayToTree($rows, $primary_field, $sort_parent); - return self::flattenTree($rows_tree, $sort_field); + return self::flattenTree($rows_tree, $sort_field, $sort_order); } /** @@ -3448,11 +3453,13 @@ class e_tree_model extends e_front_model /** * Flattens a tree into a depth-first array, sorting each node by a field's values * @param array $tree Tree with child nodes under the "_children" key - * @param string $sort_field The field name whose value is the sort order in the current tree node + * @param mixed $sort_field The field name (string) or field names (array) whose value + * is or values are the sort order in the current tree node + * @param int $sort_order Desired sorting direction: 1 if ascending, -1 if descending * @param int $depth The depth that this level of recursion is entering * @return array One-dimensional array in depth-first order with depth indicated by the "_depth" key */ - private static function flattenTree($tree, $sort_field = null, $depth = 0) + private static function flattenTree($tree, $sort_field = null, $sort_order = 1, $depth = 0) { $flat = array(); @@ -3465,18 +3472,40 @@ class e_tree_model extends e_front_model $flat[] = $item; if(is_array($children)) { - uasort($children, function($node1, $node2) + uasort($children, function($node1, $node2) use ($sort_field, $sort_order) { - if(intval($node1[$sort_field]) === intval($node2[$sort_field])) return 0; - return intval($node1[$sort_field]) < intval($node2[$sort_field]) ? -1 : 1; + return self::multiFieldCmp($node1, $node2, $sort_field, $sort_order); }); - $flat = array_merge($flat, self::flattenTree($children, $sort_field, $depth+1)); + $flat = array_merge($flat, self::flattenTree($children, $sort_field, $sort_order, $depth+1)); } } return $flat; } + /** + * Naturally compares two associative arrays given multiple sort keys and a reverse order flag + * @param array $row1 Associative array to compare to $row2 + * @param array $row2 Associative array to compare to $row1 + * @param mixed $sort_field Key (string) or keys (array) to compare + the values of in both $row1 and $row2 + * @param int $sort_order -1 to reverse the sorting order or 1 to keep the order as ascending + * @return int -1 if $row1 is less than $row2 + * 0 if $row1 is equal to $row2 + * 1 if $row1 is greater than $row2 + */ + private static function multiFieldCmp($row1, $row2, $sort_field, $sort_order = 1) + { + if (is_array($sort_field)) + $field = array_shift($sort_field); + $cmp = strnatcmp((string) $row1[$field], (string) $row2[$field]); + if ($sort_order === -1 || $sort_order === 1) $cmp *= $sort_order; + if ($cmp === 0 && count($sort_field) >= 1) + return self::multiFieldCmp($row1, $row2, $sort_field, $sort_order); + return $cmp; + + } + /** * Resiliently counts the results from the last SQL query in the given resource * @@ -3516,13 +3545,13 @@ class e_tree_model extends e_front_model private function prepareSimulatedPagination() { $db_query = $this->getParam('db_query'); - $db_query = preg_replace_callback("/LIMIT ([\d]+)[ ]*(,|OFFSET){0,1}[ ]*([\d]*)/", function($matches) + $db_query = preg_replace_callback("/LIMIT ([\d]+)[ ]*(?:,|OFFSET){0,1}[ ]*([\d]*)/i", function($matches) { // Offset and count - if (isset($matches[3])) + if (isset($matches[2])) { $this->setParam('db_limit_offset', $matches[1]); - $this->setParam('db_limit_count', $matches[3]); + $this->setParam('db_limit_count', $matches[2]); } // Count only else @@ -3535,6 +3564,42 @@ class e_tree_model extends e_front_model $this->setParam('db_query', $db_query); } + /** + * Workaround: Parse and modify query to prepare for simulation of custom ordering + * + * XXX: Not compliant with all forms of ORDER BY clauses + * XXX: Does not support quoted identifiers (`identifier`) + * XXX: Does not support mixed sort orders (identifier1 ASC, identifier2 DESC) + * + * This is a hack to enable custom ordering of tree models when + * flattening the tree. + * + * Implemented out of necessity under + * https://github.com/e107inc/e107/issues/3029 + * + * @returns null + */ + private function prepareSimulatedCustomOrdering() + { + $db_query = $this->getParam('db_query'); + $db_query = preg_replace_callback('/ORDER BY (?:.+\.)*[\.]*([A-Za-z0-9$_,]+)[ ]*(ASC|DESC)*/i', function($matches) + { + if (isset($matches[1])) + { + $current_sort_field = $this->getParam('sort_field'); + if (!empty($current_sort_field)) + { + $matches[1] = $current_sort_field.",".$matches[1]; + } + $this->setParam('sort_field', array_map('trim', explode(',', $matches[1]))); + } + if (isset($matches[2])) + $this->setParam('sort_order', + (0 === strcasecmp($matches[2], 'DESC') ? -1 : 1) + ); + }, $db_query); + } + /** * Get single model instance from the collection * @param integer $node_id From 2167a69a5013cd460f47e770e440be3e8fd5dada Mon Sep 17 00:00:00 2001 From: Deltik Date: Fri, 9 Feb 2018 04:57:01 -0600 Subject: [PATCH 2/3] Record count no longer depends on subclass impl. Subclasses may forget to run code to do a total record count, which leads to output showing "Total Records: 0" on some pages with lists, like `/e107_admin/links.php`. This commit cuts out the record counting from the getList() method of any e_admin_form_ui subclasses and the base class so that subclasses do not have to reimplement record counting. The caveat with this implementation is that it violates the Law of Demeter, as evidenced by the new chained method call: $this->getController()->getTreeModel()->getTotal() Jumping through two objects to get a value is not ideal, but this is the code we have to work with at the moment. --- e107_handlers/admin_ui.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/e107_handlers/admin_ui.php b/e107_handlers/admin_ui.php index 68b36158f..d8e1305af 100755 --- a/e107_handlers/admin_ui.php +++ b/e107_handlers/admin_ui.php @@ -6484,9 +6484,6 @@ class e_admin_form_ui extends e_form // if going through confirm screen - no JS confirm $controller->setFieldAttr('options', 'noConfirm', $controller->deleteConfirmScreen); - $this->listTotal = $tree[$id]->getTotal(); - - $fields = $controller->getFields(); // checks dispatcher acess/perms for create/edit/delete access in list mode. @@ -7017,7 +7014,7 @@ class e_admin_form_ui extends e_form $text .= " -
".e107::getParser()->lanVars(LAN_UI_TOTAL_RECORDS,number_format($this->listTotal))."
+
".e107::getParser()->lanVars(LAN_UI_TOTAL_RECORDS,number_format($this->getController()->getTreeModel()->getTotal()))."
"; From b3a274010e6b85ff680351834f647e56fae62064 Mon Sep 17 00:00:00 2001 From: Deltik Date: Fri, 9 Feb 2018 05:34:09 -0600 Subject: [PATCH 3/3] Refactored e_tree_model for Code Climate --- e107_handlers/admin_ui.php | 6 +++--- e107_handlers/model_class.php | 30 +++++++----------------------- 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/e107_handlers/admin_ui.php b/e107_handlers/admin_ui.php index d8e1305af..e21df6156 100755 --- a/e107_handlers/admin_ui.php +++ b/e107_handlers/admin_ui.php @@ -6167,7 +6167,7 @@ class e_admin_ui extends e_admin_controller_ui $this->_model = new e_admin_model(); $this->_model->setModelTable($this->table) ->setFieldIdName($this->pid) - ->setUrl($this->url) + ->setUrl($this->url) ->setValidationRules($this->validationRules) ->setDbTypes($this->fieldTypes) ->setFieldInputTypes($this->fieldInputTypes) @@ -6188,10 +6188,10 @@ class e_admin_ui extends e_admin_controller_ui $this->_tree_model = new e_admin_tree_model(); $this->_tree_model->setModelTable($this->table) ->setFieldIdName($this->pid) - ->setUrl($this->url) + ->setUrl($this->url) ->setMessageStackName('admin_ui_tree_'.$this->table) ->setParams(array('model_class' => 'e_admin_model', - 'model_message_stack' => 'admin_ui_model_'.$this->table , + 'model_message_stack' => 'admin_ui_model_'.$this->table, 'db_query' => $this->listQry, // Information necessary for PHP-based tree sort 'sort_parent' => $this->getSortParent(), diff --git a/e107_handlers/model_class.php b/e107_handlers/model_class.php index f0ab6f502..ba46ad2ee 100755 --- a/e107_handlers/model_class.php +++ b/e107_handlers/model_class.php @@ -3344,12 +3344,12 @@ class e_tree_model extends e_front_model if($sql->gen($this->getParam('db_query'), $this->getParam('db_debug') ? true : false)) { - $rows = self::flatTreeFromArray($sql->rows(), - $this->getParam('primary_field'), - $this->getParam('sort_parent'), - $this->getParam('sort_field'), - $this->getParam('sort_order') - ); + $rows_tree = self::arrayToTree($sql->rows, + $this->getParam('primary_field'), + $this->getParam('sort_parent')); + $rows = self::flattenTree($rows_tree, + $this->getParam('sort_field'), + $this->getParam('sort_order')); // Simulated pagination $rows = array_splice($rows, @@ -3387,22 +3387,6 @@ class e_tree_model extends e_front_model return $this; } - /** - * Depth-first sort a relational array with a parent field and a sort order field - * @param array $rows Relational array with a parent field and a sort order field - * @param string $primary_field The field name of the primary key (matches children to parents) - * @param string $sort_parent The field name whose value is the parent ID - * @param mixed $sort_field The field name (string) or field names (array) whose value - * is or values are the sort order in the current tree node - * @param int $sort_order Desired sorting direction: 1 if ascending, -1 if descending - * @return array Input array sorted depth-first as if it were a tree - */ - private static function flatTreeFromArray($rows, $primary_field, $sort_parent, $sort_field, $sort_order) - { - $rows_tree = self::arrayToTree($rows, $primary_field, $sort_parent); - return self::flattenTree($rows_tree, $sort_field, $sort_order); - } - /** * Converts a relational array with a parent field and a sort order field to a tree * @param array $rows Relational array with a parent field and a sort order field @@ -3488,7 +3472,7 @@ class e_tree_model extends e_front_model * @param array $row1 Associative array to compare to $row2 * @param array $row2 Associative array to compare to $row1 * @param mixed $sort_field Key (string) or keys (array) to compare - the values of in both $row1 and $row2 + * the values of in both $row1 and $row2 * @param int $sort_order -1 to reverse the sorting order or 1 to keep the order as ascending * @return int -1 if $row1 is less than $row2 * 0 if $row1 is equal to $row2