Permalink
Browse files

Merge pull request #1020 from silverstripe-rebelalliance/open/7673

API Make DataList and ArrayList immutable
  • Loading branch information...
2 parents 644cc79 + bd59f84 commit 1303d82bc07535d5101ac25f00f59818c85ac566 @sminnee sminnee committed Dec 14, 2012
@@ -18,7 +18,7 @@
including `Email_BounceHandler` and `Email_BounceRecord` classes,
as well as the `Member->Bounced` property.
* Deprecated global email methods `htmlEmail()` and `plaintextEmail`, as well as various email helper methods like `encodeMultipart()`. Use the `Email` API, or the `Mailer` class where applicable.
- * Removed non-functional `$inlineImages` option for sending emails
+ * Removed non-functional `$inlineImages` option for sending emails
* Removed support for keyed arrays in `SelectionGroup`, use new `SelectionGroup_Item` object
to populate the list instead (see [API docs](api:SelectionGroup)).
* Removed `Form->Name()`: Use getName()
@@ -29,4 +29,20 @@
* Removed `Member->sendInfo()`: use Member_ChangePasswordEmail or Member_ForgotPasswordEmail directly
* Removed `SQLMap::map()`: Use DataList::("Member")->map()
* Removed `SQLMap::mapInGroups()`: Use Member::map_in_groups()
- * Removed `PasswordEncryptor::register()/unregister()`: Use config system instead
+ * Removed `PasswordEncryptor::register()/unregister()`: Use config system instead
+ * Methods on DataList and ArrayList that used to both modify the existing list & return a new version now just return a new version. Make sure you change statements like `$list->filter(...)` to $`list = $list->filter(...)` for these methods:
+ - `ArrayList#reverse`
+ - `ArrayList#sort`
+ - `ArrayList#filter`
+ - `ArrayList#exclude`
+ - `DataList#where`
+ - `DataList#limit`
+ - `DataList#sort`
+ - `DataList#addFilter`
+ - `DataList#applyFilterContext`
+ - `DataList#innerJoin`
+ - `DataList#leftJoin`
+ - `DataList#find`
+ - `DataList#byIDs`
+ - `DataList#reverse`
+ * `DataList#dataQuery` has been changed to return a clone of the query, and so can't be used to modify the list's query directly. Use `DataList#alterDataQuery` instead to modify dataQuery in a safe manner.
View
@@ -2,6 +2,17 @@
/**
* A list object that wraps around an array of objects or arrays.
*
+ * Note that (like DataLists), the implementations of the methods from SS_Filterable, SS_Sortable and
+ * SS_Limitable return a new instance of ArrayList, rather than modifying the existing instance.
+ *
+ * For easy reference, methods that operate in this way are:
+ *
+ * - limit
+ * - reverse
+ * - sort
+ * - filter
+ * - exclude
+ *
* @package framework
* @subpackage model
*/
@@ -119,7 +130,9 @@ public function toNestedArray() {
* @return ArrayList
*/
public function limit($length, $offset = 0) {
- return new ArrayList(array_slice($this->items, $offset, $length));
+ $list = clone $this;
+ $list->items = array_slice($this->items, $offset, $length);
+ return $list;
}
/**
@@ -309,8 +322,7 @@ public function canSortBy($by) {
* @return ArrayList
*/
public function reverse() {
- // TODO 3.1: This currently mutates existing array
- $list = /* clone */ $this;
+ $list = clone $this;
$list->items = array_reverse($this->items);
return $list;
@@ -376,8 +388,7 @@ public function sort() {
$multisortArgs[] = &$sortDirection[$column];
}
- // TODO 3.1: This currently mutates existing array
- $list = /* clone */ $this;
+ $list = clone $this;
// As the last argument we pass in a reference to the items that all the sorting will be applied upon
$multisortArgs[] = &$list->items;
call_user_func_array('array_multisort', $multisortArgs);
@@ -440,8 +451,7 @@ public function filter() {
}
}
- // TODO 3.1: This currently mutates existing array
- $list = /* clone */ $this;
+ $list = clone $this;
$list->items = $itemsToKeep;
return $list;
}
@@ -504,8 +514,7 @@ public function exclude() {
}
}
- // TODO 3.1: This currently mutates existing array
- $list = /* clone */ $this;
+ $list = clone $this;
$list->items = $itemsToKeep;
return $list;
}
View
@@ -3,21 +3,21 @@
* Implements a "lazy loading" DataObjectSet.
* Uses {@link DataQuery} to do the actual query generation.
*
- * todo 3.1: In 3.0 the below is not currently true for backwards compatible reasons, but code should not rely on
- * current behaviour.
+ * DataLists are _immutable_ as far as the query they represent is concerned. When you call a method that
+ * alters the query, a new DataList instance is returned, rather than modifying the existing instance
*
- * DataLists have two sets of methods.
+ * When you add or remove an element to the list the query remains the same, but because you have modified
+ * the underlying data the contents of the list changes. These are some of those methods:
*
- * 1). Selection methods (SS_Filterable, SS_Sortable, SS_Limitable) change the way the list is built, but does not
- * alter underlying data. There are no external affects from selection methods once this list instance is
- * destructed.
+ * - add
+ * - addMany
+ * - remove
+ * - removeMany
+ * - removeByID
+ * - removeByFilter
+ * - removeAll
*
- * 2). Mutation methods change the underlying data. The change persists into the underlying data storage layer.
- *
- * DataLists are _immutable_ as far as selection methods go - they all return new instances of DataList, rather
- * than change the current list.
- *
- * DataLists are _mutable_ as far as mutation methods go - they all act on the existing DataList instance.
+ * Subclasses of DataList may add other methods that have the same effect.
*
* @package framework
* @subpackage model
@@ -85,17 +85,13 @@ public function __clone() {
/**
* Return a copy of the internal {@link DataQuery} object
*
- * todo 3.1: In 3.0 the below is not currently true for backwards compatible reasons, but code should not rely on
- * this
- *
* Because the returned value is a copy, modifying it won't affect this list's contents. If
* you want to alter the data query directly, use the alterDataQuery method
*
* @return DataQuery
*/
public function dataQuery() {
- // TODO 3.1: This method potentially mutates self
- return /* clone */ $this->dataQuery;
+ return clone $this->dataQuery;
}
/**
@@ -122,7 +118,7 @@ public function alterDataQuery($callback) {
if ($this->inAlterDataQueryCall) {
$list = $this;
- $res = $callback($list->dataQuery, $list);
+ $res = call_user_func($callback, $list->dataQuery, $list);
if ($res) $list->dataQuery = $res;
return $list;
@@ -132,7 +128,7 @@ public function alterDataQuery($callback) {
$list->inAlterDataQueryCall = true;
try {
- $res = $callback($list->dataQuery, $list);
+ $res = call_user_func($callback, $list->dataQuery, $list);
if ($res) $list->dataQuery = $res;
}
catch (Exception $e) {
@@ -146,39 +142,6 @@ public function alterDataQuery($callback) {
}
/**
- * In 3.0.0 some methods in DataList mutate their list. We don't want to change that in the 3.0.x
- * line, but we don't want people relying on it either. This does the same as alterDataQuery, but
- * _does_ mutate the existing list.
- *
- * todo 3.1: All methods that call this need to call alterDataQuery instead
- */
- protected function alterDataQuery_30($callback) {
- Deprecation::notice('3.1', 'DataList will become immutable in 3.1');
-
- if ($this->inAlterDataQueryCall) {
- $res = $callback($this->dataQuery, $this);
- if ($res) $this->dataQuery = $res;
-
- return $this;
- }
- else {
- $this->inAlterDataQueryCall = true;
-
- try {
- $res = $callback($this->dataQuery, $this);
- if ($res) $this->dataQuery = $res;
- }
- catch (Exception $e) {
- $this->inAlterDataQueryCall = false;
- throw $e;
- }
-
- $this->inAlterDataQueryCall = false;
- return $this;
- }
- }
-
- /**
* Return a new DataList instance with the underlying {@link DataQuery} object changed
*
* @param DataQuery $dataQuery
@@ -190,6 +153,21 @@ public function setDataQuery(DataQuery $dataQuery) {
return $clone;
}
+ public function setDataQueryParam($keyOrArray, $val = null) {
+ $clone = clone $this;
+
+ if(is_array($keyOrArray)) {
+ foreach($keyOrArray as $key => $val) {
+ $clone->dataQuery->setQueryParam($key, $val);
+ }
+ }
+ else {
+ $clone->dataQuery->setQueryParam($keyOrArray, $val);
+ }
+
+ return $clone;
+ }
+
/**
* Returns the SQL query that will be used to get this DataList's records. Good for debugging. :-)
*
@@ -206,7 +184,7 @@ public function sql() {
* @return DataList
*/
public function where($filter) {
- return $this->alterDataQuery_30(function($query) use ($filter){
+ return $this->alterDataQuery(function($query) use ($filter){
$query->where($filter);
});
}
@@ -243,7 +221,7 @@ public function limit($limit, $offset = 0) {
if(!$limit && !$offset) {
return $this;
}
- return $this->alterDataQuery_30(function($query) use ($limit, $offset){
+ return $this->alterDataQuery(function($query) use ($limit, $offset){
$query->limit($limit, $offset);
});
}
@@ -281,7 +259,7 @@ public function sort() {
$sort = func_get_arg(0);
}
- return $this->alterDataQuery_30(function($query, $list) use ($sort, $col, $dir){
+ return $this->alterDataQuery(function($query, $list) use ($sort, $col, $dir){
if ($col) {
// sort('Name','Desc')
@@ -346,25 +324,24 @@ public function filter() {
throw new InvalidArgumentException('Incorrect number of arguments passed to filter()');
}
- // TODO 3.1: Once addFilter doesn't mutate self, this results in a double clone
- $clone = clone $this;
- $clone->addFilter($filters);
- return $clone;
+ return $this->addFilter($filters);
}
/**
* Return a new instance of the list with an added filter
*/
public function addFilter($filterArray) {
+ $list = $this;
+
foreach($filterArray as $field => $value) {
$fieldArgs = explode(':', $field);
$field = array_shift($fieldArgs);
$filterType = array_shift($fieldArgs);
$modifiers = $fieldArgs;
- $this->applyFilterContext($field, $filterType, $modifiers, $value);
+ $list = $list->applyFilterContext($field, $filterType, $modifiers, $value);
}
- return $this;
+ return $list;
}
/**
@@ -485,7 +462,6 @@ public function getRelationName($field) {
* @todo Deprecated SearchContexts and pull their functionality into the core of the ORM
*/
private function applyFilterContext($field, $comparisators, $modifiers, $value) {
- $t = singleton($this->dataClass())->dbObject($field);
if($comparisators) {
$className = "{$comparisators}Filter";
} else {
@@ -496,7 +472,8 @@ private function applyFilterContext($field, $comparisators, $modifiers, $value)
array_unshift($modifiers, $comparisators);
}
$t = new $className($field, $value, $modifiers);
- $t->apply($this->dataQuery());
+
+ return $this->alterDataQuery(array($t, 'apply'));
}
/**
@@ -581,7 +558,7 @@ public function subtract(SS_List $list) {
* @return DataList
*/
public function innerJoin($table, $onClause, $alias = null) {
- return $this->alterDataQuery_30(function($query) use ($table, $onClause, $alias){
+ return $this->alterDataQuery(function($query) use ($table, $onClause, $alias){
$query->innerJoin($table, $onClause, $alias);
});
}
@@ -595,7 +572,7 @@ public function innerJoin($table, $onClause, $alias = null) {
* @return DataList
*/
public function leftJoin($table, $onClause, $alias = null) {
- return $this->alterDataQuery_30(function($query) use ($table, $onClause, $alias){
+ return $this->alterDataQuery(function($query) use ($table, $onClause, $alias){
$query->leftJoin($table, $onClause, $alias);
});
}
@@ -810,9 +787,7 @@ public function find($key, $value) {
$SQL_col = sprintf('"%s"', Convert::raw2sql($key));
}
- // todo 3.1: In 3.1 where won't be mutating, so this can be on $this directly
- $clone = clone $this;
- return $clone->where("$SQL_col = '" . Convert::raw2sql($value) . "'")->First();
+ return $this->where("$SQL_col = '" . Convert::raw2sql($value) . "'")->First();
}
/**
@@ -836,9 +811,7 @@ public function setQueriedColumns($queriedColumns) {
public function byIDs(array $ids) {
$ids = array_map('intval', $ids); // sanitize
$baseClass = ClassInfo::baseDataClass($this->dataClass);
- $this->where("\"$baseClass\".\"ID\" IN (" . implode(',', $ids) .")");
-
- return $this;
+ return $this->where("\"$baseClass\".\"ID\" IN (" . implode(',', $ids) .")");
}
/**
@@ -849,10 +822,7 @@ public function byIDs(array $ids) {
*/
public function byID($id) {
$baseClass = ClassInfo::baseDataClass($this->dataClass);
-
- // todo 3.1: In 3.1 where won't be mutating, so this can be on $this directly
- $clone = clone $this;
- return $clone->where("\"$baseClass\".\"ID\" = " . (int)$id)->First();
+ return $this->where("\"$baseClass\".\"ID\" = " . (int)$id)->First();
}
/**
@@ -1008,9 +978,8 @@ public function newObject($initialFields = null) {
*/
public function remove($item) {
// By default, we remove an item from a DataList by deleting it.
- if($item instanceof $this->dataClass) $item->delete();
-
- }
+ $this->removeByID($item->ID);
+ }
/**
* Remove an item from this DataList by ID
@@ -1028,7 +997,7 @@ public function removeByID($itemID) {
* @return DataList
*/
public function reverse() {
- return $this->alterDataQuery_30(function($query){
+ return $this->alterDataQuery(function($query){
$query->reverseSort();
});
}
View
@@ -2722,9 +2722,9 @@ public static function get($callerClass = null, $filter = "", $sort = "", $join
if($limit && strpos($limit, ',') !== false) {
$limitArguments = explode(',', $limit);
- $result->limit($limitArguments[1],$limitArguments[0]);
+ $result = $result->limit($limitArguments[1],$limitArguments[0]);
} elseif($limit) {
- $result->limit($limit);
+ $result = $result->limit($limit);
}
if($join) $result = $result->join($join);
Oops, something went wrong.

0 comments on commit 1303d82

Please sign in to comment.