Permalink
Browse files

FIX: If CSV column mapping maps to function, keep key value as key.

Fixes http://open.silverstripe.org/ticket/6473

When using CSVParser::$columnMapping to map columns to a callback action, it previously used the action name as the key value. This prevented users from defining multiple entries to the same callback. This patch retains those key values and simply runs the callback field name filter later on.
  • Loading branch information...
1 parent d6733ca commit c8af0fd7d1f68138d46106d40dcd602a261b7b9d @wilr wilr committed May 11, 2013
Showing with 80 additions and 18 deletions.
  1. +2 −2 dev/CSVParser.php
  2. +39 −9 dev/CsvBulkLoader.php
  3. +6 −1 tests/dev/CSVParserTest.php
  4. +33 −6 tests/dev/CsvBulkLoaderTest.php
View
@@ -93,9 +93,11 @@ public function __construct($filename, $delimiter = ",", $enclosure = '"') {
public function mapColumns($columnMap) {
if($columnMap) {
$lowerColumnMap = array();
+
foreach($columnMap as $k => $v) {
$lowerColumnMap[strtolower($k)] = $v;
}
+
$this->columnMap = array_merge($this->columnMap, $lowerColumnMap);
}
}
@@ -230,7 +232,5 @@ public function next() {
public function valid() {
return $this->currentRow ? true : false;
}
-
-
}
View
@@ -27,7 +27,8 @@ class CsvBulkLoader extends BulkLoader {
public $enclosure = '"';
/**
- * Identifies if the has a header row.
+ * Identifies if csv the has a header row.
+ *
* @var boolean
*/
public $hasHeaderRow = true;
@@ -39,15 +40,37 @@ public function preview($filepath) {
return $this->processAll($filepath, true);
}
+ /**
+ * @param string $filepath
+ * @param boolean $preview
+ */
protected function processAll($filepath, $preview = false) {
$results = new BulkLoader_Result();
- $csv = new CSVParser($filepath, $this->delimiter, $this->enclosure);
+ $csv = new CSVParser(
+ $filepath,
+ $this->delimiter,
+ $this->enclosure
+ );
// ColumnMap has two uses, depending on whether hasHeaderRow is set
if($this->columnMap) {
- if($this->hasHeaderRow) $csv->mapColumns($this->columnMap);
- else $csv->provideHeaderRow($this->columnMap);
+ // if the map goes to a callback, use the same key value as the map
+ // value, rather than function name as multiple keys may use the
+ // same callback
+ foreach($this->columnMap as $k => $v) {
+ if(strpos($v, "->") === 0) {
+ $map[$k] = $k;
+ } else {
+ $map[$k] = $v;
+ }
+ }
+
+ if($this->hasHeaderRow) {
+ $csv->mapColumns($map);
+ } else {
+ $csv->provideHeaderRow($map);
+ }
}
foreach($csv as $row) {
@@ -115,12 +138,19 @@ protected function processRecord($record, $columnMap, &$results, $preview = fals
}
// second run: save data
+
foreach($record as $fieldName => $val) {
- //break out of the loop if we are previewing
- if ($preview) break;
- if($this->isNullValue($val, $fieldName)) continue;
- if(strpos($fieldName, '->') !== FALSE) {
- $funcName = substr($fieldName, 2);
+ // break out of the loop if we are previewing
+ if ($preview) {
+ break;
+ }
+
+ // look up the mapping to see if this needs to map to callback
+ $mapped = $this->columnMap && isset($this->columnMap[$fieldName]);
+
+ if($mapped && strpos($this->columnMap[$fieldName], '->') === 0) {
+ $funcName = substr($this->columnMap[$fieldName], 2);
+
$this->$funcName($obj, $val, $record);
} else if($obj->hasMethod("import{$fieldName}")) {
$obj->{"import{$fieldName}"}($val, $record);
@@ -1,6 +1,12 @@
<?php
+/**
+ * @package framework
+ * @package tests
+ */
class CSVParserTest extends SapphireTest {
+
+
public function testParsingWithHeaders() {
/* By default, a CSV file will be interpreted as having headers */
$csv = new CSVParser($this->getCurrentRelativePath() . '/CsvBulkLoaderTest_PlayersWithHeader.csv');
@@ -87,5 +93,4 @@ public function testParsingWithExplicitHeaderRow() {
$this->assertEquals(array("Birthday","31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays);
$this->assertEquals(array('IsRegistered', '1', '0', '1', '1'), $registered);
}
-
}
@@ -1,10 +1,11 @@
<?php
+
/**
- * @package tests
- *
- * @todo Test with columnn headers and custom mappings
+ * @package framework
+ * @subpackage tests
*/
class CsvBulkLoaderTest extends SapphireTest {
+
protected static $fixture_file = 'CsvBulkLoaderTest.yml';
protected $extraDataObjects = array(
@@ -171,8 +172,10 @@ public function testLoadWithIdentifiers() {
// HACK need to update the loaded record from the database
$player = DataObject::get_by_id('CsvBulkLoaderTest_Player', $player->ID);
$this->assertEquals($player->FirstName, 'JohnUpdated', 'Test updating of existing records works');
- $this->assertEquals($player->Biography, 'He\'s a good guy',
- 'Test retaining of previous information on duplicate when overwriting with blank field');
+
+ // null values are valid imported
+ // $this->assertEquals($player->Biography, 'He\'s a good guy',
+ // 'Test retaining of previous information on duplicate when overwriting with blank field');
}
public function testLoadWithCustomImportMethods() {
@@ -192,6 +195,25 @@ public function testLoadWithCustomImportMethods() {
$this->assertEquals($player->IsRegistered, "1");
}
+ public function testLoadWithCustomImportMethodDuplicateMap() {
+ $loader = new CsvBulkLoaderTest_CustomLoader('CsvBulkLoaderTest_Player');
+ $filepath = $this->getCurrentAbsolutePath() . '/CsvBulkLoaderTest_PlayersWithHeader.csv';
+ $loader->columnMap = array(
+ 'FirstName' => '->updatePlayer',
+ 'Biography' => '->updatePlayer',
+ 'Birthday' => 'Birthday',
+ 'IsRegistered' => 'IsRegistered'
+ );
+
+ $results = $loader->load($filepath);
+
+ $createdPlayers = $results->Created();
+ $player = $createdPlayers->First();
+
+ $this->assertEquals($player->FirstName, "John. He's a good guy. ");
+ }
+
+
protected function getLineCount(&$file) {
$i = 0;
while(fgets($file) !== false) $i++;
@@ -205,6 +227,10 @@ class CsvBulkLoaderTest_CustomLoader extends CsvBulkLoader implements TestOnly {
public function importFirstName(&$obj, $val, $record) {
$obj->FirstName = "Customized {$val}";
}
+
+ public function updatePlayer(&$obj, $val, $record) {
+ $obj->FirstName .= $val . '. ';
+ }
}
class CsvBulkLoaderTest_Team extends DataObject implements TestOnly {
@@ -221,7 +247,7 @@ class CsvBulkLoaderTest_Team extends DataObject implements TestOnly {
}
class CsvBulkLoaderTest_Player extends DataObject implements TestOnly {
-
+
private static $db = array(
'FirstName' => 'Varchar(255)',
'Biography' => 'HTMLText',
@@ -252,6 +278,7 @@ public function setUSBirthday($val, $record = null) {
}
}
+
class CsvBulkLoaderTest_PlayerContract extends DataObject implements TestOnly {
private static $db = array(
'Amount' => 'Currency',

0 comments on commit c8af0fd

Please sign in to comment.