Issue #2818933 by claudiu.cristea: Make row index match the spreadsheet row number

README.md

@@ -24,9 +24,8 @@ source:
   # The worksheet to be read.
   worksheet: 'Personnel list'
   # The first row from where the table starts. Points to the row that contains
-  # the table header. It's a "zero based" value. If the table row is the first
-  # row, this should be 0. The value of 3 means that the table header is on the
-  # fourth row.
+  # the table header. If the table row is the first row, this should be 1. The
+  # value of 3 means that the table header is on the third row.
   header_row: 3
   # Columns to be returned, basically a list of table header cell values.
   columns:

src/Plugin/migrate/source/Spreadsheet.php

@@ -45,7 +45,7 @@ class Spreadsheet extends SourcePluginBase implements ConfigurablePluginInterfac
     return [
       'file' => NULL,
       'worksheet' => NULL,
-      'header_row' => 0,
+      'header_row' => 1,
       'columns' => [],
       'keys' => [],
       'row_index_column' => NULL,

src/SpreadsheetIterator.php

@@ -53,7 +53,7 @@ class SpreadsheetIterator implements SpreadsheetIteratorInterface {
   protected $columnsCount;
 
   /**
-   * The relative index of the current row.
+   * The 'zero based' relative index of the current row.
    *
    * @var int
    */
@@ -68,7 +68,7 @@ class SpreadsheetIterator implements SpreadsheetIteratorInterface {
       if (!$this->getRowIndexColumn()) {
         throw new \RuntimeException("Row index should act as key but no name has been provided. Use SpreadsheetIterator::setRowIndexColumn() to provide a name for this column.");
       }
-      return [$this->currentRow];
+      return [$this->getAbsoluteRowIndex()];
     }
 
     return array_values(array_map(
@@ -114,7 +114,7 @@ class SpreadsheetIterator implements SpreadsheetIteratorInterface {
     return array_map(
       function ($column_delta) {
         if ($column_delta === -1) {
-          return $this->currentRow;
+          return $this->getAbsoluteRowIndex();
         }
         elseif ($cell = $this->getWorksheet()->getCellByColumnAndRow($column_delta, $this->getAbsoluteRowIndex(), FALSE)) {
           return $cell->getValue();
@@ -208,14 +208,21 @@ class SpreadsheetIterator implements SpreadsheetIteratorInterface {
    * {@inheritdoc}
    */
   public function getHeaderRow() {
-    return empty($this->configuration['header_row']) ? 0 : $this->configuration['header_row'];
+    return empty($this->configuration['header_row']) ? 1 : $this->configuration['header_row'];
   }
 
   /**
    * {@inheritdoc}
    */
   public function getRowIndexColumn() {
-    return empty($this->configuration['row_index_column']) ? NULL : $this->configuration['row_index_column'];
+    $column = empty($this->configuration['row_index_column']) ? NULL : $this->configuration['row_index_column'];
+    if ($column) {
+      $headers = $this->getHeaders();
+      if (isset($headers[$column])) {
+        throw new \InvalidArgumentException("The header column '$column' cannot be used as 'row_index_column'. Chose a value for 'row_index_column' that doesn't exist in header cells.");
+      }
+    }
+    return $column;
   }
 
   /**
@@ -224,11 +231,13 @@ class SpreadsheetIterator implements SpreadsheetIteratorInterface {
   public function getHeaders() {
     if (!isset($this->headers)) {
       for ($col = 0; $col < $this->getColumnsCount(); ++$col) {
-        $value = trim($this->getWorksheet()->getCellByColumnAndRow($col, $this->getHeaderRow() + 1)->getValue());
-        if (isset($this->headers[$value])) {
-          throw new \RuntimeException("Table header '{$value}' is duplicated.");
+        if ($cell = $this->getWorksheet()->getCellByColumnAndRow($col, $this->getHeaderRow(), FALSE)) {
+          $value = trim($cell->getValue());
+          if (isset($this->headers[$value])) {
+            throw new \RuntimeException("Table header '{$value}' is duplicated.");
+          }
         }
-        if ($value) {
+        if (!empty($value)) {
           $this->headers[$value] = $col;
         }
       }
@@ -262,13 +271,8 @@ class SpreadsheetIterator implements SpreadsheetIteratorInterface {
    * @return int
    */
   protected function getAbsoluteRowIndex() {
-    return
-      $this->getHeaderRow() +
-      $this->currentRow +
-      // Add 1 because PhpSpreadsheet expects a '1 based' row.
-      1 +
-      // Add 1 because the first data row starts immediately after header row.
-      1;
+    // Add 1 because the first data row starts immediately after header row.
+    return $this->getHeaderRow() + $this->currentRow + 1;
   }
 
 }

src/SpreadsheetIteratorInterface.php

@@ -18,16 +18,16 @@ interface SpreadsheetIteratorInterface extends \Iterator{
    *   - columns (string[]): An indexed array of columns.
    *   - keys (string[]): A list of columns that are giving the primary key.
    *   - header_row (int): The index of the first row from where the table
-   *     starts. It's a 'zero based' value that points to the row that contains
-   *     the table header. If the table row is the first this should be 0. A
-   *     value of 3 would mean that the table header is on the fourth row.
+   *     starts. It's the value of the spreadsheet row that contains the table
+   *     header. If the table row is the first this should be 1. A value of 3
+   *     would mean that the table header is on the third row.
    *   - row_index_column (string): The row index column name. The 'row index
    *     column' is a pseudo-column, that not exist on the worksheet, containing
-   *     the 'zero based' current index/position/delta of each row. The caller
-   *     can pass a name to be used for that column. If a name was passed, that
-   *     column will be also outputted along with the row, in ::current(). The
-   *     name can be passed also in 'keys' list. In that case the row index will
-   *     be or will be part of the primary key.
+   *     the current index/position/delta of each row. The caller can pass a
+   *     name to be used for that column. If a name was passed, that column will
+   *     be also outputted along with the row, in ::current(). The same name can
+   *     be passed also in 'keys' list. In that case the row index will be or
+   *     will be part of the primary key.
    *
    * @return $this
    */

tests/src/Unit/SpreadsheetIteratorTest.php

@@ -36,7 +36,7 @@ class SpreadsheetIteratorTest extends UnitTestCase  {
     $this->iterator = (new SpreadsheetIterator())
       ->setConfiguration([
         'worksheet' => $this->getWorksheet(),
-        'header_row' => 1,
+        'header_row' => 2,
         'columns' => ['a', 'c', 'd'],
       ]);
   }
@@ -73,20 +73,20 @@ class SpreadsheetIteratorTest extends UnitTestCase  {
     $this->iterator->setConfiguration($config);
 
     $this->assertTrue($this->iterator->valid());
-    $this->assertSame([0], $this->iterator->key());
-    $this->assertSame(['row' => 0, 'a' => 'a0', 'c' => 'c0', 'd' => 'd0'], $this->iterator->current());
+    $this->assertSame([3], $this->iterator->key());
+    $this->assertSame(['row' => 3, 'a' => 'a0', 'c' => 'c0', 'd' => 'd0'], $this->iterator->current());
 
     // Move the cursor.
     $this->iterator->next();
     $this->assertTrue($this->iterator->valid());
-    $this->assertSame([1], $this->iterator->key());
-    $this->assertSame(['row' => 1, 'a' => 'a1', 'c' => 'c1', 'd' => 'd1'], $this->iterator->current());
+    $this->assertSame([4], $this->iterator->key());
+    $this->assertSame(['row' => 4, 'a' => 'a1', 'c' => 'c1', 'd' => 'd1'], $this->iterator->current());
 
     // Move the cursor.
     $this->iterator->next();
     $this->assertTrue($this->iterator->valid());
-    $this->assertSame([2], $this->iterator->key());
-    $this->assertSame(['row' => 2, 'a' => 'a2', 'c' => 'c2', 'd' => 'd2'], $this->iterator->current());
+    $this->assertSame([5], $this->iterator->key());
+    $this->assertSame(['row' => 5, 'a' => 'a2', 'c' => 'c2', 'd' => 'd2'], $this->iterator->current());
 
     // Move the cursor. Should run out of set.
     $this->iterator->next();
@@ -95,19 +95,20 @@ class SpreadsheetIteratorTest extends UnitTestCase  {
     // Rewind.
     $this->iterator->rewind();
     $this->assertTrue($this->iterator->valid());
-    $this->assertSame([0], $this->iterator->key());
-    $this->assertSame(['row' => 0, 'a' => 'a0', 'c' => 'c0', 'd' => 'd0'], $this->iterator->current());
+    $this->assertSame([3], $this->iterator->key());
+    $this->assertSame(['row' => 3, 'a' => 'a0', 'c' => 'c0', 'd' => 'd0'], $this->iterator->current());
 
     // Try to return all columns.
     $config['columns'] = [];
     $this->iterator->setConfiguration($config);
     $this->assertTrue($this->iterator->valid());
-    $this->assertSame([0], $this->iterator->key());
-    $this->assertSame(['row' => 0, 'a' => 'a0', 'b' => 'b0', 'c' => 'c0', 'd' => 'd0'], $this->iterator->current());
+    $this->assertSame([3], $this->iterator->key());
+    $this->assertSame(['row' => 3, 'a' => 'a0', 'b' => 'b0', 'c' => 'c0', 'd' => 'd0'], $this->iterator->current());
 
     // Use different primary keys.
     $config['columns'] = ['a', 'd'];
     $config['keys'] = ['b', 'c'];
+    unset($config['row_index_column']);
     $this->iterator->setConfiguration($config);
     $this->assertTrue($this->iterator->valid());
     $this->assertSame(['b0', 'c0'], $this->iterator->key());