Add $url Array Support for MigrateItemsXML

It would be better to extend the existing MigrateItemsXML class to handle an array of XML files, rather than introduce an entire new class. See MigrateSourceXML for an example of how to manage a list of files.

Memory-wise, it looks like you're loading all the files at once, you should take one file at a time and explicitly close each one when you're done with it.

Re: the "hybrid" post - that's exactly the approach MigrateSourceXML takes, using XMLReader to grab each element identified by the element query (which is a restricted subset of xpath syntax, since we have to implement this search ourselves), then SimpleXML over each element retrieved (enabling you to use full xpath syntax within each element).

Sorry, I think you misunderstood when I said "extend" the MigrateItemXML class to handle multiple URLs - what I meant was not to define a new class extending it, but to modify that class so it can handle an array of URLs as well as a single URL, similarly to what MigrateSourceXML does. There's no need to introduce another class here, it can be enhanced without breaking existing code.

No, at the database level deletion is often slower than insertion, it's not surprising for rollback to be slower.

It definitely breaks the wine.inc role migration. Don't have time to look in detail, I did notice at least one code typo "chache_ids"...

When rerolling, please make sure to adhere to Drupal coding standards (such as a space after "if").

Don't be scared! The actual code looks good, I just finally got around to installing Dreditor, which makes it easier to get extra-picky about comments and coding conventions...

+++ b/plugins/sources/xml.inc
@@ -299,18 +299,22 @@ abstract class XMLMigration extends Migration {
+   * @array or @string if just a single url

@var array - the constructor parameter could be a string, but the property will always be an array.

+++ b/plugins/sources/xml.inc
@@ -299,18 +299,22 @@ abstract class XMLMigration extends Migration {
+   * $activeUrl just for better understanding there could be more files available.

Huh? Better described as essentially a cursor over the urls array, I think.

+++ b/plugins/sources/xml.inc
@@ -299,18 +299,22 @@ abstract class XMLMigration extends Migration {
+   * Stores the current loaded XML document.

currently

+++ b/plugins/sources/xml.inc
@@ -329,44 +333,49 @@ class MigrateItemsXML extends MigrateItems {
+  public function __construct($urls, $itemXpath='item', $itemIDXpath='id') {

Variable naming convention is lower-case separated by _, please don't change the parameters.

+++ b/plugins/sources/xml.inc
@@ -329,44 +333,49 @@ class MigrateItemsXML extends MigrateItems {
+   * Our public face is the URL list we're getting items from

Add a period at the end.

+++ b/plugins/sources/xml.inc
@@ -329,44 +333,49 @@ class MigrateItemsXML extends MigrateItems {
+    $spaces = '       ';
+    $urls = implode('</br>' . $spaces, $this->urls);
+    return 'urls = ' . $urls . '</br></br>' . $spaces . $this->itemXpath .
+    ' | item ID xpath = ' . $this->itemIDXpath;

Looks kind of hacky (and I think you meant <br />), how about an <ul>?

+++ b/plugins/sources/xml.inc
@@ -404,22 +413,47 @@ class MigrateItemsXML extends MigrateItems {
+   * additional store them in cache_ids;

Additionally store them in cache_ids.

+++ b/plugins/sources/xml.inc
@@ -404,22 +413,47 @@ class MigrateItemsXML extends MigrateItems {
+  protected $cache_ids = NULL;

Declare the property before the constructor. Also, don't forget to declare idsMap.

+++ b/plugins/sources/xml.inc
@@ -404,22 +413,47 @@ class MigrateItemsXML extends MigrateItems {
+      //Make sure, to load new xml.

// Make sure to load new xml.

+++ b/plugins/sources/xml.inc
@@ -404,22 +413,47 @@ class MigrateItemsXML extends MigrateItems {
+  public function computeCount() {

Reordering the functions adds to the size of the patch, and makes it harder to follow what's changed.

+++ b/plugins/sources/xml.inc
@@ -548,11 +563,14 @@ class MigrateItemsXML extends MigrateItems {
+   * if $id is not in the currentItems array, look for it in the idsMap, where ids are mapped
+   * in $url => $ids relations.

Misplaced comment - unnecessary anyway I think, the code comments cover it.

+++ b/plugins/sources/xml.inc
@@ -560,8 +578,22 @@ class MigrateItemsXML extends MigrateItems {
+    // Otherwise, get it fom the right url
+    // First get the rigth url from $idsMap
+    foreach ($this->idsMap as $url => $ids) {

Needs an indent.

Oh, and $cache_ids should be $cacheIDs (lowerCamel convention for class properties).

I wouldn't want to change the public API unless there's a really compelling reason.

Committed, thanks!

I think this commit breaks "drush mi --idlist=...". Don't have time to check more.

Restoring status, new patches for a committed feature should be separate issues.