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.
2024-10-22 14:05:37 +02:00 · 2012-12-12 17:22:45 +13:00 · 2012-12-12 17:22:45 +13:00 · 27113f82c3
commit 27113f82c3
parent 644cc79ebb
18 changed files with 262 additions and 223 deletions
--- a/docs/en/changelogs/3.1.0.md
+++ b/docs/en/changelogs/3.1.0.md
@ -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.
--- a/model/ArrayList.php
+++ b/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
 */
@ -309,8 +320,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 +386,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 +449,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 +512,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;
 	}
--- a/model/DataList.php
+++ b/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();
 	}
 	
 	/**
@ -1028,7 +998,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();
 		});
 	}
--- a/model/DataObject.php
+++ b/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);
--- a/model/Filterable.php
+++ b/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
--- a/model/HasManyList.php
+++ b/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)) {
-			return "\"$this->foreignKey\" IN ('" . 
-				implode("', '", array_map('Convert::raw2sql', $this->foreignID)) . "')";
-		} else if($this->foreignID !== null){
+		if(is_array($id)) {
+			return "\"$this->foreignKey\" IN ('" .
+				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();
 	}
--- a/model/Hierarchy.php
+++ b/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;
 	}
--- a/model/Limitable.php
+++ b/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);

--- a/model/ManyManyList.php
+++ b/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.
@ -73,22 +89,25 @@ class ManyManyList extends RelationList {
 			throw new InvalidArgumentException("ManyManyList::add() expecting a $this->dataClass object, or ID value",
 				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';	
@ -134,8 +153,8 @@ 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);
--- a/model/RelationList.php
+++ b/model/RelationList.php
@ -7,44 +7,44 @@
 * @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');
 	}
-	
+
 	/**
 	 * Returns a copy of this list with the ManyMany relationship linked to the given foreign ID.
 	 * @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 */ }
+			}
+
+			// Add the new filter
+			$query->setQueryParam('Foreign.ID', $id);
+			$query->setQueryParam('Foreign.Filter', $filter);
+			$query->where($filter);
 		});
+
+		return $list;
 	}
-	
-	abstract protected function foreignIDFilter();
+
+	/**
+	 * 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);
 }
--- a/model/Sortable.php
+++ b/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();
 }
--- a/model/UnsavedRelationList.php
+++ b/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);
 	}

 	/**
--- a/model/Versioned.php
+++ b/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;
 	}
 	
--- a/search/SearchContext.php
+++ b/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'));
 				}
 			}
 		}
--- a/security/Group.php
+++ b/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);
--- a/security/Member.php
+++ b/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);
 	}
 }

--- a/tests/model/ArrayListTest.php
+++ b/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),
--- a/tests/model/DataListTest.php
+++ b/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() {