From 33cce8a54c28a0ec3602cf85509e8bf17f98dfbb Mon Sep 17 00:00:00 2001 From: webchick <drupal@webchick.net> Date: Wed, 30 Sep 2015 11:35:26 -0700 Subject: [PATCH] Issue #2571499 by benjy, mikeryan, phenaproxima: idMap source and destination id filtering requires keys --- .../modules/migrate/src/MigrateExecutable.php | 4 +- .../src/Plugin/MigrateIdMapInterface.php | 28 ++-- .../migrate/src/Plugin/migrate/id_map/Sql.php | 131 +++++++----------- .../src/Plugin/migrate/process/Migration.php | 1 + .../migrate/src/Tests/MigrateRollbackTest.php | 8 +- .../migrate/src/Tests/MigrateSkipRowTest.php | 4 +- .../tests/src/Unit/MigrateSqlIdMapTest.php | 30 ++-- 7 files changed, 83 insertions(+), 123 deletions(-) diff --git a/core/modules/migrate/src/MigrateExecutable.php b/core/modules/migrate/src/MigrateExecutable.php index b037c790cf11..9799842e01c0 100644 --- a/core/modules/migrate/src/MigrateExecutable.php +++ b/core/modules/migrate/src/MigrateExecutable.php @@ -327,7 +327,7 @@ public function rollback() { $destination = $this->migration->getDestinationPlugin(); // Loop through each row in the map, and try to roll it back. - foreach ($id_map as $serialized_key => $map_row) { + foreach ($id_map as $map_row) { $destination_key = $id_map->currentDestination(); if ($destination_key) { $this->getEventDispatcher() @@ -336,7 +336,7 @@ public function rollback() { $this->getEventDispatcher() ->dispatch(MigrateEvents::POST_ROW_DELETE, new MigrateRowDeleteEvent($this->migration, $destination_key)); // We're now done with this row, so remove it from the map. - $id_map->delete(unserialize($serialized_key)); + $id_map->deleteDestination($destination_key); } // Check for memory exhaustion. diff --git a/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php index 144c8999f8d5..36be2ddbbf1f 100644 --- a/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php @@ -57,7 +57,7 @@ public function saveIdMapping(Row $row, array $destination_id_values, $status = * Saves a message related to a source record in the migration message table. * * @param array $source_id_values - * The source identifier values of the record in error. + * The source identifier keyed values of the record, e.g. ['nid' => 5]. * @param string $message * The message to record. * @param int $level @@ -69,7 +69,7 @@ public function saveMessage(array $source_id_values, $message, $level = Migratio * Retrieves an iterator over messages relate to source records. * * @param array $source_id_values - * (optional) The source identifier values of a specific record to retrieve. + * (optional) The source identifier keyed values of the record, e.g. ['nid' => 5]. * If empty, all messages are retrieved. * @param int $level * (optional) Message severity. If NULL, retrieve messages of all severities. @@ -134,7 +134,7 @@ public function messageCount(); * Deletes the map and message entries for a given source record. * * @param array $source_id_values - * The source identifier values of the record to delete. + * The source identifier keyed values of the record, e.g. ['nid' => 5]. * @param bool $messages_only * TRUE to only delete the migrate messages. */ @@ -144,19 +144,10 @@ public function delete(array $source_id_values, $messages_only = FALSE); * Deletes the map and message table entries for a given destination row. * * @param array $destination_id_values - * The destination identifier values we should do the deletes for. + * The destination identifier key value pairs we should do the deletes for. */ public function deleteDestination(array $destination_id_values); - /** - * Deletes the map and message entries for a set of given source records. - * - * @param array $source_id_values - * The identifier values of the sources we should do the deletes for. Each - * array member is an array of identifier values for one source row. - */ - public function deleteBulk(array $source_id_values); - /** * Clears all messages from the map. */ @@ -166,7 +157,7 @@ public function clearMessages(); * Retrieves a row from the map table based on source identifier values. * * @param array $source_id_values - * The source identifier values of the record to retrieve. + * The source identifier keyed values of the record, e.g. ['nid' => 5]. * * @return array * The raw row data as an associative array. @@ -177,7 +168,7 @@ public function getRowBySource(array $source_id_values); * Retrieves a row by the destination identifiers. * * @param array $destination_id_values - * The destination identifier values of the record to retrieve. + * The destination identifier keyed values of the record, e.g. ['nid' => 5]. * * @return array * The row(s) of data. @@ -202,10 +193,11 @@ public function getRowsNeedingUpdate($count); * (possibly multi-field) source identifier value mapped to it. * * @param array $destination_id_values - * The destination identifier values of the record. + * The destination identifier keyed values of the record, e.g. ['nid' => 5]. * * @return array - * The source identifier values of the record, or NULL on failure. + * The source identifier keyed values of the record, e.g. ['nid' => 5], or + * an empty array on failure. */ public function lookupSourceID(array $destination_id_values); @@ -216,7 +208,7 @@ public function lookupSourceID(array $destination_id_values); * (possibly multi-field) destination identifier value it is mapped to. * * @param array $source_id_values - * The source identifier values of the record. + * The source identifier keyed values of the record, e.g. ['nid' => 5]. * * @return array * The destination identifier values of the record, or NULL on failure. diff --git a/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php index ff832ef52b9f..95499dc50340 100644 --- a/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php @@ -8,8 +8,6 @@ namespace Drupal\migrate\Plugin\migrate\id_map; use Drupal\Component\Utility\Unicode; -use Drupal\Core\Database\Connection; -use Drupal\Core\Entity\EntityTypeInterface; use Drupal\Core\Field\BaseFieldDefinition; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; use Drupal\Core\Plugin\PluginBase; @@ -428,8 +426,8 @@ protected function getFieldSchema(array $id_definition) { public function getRowBySource(array $source_id_values) { $query = $this->getDatabase()->select($this->mapTableName(), 'map') ->fields('map'); - foreach ($this->sourceIdFields() as $source_id) { - $query = $query->condition("map.$source_id", array_shift($source_id_values), '='); + foreach ($this->sourceIdFields() as $field_name => $source_id) { + $query->condition("map.$source_id", $source_id_values[$field_name], '='); } $result = $query->execute(); return $result->fetchAssoc(); @@ -441,8 +439,8 @@ public function getRowBySource(array $source_id_values) { public function getRowByDestination(array $destination_id_values) { $query = $this->getDatabase()->select($this->mapTableName(), 'map') ->fields('map'); - foreach ($this->destinationIdFields() as $destination_id) { - $query = $query->condition("map.$destination_id", array_shift($destination_id_values), '='); + foreach ($this->destinationIdFields() as $field_name => $destination_id) { + $query->condition("map.$destination_id", $destination_id_values[$field_name], '='); } $result = $query->execute(); return $result->fetchAssoc(); @@ -467,28 +465,36 @@ public function getRowsNeedingUpdate($count) { /** * {@inheritdoc} */ - public function lookupSourceID(array $destination_id) { - $query = $this->getDatabase()->select($this->mapTableName(), 'map') - ->fields('map', $this->sourceIdFields()); - foreach ($this->destinationIdFields() as $key_name) { - $query = $query->condition("map.$key_name", array_shift($destination_id), '='); + public function lookupSourceID(array $destination_id_values) { + $source_id_fields = $this->sourceIdFields(); + $query = $this->getDatabase()->select($this->mapTableName(), 'map'); + foreach ($source_id_fields as $source_field_name => $idmap_field_name) { + $query->addField('map', $idmap_field_name, $source_field_name); + } + foreach ($this->destinationIdFields() as $field_name => $destination_id) { + $query->condition("map.$destination_id", $destination_id_values[$field_name], '='); } $result = $query->execute(); - $source_id = $result->fetchAssoc(); - return array_values($source_id ?: array()); + return $result->fetchAssoc() ?: []; } /** * {@inheritdoc} */ - public function lookupDestinationId(array $source_id) { - if (empty($source_id)) { + public function lookupDestinationId(array $source_id_values) { + if (empty($source_id_values)) { return array(); } $query = $this->getDatabase()->select($this->mapTableName(), 'map') ->fields('map', $this->destinationIdFields()); - foreach ($this->sourceIdFields() as $key_name) { - $query = $query->condition("map.$key_name", array_shift($source_id), '='); + + // When looking up the destination ID we require an array with both the + // source key and value, e.g. ['nid' => 41]. However, the Migration process + // plugin doesn't currently have a way to get the source key so we presume + // the values have been passed through in the correct order. + $have_keys = !isset($source_id_values[0]); + foreach ($this->sourceIdFields() as $field_name => $source_id) { + $query->condition("map.$source_id", $have_keys ? $source_id_values[$field_name] : array_shift($source_id_values), '='); } $result = $query->execute(); $destination_id = $result->fetchAssoc(); @@ -544,13 +550,12 @@ public function saveIdMapping(Row $row, array $destination_id_values, $source_ro * {@inheritdoc} */ public function saveMessage(array $source_id_values, $message, $level = MigrationInterface::MESSAGE_ERROR) { - $count = 1; - foreach ($source_id_values as $id_value) { - $fields['sourceid' . $count++] = $id_value; + foreach ($this->sourceIdFields() as $field_name => $source_id) { // If any key value is not set, we can't save. - if (!isset($id_value)) { + if (!isset($source_id_values[$field_name])) { return; } + $fields[$source_id] = $source_id_values[$field_name]; } $fields['level'] = $level; $fields['message'] = $message; @@ -565,10 +570,12 @@ public function saveMessage(array $source_id_values, $message, $level = Migratio public function getMessageIterator(array $source_id_values = [], $level = NULL) { $query = $this->getDatabase()->select($this->messageTableName(), 'msg') ->fields('msg'); - $count = 1; - foreach ($source_id_values as $id_value) { - $query->condition('sourceid' . $count++, $id_value); + foreach ($this->sourceIdFields() as $field_name => $source_id) { + if (isset($source_id_values[$field_name])) { + $query->condition($source_id, $source_id_values[$field_name]); + } } + if ($level) { $query->condition('level', $level); } @@ -655,13 +662,11 @@ public function delete(array $source_id_values, $messages_only = FALSE) { $map_query = $this->getDatabase()->delete($this->mapTableName()); } $message_query = $this->getDatabase()->delete($this->messageTableName()); - $count = 1; - foreach ($source_id_values as $id_value) { + foreach ($this->sourceIdFields() as $field_name => $source_id) { if (!$messages_only) { - $map_query->condition('sourceid' . $count, $id_value); + $map_query->condition($source_id, $source_id_values[$field_name]); } - $message_query->condition('sourceid' . $count, $id_value); - $count++; + $message_query->condition($source_id, $source_id_values[$field_name]); } if (!$messages_only) { @@ -675,22 +680,20 @@ public function delete(array $source_id_values, $messages_only = FALSE) { /** * {@inheritdoc} */ - public function deleteDestination(array $destination_id) { + public function deleteDestination(array $destination_id_values) { $map_query = $this->getDatabase()->delete($this->mapTableName()); $message_query = $this->getDatabase()->delete($this->messageTableName()); - $source_id = $this->lookupSourceID($destination_id); - if (!empty($source_id)) { - $count = 1; - foreach ($destination_id as $key_value) { - $map_query->condition('destid' . $count, $key_value); - $count++; + $source_id_values = $this->lookupSourceID($destination_id_values); + if (!empty($source_id_values)) { + foreach ($this->destinationIdFields() as $field_name => $destination_id) { + $map_query->condition($destination_id, $destination_id_values[$field_name]); } // Notify anyone listening of the map row we're about to delete. - $this->eventDispatcher->dispatch(MigrateEvents::MAP_DELETE, new MigrateMapDeleteEvent($this, $source_id)); + $this->eventDispatcher->dispatch(MigrateEvents::MAP_DELETE, new MigrateMapDeleteEvent($this, $source_id_values)); $map_query->execute(); $count = 1; - foreach ($source_id as $key_value) { - $message_query->condition('sourceid' . $count, $key_value); + foreach ($this->sourceIdFields() as $field_name => $source_id) { + $message_query->condition($source_id, $source_id_values[$field_name]); $count++; } $message_query->execute(); @@ -700,54 +703,18 @@ public function deleteDestination(array $destination_id) { /** * {@inheritdoc} */ - public function setUpdate(array $source_id) { - if (empty($source_id)) { + public function setUpdate(array $source_id_values) { + if (empty($source_id_values)) { throw new MigrateException('No source identifiers provided to update.'); } $query = $this->getDatabase() ->update($this->mapTableName()) ->fields(array('source_row_status' => MigrateIdMapInterface::STATUS_NEEDS_UPDATE)); - $count = 1; - foreach ($source_id as $key_value) { - $query->condition('sourceid' . $count++, $key_value); - } - $query->execute(); - } - /** - * {@inheritdoc} - */ - public function deleteBulk(array $source_id_values) { - // If we have a single-column key, we can shortcut it. - if (count($this->migration->getSourcePlugin()->getIds()) == 1) { - $sourceids = array(); - foreach ($source_id_values as $source_id) { - // Notify anyone listening of the map rows we're about to delete. - $this->eventDispatcher->dispatch(MigrateEvents::MAP_DELETE, new MigrateMapDeleteEvent($this, $source_id)); - $sourceids[] = $source_id; - } - $this->getDatabase()->delete($this->mapTableName()) - ->condition('sourceid1', $sourceids, 'IN') - ->execute(); - $this->getDatabase()->delete($this->messageTableName()) - ->condition('sourceid1', $sourceids, 'IN') - ->execute(); - } - else { - foreach ($source_id_values as $source_id) { - // Notify anyone listening of the map rows we're deleting. - $this->eventDispatcher->dispatch(MigrateEvents::MAP_DELETE, new MigrateMapDeleteEvent($this, $source_id)); - $map_query = $this->getDatabase()->delete($this->mapTableName()); - $message_query = $this->getDatabase()->delete($this->messageTableName()); - $count = 1; - foreach ($source_id as $key_value) { - $map_query->condition('sourceid' . $count, $key_value); - $message_query->condition('sourceid' . $count++, $key_value); - } - $map_query->execute(); - $message_query->execute(); - } + foreach ($this->sourceIdFields() as $field_name => $source_id) { + $query->condition($source_id, $source_id_values[$field_name]); } + $query->execute(); } /** @@ -811,8 +778,8 @@ public function key() { public function currentDestination() { if ($this->valid()) { $result = array(); - foreach ($this->destinationIdFields() as $field_name) { - $result[$field_name] = $this->currentRow[$field_name]; + foreach ($this->destinationIdFields() as $destination_field_name => $idmap_field_name) { + $result[$destination_field_name] = $this->currentRow[$idmap_field_name]; } return $result; } diff --git a/core/modules/migrate/src/Plugin/migrate/process/Migration.php b/core/modules/migrate/src/Plugin/migrate/process/Migration.php index a23128015152..a60e773abd79 100644 --- a/core/modules/migrate/src/Plugin/migrate/process/Migration.php +++ b/core/modules/migrate/src/Plugin/migrate/process/Migration.php @@ -114,6 +114,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable $destination_plugin = $migration->getDestinationPlugin(TRUE); // Only keep the process necessary to produce the destination ID. $process = $migration->get('process'); + // We already have the source id values but need to key them for the Row // constructor. $source_ids = $migration->getSourcePlugin()->getIds(); diff --git a/core/modules/migrate/src/Tests/MigrateRollbackTest.php b/core/modules/migrate/src/Tests/MigrateRollbackTest.php index ce6eb0051e42..302425012c2b 100644 --- a/core/modules/migrate/src/Tests/MigrateRollbackTest.php +++ b/core/modules/migrate/src/Tests/MigrateRollbackTest.php @@ -75,7 +75,7 @@ public function testRollback() { /** @var Vocabulary $vocabulary */ $vocabulary = Vocabulary::load($row['id']); $this->assertTrue($vocabulary); - $map_row = $vocabulary_id_map->getRowBySource([$row['id']]); + $map_row = $vocabulary_id_map->getRowBySource(['id' => $row['id']]); $this->assertNotNull($map_row['destid1']); } @@ -116,7 +116,7 @@ public function testRollback() { /** @var Term $term */ $term = Term::load($row['id']); $this->assertTrue($term); - $map_row = $term_id_map->getRowBySource([$row['id']]); + $map_row = $term_id_map->getRowBySource(['id' => $row['id']]); $this->assertNotNull($map_row['destid1']); } @@ -125,14 +125,14 @@ public function testRollback() { foreach ($term_data_rows as $row) { $term = Term::load($row['id']); $this->assertNull($term); - $map_row = $term_id_map->getRowBySource([$row['id']]); + $map_row = $term_id_map->getRowBySource(['id' => $row['id']]); $this->assertFalse($map_row); } $vocabulary_executable->rollback(); foreach ($vocabulary_data_rows as $row) { $term = Vocabulary::load($row['id']); $this->assertNull($term); - $map_row = $vocabulary_id_map->getRowBySource([$row['id']]); + $map_row = $vocabulary_id_map->getRowBySource(['id' => $row['id']]); $this->assertFalse($map_row); } diff --git a/core/modules/migrate/src/Tests/MigrateSkipRowTest.php b/core/modules/migrate/src/Tests/MigrateSkipRowTest.php index 4bff07f268aa..d27a9aedb191 100644 --- a/core/modules/migrate/src/Tests/MigrateSkipRowTest.php +++ b/core/modules/migrate/src/Tests/MigrateSkipRowTest.php @@ -63,10 +63,10 @@ public function testPrepareRowSkip() { $id_map_plugin = $migration->getIdMap(); // The first row is recorded in the map as ignored. - $map_row = $id_map_plugin->getRowBySource([1]); + $map_row = $id_map_plugin->getRowBySource(['id' => 1]); $this->assertEqual(MigrateIdMapInterface::STATUS_IGNORED, $map_row['source_row_status']); // The second row is not recorded in the map. - $map_row = $id_map_plugin->getRowBySource([2]); + $map_row = $id_map_plugin->getRowBySource(['id' => 2]); $this->assertFalse($map_row); } diff --git a/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php index 3460ca3e1ab2..0b61f30fa834 100644 --- a/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php @@ -191,7 +191,7 @@ public function testClearMessages() { // Insert 4 message for later delete. foreach ($expected_results as $key => $expected_result) { - $id_map->saveMessage([$key], $message); + $id_map->saveMessage(['source_id_property' => $key], $message); } // Truncate and check that 4 messages were deleted. @@ -240,7 +240,7 @@ public function testGetRowsNeedingUpdate() { // Assert the row matches its original source. $source_id = $expected_results[MigrateIdMapInterface::STATUS_NEEDS_UPDATE]['sourceid1']; - $test_row = $id_map->getRowBySource([$source_id]); + $test_row = $id_map->getRowBySource(['source_id_property' => $source_id]); // $row_needing_update is an array of objects returned from the database, // but $test_row is an array, so the cast is necessary. $this->assertSame($test_row, (array) $row_needing_update[0]); @@ -268,7 +268,7 @@ public function testMessageCount() { foreach ($expected_results as $key => $expected_result) { $count = $id_map->messageCount(); $this->assertEquals($expected_result, $count); - $id_map->saveMessage([$key], $message); + $id_map->saveMessage(['source_id_property' => $key], $message); } } @@ -286,7 +286,7 @@ public function testMessageSave() { $id_map = $this->getIdMap(); foreach ($expected_results as $key => $expected_result) { - $id_map->saveMessage([$key], $message, $expected_result['level']); + $id_map->saveMessage(['source_id_property' => $key], $message, $expected_result['level']); } foreach ($id_map->getMessageIterator() as $message_row) { @@ -297,8 +297,8 @@ public function testMessageSave() { // Insert with default level. $message_default = 'Hello world default.'; - $id_map->saveMessage([5], $message_default); - $messages = $id_map->getMessageIterator([5]); + $id_map->saveMessage(['source_id_property' => 5], $message_default); + $messages = $id_map->getMessageIterator(['source_id_property' => 5]); $count = 0; foreach ($messages as $key => $message_row) { $count = 1; @@ -334,11 +334,11 @@ public function testGetRowBySource() { 'destid1' => 'destination_id_value_2', ] + $this->idMapDefaults(); $this->saveMap($row); - $source_id_values = [$row['sourceid1'], $row['sourceid2']]; + $source_id_values = ['source_id_property' => $row['sourceid1'], 'sourceid2' => $row['sourceid2']]; $id_map = $this->getIdMap(); $result_row = $id_map->getRowBySource($source_id_values); $this->assertSame($row, $result_row); - $source_id_values = ['missing_value_1', 'missing_value_2']; + $source_id_values = ['source_id_property' => 'missing_value_1', 'sourceid2' => 'missing_value_2']; $result_row = $id_map->getRowBySource($source_id_values); $this->assertFalse($result_row); } @@ -416,12 +416,12 @@ public function testGetRowByDestination() { 'destid1' => 'destination_id_value_2', ] + $this->idMapDefaults(); $this->saveMap($row); - $dest_id_values = [$row['destid1']]; + $dest_id_values = ['destination_id_property' => $row['destid1']]; $id_map = $this->getIdMap(); $result_row = $id_map->getRowByDestination($dest_id_values); $this->assertSame($row, $result_row); // This value does not exist. - $dest_id_values = ['invalid_destination_id_property']; + $dest_id_values = ['destination_id_property' => 'invalid_destination_id_property']; $id_map = $this->getIdMap(); $result_row = $id_map->getRowByDestination($dest_id_values); $this->assertFalse($result_row); @@ -463,15 +463,15 @@ public function testLookupSourceIDMapping($num_source_fields, $num_destination_f $expected_result = []; for ($i = 1; $i <= $num_source_fields; $i++) { $row["sourceid$i"] = "source_id_value_$i"; - $expected_result[] = "source_id_value_$i"; + $expected_result["source_id_property_$i"] = "source_id_value_$i"; $this->sourceIds["source_id_property_$i"] = []; } $destination_id_values = []; $nonexistent_id_values = []; for ($i = 1; $i <= $num_destination_fields; $i++) { $row["destid$i"] = "destination_id_value_$i"; - $destination_id_values[] = "destination_id_value_$i"; - $nonexistent_id_values[] = "nonexistent_destination_id_value_$i"; + $destination_id_values["destination_id_property_$i"] = "destination_id_value_$i"; + $nonexistent_id_values["destination_id_property_$i"] = "nonexistent_destination_id_value_$i"; $this->destinationIds["destination_id_property_$i"] = []; } $this->saveMap($row); @@ -662,7 +662,7 @@ public function testSetUpdate() { $this->queryResultTest($this->getIdMapContents(), $expected_results); // Mark each row as STATUS_NEEDS_UPDATE. foreach ($row_statuses as $status) { - $id_map->setUpdate(['source_value_' . $status]); + $id_map->setUpdate(['source_id_property' => 'source_value_' . $status]); } // Update expected results. foreach ($expected_results as $key => $value) { @@ -737,7 +737,7 @@ public function testDestroy() { $message_table_name = $id_map->messageTableName(); $row = new Row(['source_id_property' => 'source_value'], ['source_id_property' => []]); $id_map->saveIdMapping($row, ['destination_id_property' => 2]); - $id_map->saveMessage(['source_value'], 'A message'); + $id_map->saveMessage(['source_id_property' => 'source_value'], 'A message'); $this->assertTrue($this->database->schema()->tableExists($map_table_name), "$map_table_name exists"); $this->assertTrue($this->database->schema()->tableExists($message_table_name), -- GitLab