Problem/Motivation

In the real world the header row doesn't necessary start in row 1. There are spreadsheets having the table header after a title or other content. Right now MigrateSourceSpreadsheet assumes that the header row is the first row in the spreadsheet. The only way to workaround this is to extend the class and fork a lot of code.

Proposed resolution

Introduce a first_row the $options array that is passed to constructor, defaulting to 1.

Remaining tasks

None.

User interface changes

None.

API changes

Migrations can specify a different starting row by passing the value in the $options array as first_row key.

class MyMigration extends Migration {
  ...
  $this->source = new MigrateSourceSpreadsheet(
    'foo.xls',
    'Sheet1',
    array('bar', 'baz', 'fooo'),
    array('first_row' => 4) // The header is on the 4th row.
  );
  ...
}

Data model changes

The content of $options array accepts an integer keyed as first_row.

Comments

claudiu.cristea’s picture

Status: Active » Needs review
StatusFileSize
new2.08 KB

Patch.

claudiu.cristea’s picture

Title: Allow spreadsheet source data on a different row that 1 » Allow spreadsheet source data on a row different that 1
mikeryan’s picture

Status: Needs review » Needs work

For consistency, how about using 'header_rows' as the CSV source does?

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new2.65 KB
new2.88 KB

OK. I changed also the value to be "zero based" as is in csv source.

Status: Needs review » Needs work

The last submitted patch, 4: allow_spreadsheet-2532222-4.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new2.65 KB
new487 bytes

Oops!

Status: Needs review » Needs work

The last submitted patch, 6: allow_spreadsheet-2532222-6.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
StatusFileSize
new2.69 KB
new1.03 KB

The HEAD changed in the meantime :)

Interdiff against #4.

Status: Needs review » Needs work

The last submitted patch, 8: allow_spreadsheet-2532222-8.patch, failed testing.

The last submitted patch, 8: allow_spreadsheet-2532222-8.patch, failed testing.

mikeryan’s picture

I'm getting the user test failures too, without your patch - I'll open an issue for that.

mikeryan’s picture

mikeryan’s picture

Status: Needs work » Fixed
Issue tags: +Migrate 2.9

Committed, thanks (also for your patience with the tests!).

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.