Simplify and improve logging for row skipping

zerocrates · zerocrates · commit abe91a6a9e4a · 2019-03-27T16:57:39.000-04:00
diff --git a/models/CsvImport/File.php b/models/CsvImport/File.php
@@ -98,8 +98,9 @@ public function parse()
             $this->_parseErrors[] = $e->getMessage() 
                 . ' ' . __('Please ensure that all column names are unique.');
             return false;
-        } catch (CsvImport_MissingColumnException $e) {
-            $this->_parseErrors[] = $e->getMessage()
+        }
+        if ($this->_columnExamples === false) {
+            $this->_parseErrors[] = __('Incorrect number of columns in first row.')
                 . ' ' . __('Please ensure that the CSV file is formatted correctly'
                 . ' and contains the expected number of columns for each row.');
             return false;
@@ -116,4 +117,4 @@ public function getErrorString()
     {
         return join(' ', $this->_parseErrors);
     }
-}
+}
diff --git a/models/CsvImport/Import.php b/models/CsvImport/Import.php
@@ -579,18 +579,24 @@ protected function _importLoop($startAt = null)
             if ($startAt) {
                 $rows->seek($startAt);
             }
-            $rows->skipInvalidRows(true);
             $this->_log("Running item import loop. Memory usage: %memory%");
             while ($rows->valid()) {
                 $row = $rows->current();
                 $index = $rows->key();
-                $this->skipped_row_count += $rows->getSkippedCount();
-                if ($item = $this->_addItemFromRow($row)) {
-                    release_object($item);
-                } else {
+
+                if ($row === false) {
+                    $this->skipped_row_count++;
+                    $this->_log("Row {$index}: Incorrect number of columns, skipped.", Zend_Log::WARN);
+                } elseif ($this->_rowIsBlank($row)) {
+                    $this->skipped_row_count++;
+                    $this->_log("Row {$index}: Blank row, skipped.", Zend_Log::WARN);
+                } elseif (!($item = $this->_addItemFromRow($row))) {
                     $this->skipped_item_count++;
-                    $this->_log("Skipped item on row #{$index}.", Zend_Log::WARN);
+                    $this->_log("Row {$index}: Skipped item.", Zend_Log::WARN);
+                } else {
+                    release_object($item);
                 }
+
                 $this->file_position = $this->getCsvFile()->getIterator()->tell();
                 if ($this->_batchSize && ($index % $this->_batchSize == 0)) {
                     $this->_log("Completed importing batch of $this->_batchSize "
@@ -599,7 +605,6 @@ protected function _importLoop($startAt = null)
                 }
                 $rows->next();
             }
-            $this->skipped_row_count += $rows->getSkippedCount();
             return $this->complete();
         } catch (Omeka_Job_Worker_InterruptException $e) {
             // Interruptions usually indicate that we should resume from
@@ -671,27 +676,28 @@ protected function _undoImportLoop()
     }
 
     /**
-     * Adds a new item based on a row string in the CSV file and returns it.
+     * Check if a given row is completely blank (no values).
      *
-     * @param string $row A row string in the CSV file
-     * @return Item|boolean The inserted item or false if an item could not be added.
+     * @param array $row A row of CSV values
+     * @return bool
      */
-    protected function _addItemFromRow($row)
+    protected function _rowIsBlank($row)
     {
-        // check if this row is all blank cells
-        $blank = TRUE;
         foreach ($row as $value) {
             if (trim($value) !== '') {
-                $blank = FALSE;
-                break;
+                return false;
             }
         }
-        // if nothing but blank cells, skip this row
-        if ($blank) {
-            $this->_log('Blank row.', Zend_Log::INFO);
-            return FALSE;
-        }
-
+        return true;
+    }
+    /**
+     * Adds a new item based on a row string in the CSV file and returns it.
+     *
+     * @param string $row A row string in the CSV file
+     * @return Item|boolean The inserted item or false if an item could not be added.
+     */
+    protected function _addItemFromRow($row)
+    {
         $result = $this->getColumnMaps()->map($row);
         $tags = $result[CsvImport_ColumnMap::TYPE_TAG];
         $itemMetadata = array(
diff --git a/models/CsvImport/MissingColumnException.php b/models/CsvImport/MissingColumnException.php
diff --git a/models/CsvImport/RowIterator.php b/models/CsvImport/RowIterator.php
@@ -19,8 +19,6 @@ class CsvImport_RowIterator implements SeekableIterator
     private $_valid = true;
     private $_colNames = array();
     private $_colCount = 0;
-    private $_skipInvalidRows = false;
-    private $_skippedRowCount = 0;
 
     /**
      * @param string $filePath
@@ -103,16 +101,7 @@ public function key()
      */
     public function next()
     {
-        try {
-            $this->_moveNext();
-        } catch (CsvImport_MissingColumnException $e) {
-            if ($this->_skipInvalidRows) {
-                $this->_skippedRowCount++;
-                $this->next();
-            } else {
-                throw $e;
-            }
-        }
+        $this->_moveNext();
     }
 
     /**
@@ -149,9 +138,6 @@ protected function _moveNext()
             $this->_currentRow = $this->_formatRow($nextRow);
         } else {
             $this->_currentRow = array();
-        }
-
-        if (!$this->_currentRow) {
             fclose($this->_handle);
             $this->_valid = false;
             $this->_handle = null;
@@ -187,38 +173,11 @@ public function getColumnNames()
         return $this->_colNames;
     }
 
-    /**
-     * Returns the number of rows that were skipped since the last time the
-     * function was called.
-     *
-     * Skipped count is reset to 0 after each call to getSkippedCount(). This
-     * makes it easier to aggregate the number over multiple job runs.
-     *
-     * @return int The number of rows skipped since last time function was called
-     */
-    public function getSkippedCount()
-    {
-        $skipped = $this->_skippedRowCount;
-        $this->_skippedRowCount = 0;
-        return $skipped;
-    }
-
-    /**
-     * Sets whether to skip invalid rows.
-     *
-     * @param boolean $flag
-     */
-    public function skipInvalidRows($flag)
-    {
-        $this->_skipInvalidRows = (boolean)$flag;
-    }
-
     /**
      * Formats a row.
      *
      * @throws LogicException
-     * @throws CsvImport_MissingColumnException
-     * @return array The formatted row
+     * @return array|false The formatted row, false if malformed (wrong number of columns)
      */
     protected function _formatRow($row)
     {
@@ -228,10 +187,7 @@ protected function _formatRow($row)
                 . "names have been set.");
         }
         if (count($row) != $this->_colCount) {
-            $printable = substr(join($this->_columnDelimiter, $row), 0, 30) . '...';
-            throw new CsvImport_MissingColumnException("Row beginning with "
-                . "'$printable' does not have the required {$this->_colCount} "
-                . "rows.");
+            return false;
         }
         for ($i = 0; $i < $this->_colCount; $i++)
         {
