From e77389d0c88225e68098fa8250a6b2ccdbf8a4ac Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 22 Jan 2016 09:29:53 +1300 Subject: [PATCH] API Standardise SS_List::map() implementation Fixes #1593 --- model/ArrayList.php | 27 ++++++++++++-------------- model/List.php | 2 +- model/Map.php | 36 +++++++++++++++++++++++------------ security/Member.php | 3 +-- tests/model/ArrayListTest.php | 8 ++++++-- tests/security/MemberTest.php | 4 ++-- 6 files changed, 46 insertions(+), 34 deletions(-) diff --git a/model/ArrayList.php b/model/ArrayList.php index 8fed45df7..9ce561b7f 100644 --- a/model/ArrayList.php +++ b/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 $titlefield - the value field of the result array - * @return array + * @param string $keyfield The 'key' field of the result array + * @param string $titlefield The value field of the result array + * @return SS_Map */ public function map($keyfield = 'ID', $titlefield = 'Title') { - $map = array(); - - foreach ($this->items as $item) { - $map[$this->extractValue($item, $keyfield)] = $this->extractValue( - $item, - $titlefield - ); - } - - return $map; + $list = clone $this; + return new SS_Map($list, $keyfield, $titlefield); } /** @@ -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]; + } } } diff --git a/model/List.php b/model/List.php index 633f80ec2..a9f6c3608 100644 --- a/model/List.php +++ b/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'); diff --git a/model/Map.php b/model/Map.php index 719625799..9f7d9af51 100644 --- a/model/Map.php +++ b/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 $rewoundItem->{$this->titleField}(); - } - - return $rewoundItem->{$this->titleField}; + return $this->extractValue($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->items->current()->{$this->titleField}(); - } + } + return $this->extractValue($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); } } diff --git a/security/Member.php b/security/Member.php index 066d0342f..7f4bfb5a7 100644 --- a/security/Member.php +++ b/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. - * @see map() + * @return SS_Map Returns an SS_Map that returns all Member data. */ public static function map_in_groups($groups = null) { $groupIDList = array(); diff --git a/tests/model/ArrayListTest.php b/tests/model/ArrayListTest.php index 1f91a58d6..848ea92a5 100644 --- a/tests/model/ArrayListTest.php +++ b/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() { diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php index 7f6faf3b4..b9f5ff37d 100644 --- a/tests/security/MemberTest.php +++ b/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');