Merge pull request #4964 from tractorcow/pulls/4.0/tomap-standardise

API Standardise SS_List::map() implementation

model/ArrayList.php

@@ -275,21 +275,13 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta
 	/**
 	 * Returns a map of this list
 	 *
-	 * @param string $keyfield - the 'key' field of the result array
+	 * @param string $keyfield The 'key' field of the result array
-	 * @param string $titlefield - the value field of the result array
+	 * @param string $titlefield The value field of the result array
-	 * @return array
+	 * @return SS_Map
 	 */
 	public function map($keyfield = 'ID', $titlefield = 'Title') {
-		$map = array();
+		$list = clone $this;
-
+		return new SS_Map($list, $keyfield, $titlefield);
-		foreach ($this->items as $item) {
-			$map[$this->extractValue($item, $keyfield)] = $this->extractValue(
-				$item,
-				$titlefield
-			);
-		}
-
-		return $map;
 	}
 
 	/**
@@ -674,9 +666,14 @@ class ArrayList extends ViewableData implements SS_List, SS_Filterable, SS_Sorta
 	 */
 	protected function extractValue($item, $key) {
 		if (is_object($item)) {
-			return $item->$key;
+			if(method_exists($item, 'hasMethod') && $item->hasMethod($key)) {
+				return $item->{$key}();
+			}
+			return $item->{$key};
 		} else {
-			if (array_key_exists($key, $item)) return $item[$key];
+			if (array_key_exists($key, $item)) {
+				return $item[$key];
+			}
 		}
 	}
 

model/List.php

@@ -56,7 +56,7 @@ interface SS_List extends ArrayAccess, Countable, IteratorAggregate {
 	 *
 	 * @param  string $keyfield
 	 * @param  string $titlefield
-	 * @return array
+	 * @return SS_Map
 	 */
 	public function map($keyfield = 'ID', $titlefield = 'Title');
 

model/Map.php

@@ -286,7 +286,7 @@ class SS_Map_Iterator implements Iterator {
 	 * @param Iterator $items The iterator to build this map from
 	 * @param string $keyField The field to use for the keys
 	 * @param string $titleField The field to use for the values
-	 * @param array $fristItems An optional map of items to show first
+	 * @param array $firstItems An optional map of items to show first
 	 * @param array $lastItems An optional map of items to show last
 	 */
 	public function __construct(Iterator $items, $keyField, $titleField, $firstItems = null, $lastItems = null) {
@@ -326,11 +326,7 @@ class SS_Map_Iterator implements Iterator {
 			return $this->firstItems[$this->firstItemIdx][1];
 		} else {
 			if($rewoundItem) {
-				if($rewoundItem->hasMethod($this->titleField)) {
+				return $this->extractValue($rewoundItem, $this->titleField);
-					return $rewoundItem->{$this->titleField}();
-				}
-
-				return $rewoundItem->{$this->titleField};
 			} else if(!$this->items->valid() && $this->lastItems) {
 				$this->endItemIdx = 0;
 
@@ -349,12 +345,28 @@ class SS_Map_Iterator implements Iterator {
 			return $this->lastItems[$this->endItemIdx][1];
 		} else if(isset($this->firstItems[$this->firstItemIdx])) {
 			return $this->firstItems[$this->firstItemIdx][1];
-		} else {
+		}
-			if($this->items->current()->hasMethod($this->titleField)) {
+		return $this->extractValue($this->items->current(), $this->titleField);
-				return $this->items->current()->{$this->titleField}();
 	}
 
-			return $this->items->current()->{$this->titleField};
+	/**
+	 * Extracts a value from an item in the list, where the item is either an
+	 * object or array.
+	 *
+	 * @param  array|object $item
+	 * @param  string $key
+	 * @return mixed
+	 */
+	protected function extractValue($item, $key) {
+		if (is_object($item)) {
+			if(method_exists($item, 'hasMethod') && $item->hasMethod($key)) {
+				return $item->{$key}();
+			}
+			return $item->{$key};
+		} else {
+			if (array_key_exists($key, $item)) {
+				return $item[$key];
+			}
 		}
 	}
 
@@ -369,7 +381,7 @@ class SS_Map_Iterator implements Iterator {
 		} else if(isset($this->firstItems[$this->firstItemIdx])) {
 			return $this->firstItems[$this->firstItemIdx][0];
 		} else {
-			return $this->items->current()->{$this->keyField};
+			return $this->extractValue($this->items->current(), $this->keyField);
 		}
 	}
 

security/Member.php

@@ -1176,8 +1176,7 @@ class Member extends DataObject implements TemplateGlobalProvider {
 	 * If no $groups is passed, all members will be returned
 	 *
 	 * @param mixed $groups - takes a SS_List, an array or a single Group.ID
-	 * @return SQLMap Returns an SQLMap that returns all Member data.
+	 * @return SS_Map Returns an SS_Map that returns all Member data.
-	 * @see map()
 	 */
 	public static function map_in_groups($groups = null) {
 		$groupIDList = array();

tests/model/ArrayListTest.php

@@ -214,11 +214,15 @@ class ArrayListTest extends SapphireTest {
 			(object) array('ID' => 3, 'Name' => 'Bob'),
 			array('ID' => 5, 'Name' => 'John')
 		));
-		$this->assertEquals($list->map('ID', 'Name'), array(
+		$map = $list->map('ID', 'Name');
+		// Items added after calling map should not be included retroactively
+		$list->add(array('ID' => 7, 'Name' => 'Andrew'));
+		$this->assertInstanceOf('SS_Map', $map);
+		$this->assertEquals(array(
 			1 => 'Steve',
 			3 => 'Bob',
 			5 => 'John'
-		));
+		), $map->toArray());
 	}
 
 	public function testFind() {

tests/security/MemberTest.php

@@ -709,7 +709,7 @@ class MemberTest extends FunctionalTest {
 	 */
 	public function testMap_in_groupsReturnsAll() {
 		$members = Member::map_in_groups();
-		$this->assertEquals(13, count($members), 'There are 12 members in the mock plus a fake admin');
+		$this->assertEquals(13, $members->count(), 'There are 12 members in the mock plus a fake admin');
 	}
 
 	/**
@@ -717,7 +717,7 @@ class MemberTest extends FunctionalTest {
 	 */
 	public function testMap_in_groupsReturnsAdmins() {
 		$adminID = $this->objFromFixture('Group', 'admingroup')->ID;
-		$members = Member::map_in_groups($adminID);
+		$members = Member::map_in_groups($adminID)->toArray();
 
 		$admin = $this->objFromFixture('Member', 'admin');
 		$otherAdmin = $this->objFromFixture('Member', 'other-admin');