Spreadsheet source plugin

Sounds like it should be a separate contrib module.

I believe migrate itself is trying to support only core destinations, and it's my understanding that new migrations generally should not be added to migrate extras.

I'm closing as won't fix based on the above, but obviously open the issue again if anything disagrees.

Any news regarding this plugin?

Thanks!

Migrate is indeed only taking Destination plugins just for core but this is a Source plugin. I think an XLS source can be really handy when source data is changing frequently, often by hand edits.

Setting to CNR for Mike. If he doesn't want it, he will close this issue.

It's worth including, but a few points to clean up:

1. Documentation - notes in both the code, and a child page under http://drupal.org/node/1006986, on how to obtain and install the required library.
2. Error checking - throw an exception in the constructor if the library can't be loaded.
3. I'd like to see excess empty lines (like after if statements) removed.
4. Please submit a proper patch, including the .info file.

Thanks.

The code in #3 uses the phpExcelReader library (http://code.google.com/p/php-excel-reader/) which hasn't had a release since early 2009. Upon trying to use this library, it could not read my Excel file and simply called the PHP die() function leaving my site in a terrible state.

I have made the following changes to the code in #3:

• Updated the code to use the PHPExcel library (http://phpexcel.codeplex.com/) which seems to be a much more robust and active project.
• Added instructions for installing the library at the top of the file.
• Added error checking for various points of failure, although no exception is being thrown, so I'd like to know what type of exception should be thrown and where I could find an example?

A child page can be created at http://drupal.org/node/1006986 once the code is committed to the project.

Patch is attached!

Rather than using watchdog(), please throw a MigrateException.

I tried throwing a MigrationException, but this completely broke any page on the site related to migrations, I am guessing because the exceptions were not being handled. I looked through the other source plugins, and none of them throw a MigrationException, but instead use the Migration::displayMessage() function, so this is what I decided to go with. This seems to accomplish my initial goal of alerting the user that something is wrong but not breaking the entire migration process, including viewing the migration overview page. Let me know if throwing a MigrationException was the correct way to go and that I was doing something wrong.

I think the handling of the libraries api could be a bit cleaner. By using hook_libraries_info(), it can auto-load PHPExcel.php without the slightly more clumsy concatenation method that's in #10.

/**
 * Implements hook_libraries_info().
 */
function phpexcel_libraries_info() {
  $libraries['PHPExcel'] = array(
    'name' => 'PHPExcel',
    'vendor url' => 'http://phpexcel.codeplex.com/',
    'download url' => 'https://github.com/PHPOffice/PHPExcel',
    'version arguments' => array(
      'file' => 'changelog.txt',
      'pattern' => "/@version\s+([0-9\.]+)/",
      'lines' => 22,
    ),
    'files' => array(
      'php' => array(
        'Classes/PHPExcel.php',
      ),
    ),
  );

  return $libraries;
}

By using the hook, including the PHPExcel library in a later function looks more like libraries_load('PHPExcel');

The PHPExcel library has the ability to do some auto-detection of file types. It can interpret that it should start with a BIFF parser by the xls extension, but then can discover that's wrong and switch to a CSV parser. I see that's handled with the PHPExcel_IOFactory::identify($this->file);, but then it can be misleading to say that this is just a XLS parser, because it could also then read gnumeric, xlsx, or some others. Perhaps MigrateSourceSpreadsheet is more appropriate, if you're willing to use that more broad umbrella.

Upon closer investigation, #10 just assumes the libraries module is installed. I'll propose an update.

Here's my progress so far. I haven't actually run this code, but it should be a lot closer to what I've mentioned.

It includes a new libraries hook. I didn't make libraries a dependency since hooks can just hang "out there" innocently.

Adjusting the title to match my proposal.

I've been trying to implement a new migration with this plugin and as part of that switched to throw an exception when the filename wasn't available. The problem I have with that seems to be shared by others - when intending to do a dynamic migration (where the source file isn't statically located) loading admin/content/migrate throws the exception and the page completely fails to load then. When loading the baseball example file, at least the migration page still loads even if the source files aren't where they're supposed to be; it throws a PHP warning instead.

Thought I had replied a little ways back... Yes, an exception in the constructor doesn't work too well, better to call $this->displayMessage().

With libraries not installed I've got the following error:

[error] [client XXX.XXX.XXX.XX] PHP Fatal error:  Can't use function return value in write context in [...]/modules/migrate/plugins/sources/spreadsheet.inc on line 110, referer: [...]

So this patch will make migrate dependent on libraries module and I think the best place for this is in a separate custom module not in migrate core (or at least, even if this will be in core please make it as separate module like Migrate UI or Migrate Example).

Also the module should have dependencies on libraries because unespected results could happen if not.

Starting to try out #14

I'd say the version callback approach is not really reliable for this library:
https://github.com/PHPOffice/PHPExcel/blob/develop/changelog.txt#L22
We should hardcore the 'version' element.

Edit: See #1908282: Libraries Usage causes Problem on detecting the version number.

+++ b/plugins/sources/spreadsheet.inc
@@ -0,0 +1,219 @@
+      if (empty(libraries_load('PHPExcel'))) {

Instead of looking for an empty library, it should look if the library has been loaded, that would solve #18 too.

      $library = libraries_load('PHPExcel');
      if (empty($library['loaded'])) {

Is there any reason not to rely on PHPExcel module to include the libraries?

Here's a new patch that reliefs migrate from the integration with PHPExcel so the adding of the library is PHPExcel module business.

I've been trying this patch with success so far, is there anything left for getting it in?

Hey pcambra. I'll start reviewing this shortly. One recommendation for speeding up the process would be a test migrate patch to review it against. I'll submit that if I can verify #20 works as expected.

Hi pcambra. I'm trying to create a test that can use the patch in #20, but so far no luck. Here is #20 + some sample modifications on the migrate_example module to test against. I'm seeing the following:

PHPExcel_Exception: Your requested sheet index: 0 is out of bounds. The actual number of sheets is 0. in PHPExcel->getSheet() (line 333 of /assets/d7-base/releases/20130907191316/profiles/nmd/libraries/PHPExcel/Classes/PHPExcel.php).

I'm using the phpexcel-7.x-3.7 module with the 1.7.9 library. I put this together very quickly and there may be an issue with the code of my example migration. However, it's looking like the basic loading of the spreadsheet is throwing an error as per error message above. I'm attaching a patch and a test XLS file to use for testing purposes. If there is some obvious issue in my setup, let me know and I'll be happy to continue testing.

+++ b/migrate_example/spreadsheet.inc
@@ -0,0 +1,41 @@
+    $this->addFieldMapping('uid')
+      ->defaultValue(1);
+    $this->addFieldMapping('title', 'title');
+    $this->addFieldMapping('body', 'body');
+  }

I think that the problem is that the source expects "title" and "body" as the first row of the xls file.

So if your xls file has the headers expected, it should be fine (it's probably case sensitive, so 'Title' should be used).

I've encountered some really bad performance problems in this approach and I'm using #20 as base to include a performance boost, instead of checking all rows and cells, it uses only those in the field mappings.

@rickmanelius sorry for overriding your stuff but needed something green. Feel free to place your example module on top of this one again.

Ooops, #25 is not what I wanted, here's a patch with the source field set correctly calculated

Updated the existing patch so that it doesn't look at mapped migrate fields and functions more like the CSV source.

* Added $columns array to constructor with an empty array as default.
* Updated getNextRow() to look at columns array, or pull all items if empty.

Saved the file from #27 again as UTF-8 to make sure encoding is correct.

Forgot to change to needs review.

Fixed line endings and formatting issue.

Fixed the issue with the EoF encoding.

Could someone who hasn't contributed to the patch test it and give it an RTBC?

Thanks.

I tested and verified that it worked. However, there is a small issue with an if condition where it would say "The Libraries API module is not enabled." when it could be the libraries module not enabled OR the phpexcel module not enabled. I made a correction to that logic in the provided patch.

At long last, committed!