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

API Make DataList and ArrayList immutable

docs/en/changelogs/3.1.0.md

@@ -30,3 +30,19 @@
  * Removed `SQLMap::map()`: Use DataList::("Member")->map()
  * Removed `SQLMap::mapInGroups()`: Use Member::map_in_groups()
  * 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.

model/ArrayList.php

@@ -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 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta
 	 * @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 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta
 	 * @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 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta
 			$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 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta
 			}
 		}
 
-		// TODO 3.1: This currently mutates existing array
-		$list = /* clone */ $this;
+		$list = clone $this;
 		$list->items = $itemsToKeep;
 		return $list;
 	}
@@ -504,8 +514,7 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta
 			}
 		}
 
-		// TODO 3.1: This currently mutates existing array
-		$list = /* clone */ $this;
+		$list = clone $this;
 		$list->items = $itemsToKeep;
 		return $list;
 	}

model/DataList.php

@@ -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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	/**
 	 * 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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 		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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 			$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) {
@@ -145,39 +141,6 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 		}
 	}
 
-	/**
-	 * 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
 	 *
@@ -190,6 +153,21 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 		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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 * @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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 		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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 			$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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 				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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 * @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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 			array_unshift($modifiers, $comparisators);
 		}
 		$t = new $className($field, $value, $modifiers);
-		$t->apply($this->dataQuery());
+
+		return $this->alterDataQuery(array($t, 'apply'));
 	}
 	
 	/**
@@ -581,7 +558,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 * @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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 * @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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 			$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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	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 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 */
 	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,8 +978,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 */
 	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);
 		}
 
 	/**
@@ -1028,7 +997,7 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 	 * @return DataList
 	 */
 	public function reverse() {
-		return $this->alterDataQuery_30(function($query){
+		return $this->alterDataQuery(function($query){
 			$query->reverseSort();
 		});
 	}

model/DataObject.php

@@ -2722,9 +2722,9 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 
 		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);

model/Filterable.php

@@ -19,8 +19,9 @@ interface SS_Filterable {
 	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 @@ interface SS_Filterable {
 	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

model/HasManyList.php

@@ -21,14 +21,16 @@ class HasManyList extends RelationList {
 		$this->foreignKey = $foreignKey;
 	}
 	
-	protected function foreignIDFilter() {
+	protected function foreignIDFilter($id = null) {
+		if ($id === null) $id = $this->getForeignID();
+
 		// Apply relation filter
-		if(is_array($this->foreignID)) {
+		if(is_array($id)) {
 			return "\"$this->foreignKey\" IN ('" .
-				implode("', '", array_map('Convert::raw2sql', $this->foreignID)) . "')";
-		} else if($this->foreignID !== null){
+				implode("', '", array_map('Convert::raw2sql', $id)) . "')";
+		} else if($id !== null){
 			return "\"$this->foreignKey\" = '" . 
-				Convert::raw2sql($this->foreignID) . "'";
+				Convert::raw2sql($id) . "'";
 		}
 	}
 
@@ -44,18 +46,20 @@ class HasManyList extends RelationList {
 			user_error("HasManyList::add() expecting a $this->dataClass object, or ID value", E_USER_ERROR);
 		}
 
+		$foreignID = $this->getForeignID();
+
 		// Validate foreignID
-		if(!$this->foreignID) {
+		if(!$foreignID) {
 			user_error("ManyManyList::add() can't be called until a foreign ID is set", E_USER_WARNING);
 			return;
 		}
-		if(is_array($this->foreignID)) {
+		if(is_array($foreignID)) {
 			user_error("ManyManyList::add() can't be called on a list linked to mulitple foreign IDs", E_USER_WARNING);
 			return;
 		}
 
 		$fk = $this->foreignKey;
-		$item->$fk = $this->foreignID;
+		$item->$fk = $foreignID;
 
 		$item->write();
 	}

model/Hierarchy.php

@@ -591,12 +591,13 @@ class Hierarchy extends DataExtension {
 		$id = $this->owner->ID;
 		
 		$children = DataObject::get($baseClass)
-			->where("\"{$baseClass}\".\"ParentID\" = $id AND \"{$baseClass}\".\"ID\" != $id");
-		if(!$showAll) $children = $children->where('"ShowInMenus" = 1');
+			->where("\"{$baseClass}\".\"ParentID\" = $id AND \"{$baseClass}\".\"ID\" != $id")
+			->setDataQueryParam(array(
+				'Versioned.mode' => $onlyDeletedFromStage ? 'stage_unique' : 'stage',
+				'Versioned.stage' => 'Live'
+			));
 
-		// Query the live site
-		$children->dataQuery()->setQueryParam('Versioned.mode', $onlyDeletedFromStage ? 'stage_unique' : 'stage');
-		$children->dataQuery()->setQueryParam('Versioned.stage', 'Live');
+		if(!$showAll) $children = $children->where('"ShowInMenus" = 1');
 
 		return $children;
 	}

model/Limitable.php

@@ -11,11 +11,11 @@
 interface SS_Limitable {
 
 	/**
-	 * Returns a filtered version of this where no more than $limit records are included.
+	 * Returns a new instance of this list where no more than $limit records are included.
 	 * If $offset is specified, then that many records at the beginning of the list will be skipped.
 	 * This matches the behaviour of the SQL LIMIT clause.
 	 * 
-	 * @return SS_List
+	 * @return SS_Limitable
 	 */
 	public function limit($limit, $offset = 0);
 

model/ManyManyList.php

@@ -9,7 +9,7 @@ class ManyManyList extends RelationList {
 	
 	protected $localKey;
 	
-	protected $foreignKey, $foreignID;
+	protected $foreignKey;
 
 	protected $extraFields;
 
@@ -48,19 +48,35 @@ class ManyManyList extends RelationList {
 	}
 
 	/**
-	 * Return a filter expression for the foreign ID.
+	 * Return a filter expression for when getting the contents of the relationship for some foreign ID
+	 * @return string
 	 */
-	protected function foreignIDFilter() {
+	protected function foreignIDFilter($id = null) {
+		if ($id === null) $id = $this->getForeignID();
+
 		// Apply relation filter
-		if(is_array($this->foreignID)) {
+		if(is_array($id)) {
 			return "\"$this->joinTable\".\"$this->foreignKey\" IN ('" . 
-				implode("', '", array_map('Convert::raw2sql', $this->foreignID)) . "')";
-		} else if($this->foreignID !== null){
+				implode("', '", array_map('Convert::raw2sql', $id)) . "')";
+		} else if($id !== null){
 			return "\"$this->joinTable\".\"$this->foreignKey\" = '" . 
-				Convert::raw2sql($this->foreignID) . "'";
+				Convert::raw2sql($id) . "'";
 		}
 	}
 
+	/**
+	 * Return a filter expression for the join table when writing to the join table
+	 *
+	 * When writing (add, remove, removeByID), we need to filter the join table to just the relevant
+	 * entries. However some subclasses of ManyManyList (Member_GroupSet) modify foreignIDFilter to
+	 * include additional calculated entries, so we need different filters when reading and when writing
+	 *
+	 * @return string
+	 */
+	protected function foreignIDWriteFilter($id = null) {
+		return $this->foreignIDFilter($id);
+	}
+
 	/**
 	 * Add an item to this many_many relationship
 	 * Does so by adding an entry to the joinTable.
@@ -74,21 +90,24 @@ class ManyManyList extends RelationList {
 				E_USER_ERROR);
 		}
 
+		$foreignIDs = $this->getForeignID();
+		$foreignFilter = $this->foreignIDWriteFilter();
+
 		// Validate foreignID
-		if(!$this->foreignID) {
+		if(!$foreignIDs) {
 			throw new Exception("ManyManyList::add() can't be called until a foreign ID is set", E_USER_WARNING);
 		}
 
-		if($filter = $this->foreignIDFilter()) {
+		if($foreignFilter) {
 			$query = new SQLQuery("*", array("\"$this->joinTable\""));
-			$query->setWhere($filter);
+			$query->setWhere($foreignFilter);
 			$hasExisting = ($query->count() > 0);
 		} else {
 			$hasExisting = false;	
 		}
 
 		// Insert or update
-		foreach((array)$this->foreignID as $foreignID) {
+		foreach((array)$foreignIDs as $foreignID) {
 			$manipulation = array();
 			if($hasExisting) {
 				$manipulation[$this->joinTable]['command'] = 'update';	
@@ -135,7 +154,7 @@ class ManyManyList extends RelationList {
 		$query = new SQLQuery("*", array("\"$this->joinTable\""));
 		$query->setDelete(true);
 
-		if($filter = $this->foreignIDFilter()) {
+		if($filter = $this->foreignIDWriteFilter($this->getForeignID())) {
 			$query->setWhere($filter);
 		} else {
 			user_error("Can't call ManyManyList::remove() until a foreign ID is set", E_USER_WARNING);
@@ -177,7 +196,7 @@ class ManyManyList extends RelationList {
 		if($this->extraFields) {
 			foreach($this->extraFields as $fieldName => $dbFieldSpec) {
 				$query = new SQLQuery("\"$fieldName\"", array("\"$this->joinTable\""));
-				if($filter = $this->foreignIDFilter()) {
+				if($filter = $this->foreignIDWriteFilter($this->getForeignID())) {
 					$query->setWhere($filter);
 				} else {
 					user_error("Can't call ManyManyList::getExtraData() until a foreign ID is set", E_USER_WARNING);

model/RelationList.php

@@ -7,33 +7,9 @@
  * @todo Is this additional class really necessary?
  */
 abstract class RelationList extends DataList {
-	protected $foreignID;
 
-	/**
-	 * Set the ID of the record that this ManyManyList is linking *from*.
-	 *
-	 * This is the mutatable version of this function, and will be protected only
-	 * from 3.1. Use forForeignID instead
-	 *
-	 * @param $id A single ID, or an array of IDs
-	 */
-	public function setForeignID($id) {
-		// If already filtered on foreign ID, remove that first
-		if($this->foreignID !== null) {
-			$oldFilter = $this->foreignIDFilter();
-			try {
-				$this->dataQuery->removeFilterOn($oldFilter);	
-			}
-			catch(InvalidArgumentException $e) { /* NOP */ }
-		}
-
-		// Turn a 1-element array into a simple value
-		if(is_array($id) && sizeof($id) == 1) $id = reset($id);
-		$this->foreignID = $id;
-		
-		$this->dataQuery->where($this->foreignIDFilter());
-
-		return $this;
+	public function getForeignID() {
+		return $this->dataQuery->getQueryParam('Foreign.ID');
 	}
 
 	/**
@@ -41,10 +17,34 @@ abstract class RelationList extends DataList {
 	 * @param $id An ID or an array of IDs.
 	 */
 	public function forForeignID($id) {
-		return $this->alterDataQuery_30(function($query, $list) use ($id){
-			$list->setForeignID($id);
-		});
+		// Turn a 1-element array into a simple value
+		if(is_array($id) && sizeof($id) == 1) $id = reset($id);
+
+		// Calculate the new filter
+		$filter = $this->foreignIDFilter($id);
+
+		$list = $this->alterDataQuery(function($query, $list) use ($id, $filter){
+			// Check if there is an existing filter, remove if there is
+			$currentFilter = $query->getQueryParam('Foreign.Filter');
+			if($currentFilter) {
+				try {
+					$query->removeFilterOn($currentFilter);
+				}
+				catch (Exception $e) { /* NOP */ }
 			}
 
-	abstract protected function foreignIDFilter();
+			// Add the new filter
+			$query->setQueryParam('Foreign.ID', $id);
+			$query->setQueryParam('Foreign.Filter', $filter);
+			$query->where($filter);
+		});
+
+		return $list;
+	}
+
+	/**
+	 * Returns a where clause that filters the members of this relationship to just the related items
+	 * @param $id (optional) An ID or an array of IDs - if not provided, will use the current ids as per getForeignID
+	 */
+	abstract protected function foreignIDFilter($id = null);
 }

model/Sortable.php

@@ -19,9 +19,10 @@ interface SS_Sortable {
 	public function canSortBy($by);
 
 	/**
-	 * Sorts this list by one or more fields. You can either pass in a single
+	 * Return a new instance of this list that is sorted by one or more fields. You can either pass in a single
 	 * field name and direction, or a map of field names to sort directions.
 	 *
+	 * @return SS_Sortable
 	 * @example $list = $list->sort('Name'); // default ASC sorting
 	 * @example $list = $list->sort('Name DESC'); // DESC sorting
 	 * @example $list = $list->sort('Name', 'ASC');
@@ -31,11 +32,10 @@ interface SS_Sortable {
 	
 	
 	/**
-	 * Reverses the list based on reversing the current sort.
+	 * Return a new instance of this list based on reversing the current sort.
 	 *
+	 * @return SS_Sortable
 	 * @example $list = $list->reverse();
-	 *
-	 * @return array
 	 */
 	public function reverse();
 }

model/UnsavedRelationList.php

@@ -239,25 +239,14 @@ class UnsavedRelationList extends ArrayList {
 		return $list->column('ID');
 	}
 
-	/**
-	 * Set the ID of the record that this RelationList is linking.
-	 *
-	 * Adds the 
-	 *
-	 * @param $id A single ID, or an array of IDs
-	 */
-	public function setForeignID($id) {
-		$class = singleton($this->baseClass);
-		$class->ID = 1;
-		return $class->{$this->relationName}()->setForeignID($id);
-	}
-	
 	/**
 	 * Returns a copy of this list with the relationship linked to the given foreign ID.
 	 * @param $id An ID or an array of IDs.
 	 */
 	public function forForeignID($id) {
-		return $this->setForeignID($id);
+		$class = singleton($this->baseClass);
+		$class->ID = 1;
+		return $class->{$this->relationName}()->forForeignID($id);
 	}
 
 	/**

model/Versioned.php

@@ -1003,10 +1003,10 @@ class Versioned extends DataExtension {
 			$containerClass = 'DataList') {
 
 		$result = DataObject::get($class, $filter, $sort, $join, $limit, $containerClass);
-		$dq = $result->dataQuery();
-		$dq->setQueryParam('Versioned.mode', 'stage');
-		$dq->setQueryParam('Versioned.stage', $stage);
-		return $result;
+		return $result->setDataQueryParam(array(
+			'Versioned.mode' => 'stage',
+			'Versioned.stage' => $stage
+		));
 	}
 	
 	public function deleteFromStage($stage) {
@@ -1049,8 +1049,10 @@ class Versioned extends DataExtension {
 	 */
 	public static function get_latest_version($class, $id) {
 		$baseClass = ClassInfo::baseDataClass($class);
-		$list = DataList::create($baseClass)->where("\"$baseClass\".\"RecordID\" = $id");
-		$list->dataQuery()->setQueryParam("Versioned.mode", "latest_versions");
+		$list = DataList::create($baseClass)
+			->where("\"$baseClass\".\"RecordID\" = $id")
+			->setDataQueryParam("Versioned.mode", "latest_versions");
+
 		return $list->First();
 	}
 	
@@ -1077,8 +1079,11 @@ class Versioned extends DataExtension {
 	 * In particular, this will query deleted records as well as active ones.
 	 */
 	public static function get_including_deleted($class, $filter = "", $sort = "") {
-		$list = DataList::create($class)->where($filter)->sort($sort);
-		$list->dataQuery()->setQueryParam("Versioned.mode", "latest_versions");
+		$list = DataList::create($class)
+			->where($filter)
+			->sort($sort)
+			->setDataQueryParam("Versioned.mode", "latest_versions");
+
 		return $list;
 	}
 	
@@ -1093,8 +1098,9 @@ class Versioned extends DataExtension {
 		$baseClass = ClassInfo::baseDataClass($class);
 		$list = DataList::create($baseClass)
 			->where("\"$baseClass\".\"RecordID\" = $id")
-			->where("\"$baseClass\".\"Version\" = " . (int)$version);
-		$list->dataQuery()->setQueryParam('Versioned.mode', 'all_versions');
+			->where("\"$baseClass\".\"Version\" = " . (int)$version)
+			->setDataQueryParam("Versioned.mode", 'all_versions');
+
 		return $list->First();
 	}
 
@@ -1104,8 +1110,10 @@ class Versioned extends DataExtension {
 	 */
 	public static function get_all_versions($class, $id) {
 		$baseClass = ClassInfo::baseDataClass($class);
-		$list = DataList::create($class)->where("\"$baseClass\".\"RecordID\" = $id");
-		$list->dataQuery()->setQueryParam('Versioned.mode', 'all_versions');
+		$list = DataList::create($class)
+			->where("\"$baseClass\".\"RecordID\" = $id")
+			->setDataQueryParam('Versioned.mode', 'all_versions');
+
 		return $list;
 	}
 	

search/SearchContext.php

@@ -153,7 +153,7 @@ class SearchContext extends Object {
 				$filter->setModel($this->modelClass);
 				$filter->setValue($value);
 				if(! $filter->isEmpty()) {
-					$filter->apply($query->dataQuery());
+					$query = $query->alterDataQuery(array($filter, 'apply'));
 				}
 			}
 		}

security/Group.php

@@ -236,7 +236,9 @@ class Group extends DataObject {
 		// Remove the default foreign key filter in prep for re-applying a filter containing all children groups.
 		// Filters are conjunctive in DataQuery by default, so this filter would otherwise overrule any less specific
 		// ones.
-		$result->dataQuery()->removeFilterOn('Group_Members');
+		$result = $result->alterDataQuery(function($query){
+			$query->removeFilterOn('Group_Members');
+		});
 		// Now set all children groups as a new foreign key
 		$groups = Group::get()->byIDs($this->collateFamilyIDs());
 		$result = $result->forForeignID($groups->column('ID'))->where($filter)->sort($sort)->limit($limit);

security/Member.php

@@ -1420,14 +1420,12 @@ class Member_GroupSet extends ManyManyList {
 	/**
 	 * Link this group set to a specific member.
 	 */
-	public function setForeignID($id) {
-		// Turn a 1-element array into a simple value
-		if(is_array($id) && sizeof($id) == 1) $id = reset($id);
-		$this->foreignID = $id;
+	public function foreignIDFilter($id = null) {
+		if ($id === null) $id = $this->getForeignID();
 
 		// Find directly applied groups
-		$manymanyFilter = $this->foreignIDFilter();
-		$groupIDs = DB::query('SELECT "GroupID" FROM "Group_Members" WHERE ' . $manymanyFilter)->column();
+		$manyManyFilter = parent::foreignIDFilter($id);
+		$groupIDs = DB::query('SELECT "GroupID" FROM "Group_Members" WHERE ' . $manyManyFilter)->column();
 
 		// Get all ancestors
 		$allGroupIDs = array();
@@ -1438,8 +1436,16 @@ class Member_GroupSet extends ManyManyList {
 		}
 		
 		// Add a filter to this DataList
-		if($allGroupIDs) $this->byIDs($allGroupIDs);
-		else $this->byIDs(array(0));
+		if($allGroupIDs) {
+			return "\"Group\".\"ID\" IN (" . implode(',', $allGroupIDs) .")";
+		}
+		else {
+			return "\"Group\".\"ID\" = 0";
+		}
+	}
+
+	public function foreignIDWriteFilter($id = null) {
+		return parent::foreignIDFilter($id);
 	}
 }
 

tests/model/ArrayListTest.php

@@ -229,14 +229,14 @@ class ArrayListTest extends SapphireTest {
 		));
 	}
 
-	public function testSortSimpleDefualtIsSortedASC() {
+	public function testSortSimpleDefaultIsSortedASC() {
 		$list = new ArrayList(array(
 			array('Name' => 'Steve'),
 			(object) array('Name' => 'Bob'),
 			array('Name' => 'John')
 		));
 
-		$list->sort('Name');
+		$list = $list->sort('Name');
 		$this->assertEquals($list->toArray(), array(
 			(object) array('Name' => 'Bob'),
 			array('Name' => 'John'),
@@ -250,7 +250,8 @@ class ArrayListTest extends SapphireTest {
 			(object) array('Name' => 'Bob'),
 			array('Name' => 'John')
 		));
-		$list->sort('Name','asc');
+
+		$list = $list->sort('Name','asc');
 		$this->assertEquals($list->toArray(), array(
 			(object) array('Name' => 'Bob'),
 			array('Name' => 'John'),
@@ -265,7 +266,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'John')
 		));
 
-		$list->sort('Name', 'DESC');
+		$list = $list->sort('Name', 'DESC');
 		$this->assertEquals($list->toArray(), array(
 			array('Name' => 'Steve'),
 			array('Name' => 'John'),
@@ -280,8 +281,8 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'Steve')
 		));
 
-		$list->sort('Name', 'ASC');
-		$list->reverse();
+		$list = $list->sort('Name', 'ASC');
+		$list = $list->reverse();
 		
 		$this->assertEquals($list->toArray(), array(
 			array('Name' => 'Steve'),
@@ -297,11 +298,11 @@ class ArrayListTest extends SapphireTest {
 			(object) array('Name'=>'Object3', 'F1'=>5, 'F2'=>2, 'F3'=>2),
 		));
 
-		$list->sort('F3', 'ASC');
+		$list = $list->sort('F3', 'ASC');
 		$this->assertEquals($list->first()->Name, 'Object3', 'Object3 should be first in the list');
 		$this->assertEquals($list->last()->Name, 'Object2', 'Object2 should be last in the list');
 
-		$list->sort('F3', 'DESC');
+		$list = $list->sort('F3', 'DESC');
 		$this->assertEquals($list->first()->Name, 'Object2', 'Object2 should be first in the list');
 		$this->assertEquals($list->last()->Name, 'Object3', 'Object3 should be last in the list');
 	}
@@ -313,11 +314,11 @@ class ArrayListTest extends SapphireTest {
 			(object) array('ID'=>2, 'Name'=>'Aron', 'Importance'=>1),
 		));
 		
-		$list->sort(array('Name'=>'ASC', 'Importance'=>'ASC'));
+		$list = $list->sort(array('Name'=>'ASC', 'Importance'=>'ASC'));
 		$this->assertEquals($list->first()->ID, 2, 'Aron.2 should be first in the list');
 		$this->assertEquals($list->last()->ID, 3, 'Bert.3 should be last in the list');
 		
-		$list->sort(array('Name'=>'ASC', 'Importance'=>'DESC'));
+		$list = $list->sort(array('Name'=>'ASC', 'Importance'=>'DESC'));
 		$this->assertEquals($list->first()->ID, 1, 'Aron.2 should be first in the list');
 		$this->assertEquals($list->last()->ID, 3, 'Bert.3 should be last in the list');
 	}
@@ -331,7 +332,7 @@ class ArrayListTest extends SapphireTest {
 			(object) array('Name' => 'Bob'),
 			array('Name' => 'John')
 		));
-		$list->filter('Name','Bob');
+		$list = $list->filter('Name','Bob');
 		$this->assertEquals(array((object)array('Name'=>'Bob')), $list->toArray(), 'List should only contain Bob');
 	}
 	
@@ -349,7 +350,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'Steve'),
 			array('Name' => 'John')
 		);
-		$list->filter('Name',array('Steve','John'));
+		$list = $list->filter('Name',array('Steve','John'));
 		$this->assertEquals($expected, $list->toArray(), 'List should only contain Steve and John');
 	}
 	
@@ -362,7 +363,7 @@ class ArrayListTest extends SapphireTest {
 			(object) array('Name' => 'Steve', 'ID' => 2),
 			array('Name' => 'John', 'ID' => 2)
 		));
-		$list->filter(array('Name'=>'Clair'));
+		$list = $list->filter(array('Name'=>'Clair'));
 		$this->assertEquals(array(), $list->toArray(), 'List should be empty');
 	}
 	
@@ -375,7 +376,7 @@ class ArrayListTest extends SapphireTest {
 			(object) array('Name' => 'Steve', 'ID' => 2),
 			array('Name' => 'John', 'ID' => 2)
 		));
-		$list->filter(array('Name'=>'Steve', 'ID'=>2));
+		$list = $list->filter(array('Name'=>'Steve', 'ID'=>2));
 		$this->assertEquals(array((object)array('Name'=>'Steve', 'ID'=>2)), $list->toArray(),
 			'List should only contain object Steve');
 	}
@@ -389,7 +390,7 @@ class ArrayListTest extends SapphireTest {
 			(object) array('Name' => 'Steve', 'ID' => 2),
 			array('Name' => 'John', 'ID' => 2)
 		));
-		$list->filter(array('Name'=>'Steve', 'ID'=>4));
+		$list = $list->filter(array('Name'=>'Steve', 'ID'=>4));
 		$this->assertEquals(array(), $list->toArray(), 'List should be empty');
 	}
 	
@@ -404,7 +405,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'Steve', 'ID' => 3, 'Age'=>43)
 		));
 		
-		$list->filter(array('Name'=>'Steve','Age'=>array(21, 43)));
+		$list = $list->filter(array('Name'=>'Steve','Age'=>array(21, 43)));
 		
 		$expected = array(
 			array('Name' => 'Steve', 'ID' => 1, 'Age'=>21),
@@ -426,7 +427,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'Steve', 'ID' => 3, 'Age'=>43)
 		));
 		
-		$list->filter(array('Name'=>array('Steve','Clair'),'Age'=>array(21, 43)));
+		$list = $list->filter(array('Name'=>array('Steve','Clair'),'Age'=>array(21, 43)));
 		
 		$expected = array(
 			array('Name' => 'Steve', 'ID' => 1, 'Age'=>21),
@@ -448,7 +449,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'John')
 		));
 		
-		$list->exclude('Name', 'Bob');
+		$list = $list->exclude('Name', 'Bob');
 		$expected = array(
 			array('Name' => 'Steve'),
 			array('Name' => 'John')
@@ -467,7 +468,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'John')
 		));
 		
-		$list->exclude('Name', 'Clair');
+		$list = $list->exclude('Name', 'Clair');
 		$expected = array(
 			array('Name' => 'Steve'),
 			array('Name' => 'Bob'),
@@ -485,7 +486,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'Bob'),
 			array('Name' => 'John')
 		));
-		$list->exclude('Name', array('Steve','John'));
+		$list = $list->exclude('Name', array('Steve','John'));
 		$expected = array(array('Name' => 'Bob'));
 		$this->assertEquals(1, $list->count());
 		$this->assertEquals($expected, $list->toArray(), 'List should only contain Bob');
@@ -501,7 +502,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'John', 'Age' => 21)
 		));
 		
-		$list->exclude(array('Name' => 'Bob', 'Age' => 21));
+		$list = $list->exclude(array('Name' => 'Bob', 'Age' => 21));
 		
 		$expected = array(
 			array('Name' => 'Bob', 'Age' => 32),
@@ -527,7 +528,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'phil', 'Age' => 16)
 		));
 
-		$list->exclude(array('Name'=>array('bob','phil'),'Age'=>array(10, 16)));
+		$list = $list->exclude(array('Name'=>array('bob','phil'),'Age'=>array(10, 16)));
 		$expected = array(
 			array('Name' => 'phil', 'Age' => 11),
 			array('Name' => 'bob', 'Age' => 12),
@@ -553,7 +554,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'phil', 'Age' => 16)
 		));
 
-		$list->exclude(array('Name'=>array('bob','phil'),'Age'=>array(10, 16),'Bananas'=>true));
+		$list = $list->exclude(array('Name'=>array('bob','phil'),'Age'=>array(10, 16),'Bananas'=>true));
 		$expected = array(
 			array('Name' => 'bob', 'Age' => 10),
 			array('Name' => 'phil', 'Age' => 11),
@@ -584,7 +585,7 @@ class ArrayListTest extends SapphireTest {
 			array('Name' => 'clair','Age' => 16, 'HasBananas'=>true)
 		));
 
-		$list->exclude(array('Name'=>array('bob','phil'),'Age'=>array(10, 16),'HasBananas'=>true));
+		$list = $list->exclude(array('Name'=>array('bob','phil'),'Age'=>array(10, 16),'HasBananas'=>true));
 		$expected = array(
 			array('Name' => 'bob', 'Age' => 10, 'HasBananas'=>false),
 			array('Name' => 'phil','Age' => 11, 'HasBananas'=>true),

tests/model/DataListTest.php

@@ -79,9 +79,14 @@ class DataListTest extends SapphireTest {
 	
 	public function testInnerJoin() {
 		$db = DB::getConn();
+
 		$list = DataObjectTest_TeamComment::get();
-		$list->innerJoin('DataObjectTest_Team', '"DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"',
-			'Team');
+		$list = $list->innerJoin(
+			'DataObjectTest_Team',
+			'"DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"',
+			'Team'
+		);
+
 		$expected = 'SELECT DISTINCT "DataObjectTest_TeamComment"."ClassName", "DataObjectTest_TeamComment"."Created",'
 			. ' "DataObjectTest_TeamComment"."LastEdited", "DataObjectTest_TeamComment"."Name",'
 			. ' "DataObjectTest_TeamComment"."Comment", "DataObjectTest_TeamComment"."TeamID",'
@@ -89,14 +94,20 @@ class DataListTest extends SapphireTest {
 			. ' THEN "DataObjectTest_TeamComment"."ClassName" ELSE '.$db->prepStringForDB('DataObjectTest_TeamComment')
 			. ' END AS "RecordClassName" FROM "DataObjectTest_TeamComment" INNER JOIN "DataObjectTest_Team" AS "Team"'
 			. ' ON "DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"';
+
 		$this->assertEquals($expected, $list->sql());
 	}
 	
 	public function testLeftJoin() {
 		$db = DB::getConn();
+
 		$list = DataObjectTest_TeamComment::get();
-		$list->leftJoin('DataObjectTest_Team', '"DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"',
-			'Team');
+		$list = $list->leftJoin(
+			'DataObjectTest_Team',
+			'"DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"',
+			'Team'
+		);
+
 		$expected = 'SELECT DISTINCT "DataObjectTest_TeamComment"."ClassName", "DataObjectTest_TeamComment"."Created",'
 			. ' "DataObjectTest_TeamComment"."LastEdited", "DataObjectTest_TeamComment"."Name",'
 			. ' "DataObjectTest_TeamComment"."Comment", "DataObjectTest_TeamComment"."TeamID",'
@@ -104,14 +115,16 @@ class DataListTest extends SapphireTest {
 			. ' THEN "DataObjectTest_TeamComment"."ClassName" ELSE '.$db->prepStringForDB('DataObjectTest_TeamComment')
 			. ' END AS "RecordClassName" FROM "DataObjectTest_TeamComment" LEFT JOIN "DataObjectTest_Team" AS "Team"'
 			. ' ON "DataObjectTest_Team"."ID" = "DataObjectTest_TeamComment"."TeamID"';
+
 		$this->assertEquals($expected, $list->sql());
 
 		// Test with namespaces (with non-sensical join, but good enough for testing)
 		$list = DataObjectTest_TeamComment::get();
-		$list->leftJoin(
+		$list = $list->leftJoin(
 			'DataObjectTest\NamespacedClass', 
 			'"DataObjectTest\NamespacedClass"."ID" = "DataObjectTest_TeamComment"."ID"'
 		);
+
 		$expected = 'SELECT DISTINCT "DataObjectTest_TeamComment"."ClassName", '
 			. '"DataObjectTest_TeamComment"."Created", '
 			. '"DataObjectTest_TeamComment"."LastEdited", '
@@ -126,6 +139,7 @@ class DataListTest extends SapphireTest {
 			. 'LEFT JOIN "DataObjectTest\NamespacedClass" ON '
 			. '"DataObjectTest\NamespacedClass"."ID" = "DataObjectTest_TeamComment"."ID"';
 		$this->assertEquals($expected, $list->sql(), 'Retains backslashes in namespaced classes');
+
 	}
 	
 	public function testToNestedArray() {