Permalink
Browse files

API Make DataList and ArrayList immutable

In 3.0 there was some confusion about whether DataLists and ArrayLists
were mutable or not. If DataLists were immutable, they'd return the result, and your code
would look like

  $list = $list->filter(....);

If DataLists were mutable, they'd operate on themselves, returning nothing, and your code
would look like

 $list->filter(....);

This makes all DataLists and ArrayList immutable for all _searching_ operations.
Operations on DataList that modify the underlying SQL data store remain mutating.

- These functions no longer mutate the existing object, and if you do not capture the value
returned by them will have no effect:

  ArrayList#reverse
  ArrayList#sort
  ArrayList#filter
  ArrayList#exclude

  DataList#dataQuery (use DataList#alterDataQuery to modify dataQuery in a safe manner)
  DataList#where
  DataList#limit
  DataList#sort
  DataList#addFilter
  DataList#applyFilterContext
  DataList#innerJoin
  DataList#leftJoin
  DataList#find
  DataList#byIDs
  DataList#reverse

- DataList#setDataQueryParam has been added as syntactic sugar around the most common
cause of accessing the dataQuery directly - setting query parameters

- RelationList#setForeignID has been removed. Always use RelationList#forForeignID
when querying, and overload RelationList#foreignIDList when subclassing.

- Relatedly,the protected variable RelationList->foreignID has been removed, as the ID is
now stored on a query parameter. Use RelationList#getForeignID to read it.
  • Loading branch information...
1 parent 644cc79 commit 27113f82c30dd08a2d60bf0f481ae06054363ad0 Hamish Friedlander committed Dec 12, 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
*/
@@ -309,8 +320,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 +386,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 +449,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 +512,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();
}
/**
@@ -1028,7 +998,7 @@ public function removeByID($itemID) {
* @return DataList
*/
public function reverse() {
- return $this->alterDataQuery_30(function($query){
+ return $this->alterDataQuery(function($query){
$query->reverseSort();
});
}
@@ -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);
@@ -19,8 +19,9 @@
public function canFilterBy($by);
/**
- * Filter the list to include items with these charactaristics
- *
+ * Return a new instance of this list that only includes items with these charactaristics
+ *
+ * @return SS_Filterable
* @example $list = $list->filter('Name', 'bob'); // only bob in the list
* @example $list = $list->filter('Name', array('aziz', 'bob'); // aziz and bob in list
* @example $list = $list->filter(array('Name'=>'bob, 'Age'=>21)); // bob with the age 21
@@ -31,8 +32,9 @@ public function canFilterBy($by);
public function filter();
/**
- * Exclude the list to not contain items with these charactaristics
+ * Return a new instance of this list that excludes any items with these charactaristics
*
+ * @return SS_Filterable
* @example $list = $list->exclude('Name', 'bob'); // exclude bob from list
* @example $list = $list->exclude('Name', array('aziz', 'bob'); // exclude aziz and bob from list
* @example $list = $list->exclude(array('Name'=>'bob, 'Age'=>21)); // exclude bob that has Age 21
Oops, something went wrong.

0 comments on commit 27113f8

Please sign in to comment.