Commit f72ef077 authored by webchick's avatar webchick

Issue #2522012 by phenaproxima, mikeryan, chx, benjy: Remove broken idlist...

Issue #2522012 by phenaproxima, mikeryan, chx, benjy: Remove broken idlist handling, replace with more robust exception handling
parent 63a6753b
......@@ -24,8 +24,6 @@ class ActionTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_action',
),
......
......@@ -20,7 +20,6 @@ class AggregatorFeedTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
'id' => 'test',
'idlist' => array(),
'source' => array(
'plugin' => 'd6_aggregator_feed',
),
......
......@@ -22,8 +22,6 @@ class AggregatorItemTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_aggregator_item',
),
......
......@@ -21,7 +21,6 @@ class BlockedIpsTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = [
'id' => 'test',
'idlist' => [],
'source' => [
'plugin' => 'd7_blocked_ips',
],
......
......@@ -25,7 +25,6 @@ class BlockTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test',
'idlist' => array(),
'source' => array(
'plugin' => 'd6_block',
),
......
......@@ -24,8 +24,6 @@ class BoxTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_boxes',
),
......
......@@ -22,8 +22,6 @@ abstract class CommentTestBase extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test',
// Leave it empty for now.
'idlist' => array(),
// This needs to be the identifier of the actual key: cid for comment, nid
// for node and so on.
'source' => array(
......
......@@ -43,6 +43,7 @@ public function query() {
*/
public function prepareRow(Row $row) {
$row->setSourceProperty('recipients', explode(',', $row->getSourceProperty('recipients')));
return parent::prepareRow($row);
}
/**
......
......@@ -20,7 +20,6 @@ class ContactCategoryTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
'id' => 'test',
'idlist' => array(),
'source' => array(
'plugin' => 'd6_contact_category',
),
......
......@@ -24,8 +24,6 @@ class FieldInstancePerViewModeTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'view_mode_test',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_field_instance_per_view_mode',
),
......
......@@ -24,8 +24,6 @@ class FieldInstanceTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = [
// The id of the entity, can be any string.
'id' => 'test_fieldinstance',
// Leave it empty for now.
'idlist' => [],
'source' => [
'plugin' => 'd6_field_instance',
],
......
......@@ -24,8 +24,6 @@ class FieldTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The id of the entity, can be any string.
'id' => 'test_field',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_field',
),
......
......@@ -47,6 +47,7 @@ public function prepareRow(Row $row) {
->orderBy('u.weight');
$query->innerJoin('node', 'n', static::JOIN);
$row->setSourceProperty('upload', $query->execute()->fetchAll());
return parent::prepareRow($row);
}
/**
......
......@@ -71,6 +71,7 @@ public function prepareRow(Row $row) {
// the source_base_path in order to make them all relative.
$path = str_replace($this->migration->get('destination.source_base_path'), NULL, $path);
$row->setSourceProperty('filepath', $path);
return parent::prepareRow($row);
}
/**
......
......@@ -22,8 +22,6 @@ class FileTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_file',
),
......
......@@ -21,7 +21,6 @@ class FilterFormatTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
'id' => 'test',
'highWaterProperty' => array('field' => 'test'),
'idlist' => array(),
'source' => array(
'plugin' => 'd6_filter_formats',
),
......
......@@ -24,8 +24,6 @@ class MenuLinkSourceTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'mlid',
// Leave it empty for now.
'idlist' => array(),
// This needs to be the identifier of the actual key: cid for comment, nid
// for node and so on.
'source' => array(
......
......@@ -12,4 +12,35 @@
*/
class MigrateSkipRowException extends \Exception {
/**
* Whether to record the skip in the map table, or skip silently.
*
* @var bool
* TRUE to record as STATUS_IGNORED in the map, FALSE to skip silently.
*/
protected $saveToMap;
/**
* Constructs a MigrateSkipRowException object.
*
* @param string $message
* The message for the exception.
* @param bool $save_to_map
* TRUE to record as STATUS_IGNORED in the map, FALSE to skip silently.
*/
public function __construct($message = NULL, $save_to_map = TRUE) {
parent::__construct($message);
$this->saveToMap = $save_to_map;
}
/**
* Whether the thrower wants to record this skip in the map table.
*
* @return bool
* TRUE to record as STATUS_IGNORED in the map, FALSE to skip silently.
*/
public function getSaveToMap() {
return $this->saveToMap;
}
}
......@@ -753,8 +753,6 @@ public function destroy() {
* Implementation of Iterator::rewind().
*
* This is called before beginning a foreach loop.
*
* @todo Support idlist, itemlimit.
*/
public function rewind() {
$this->currentRow = NULL;
......@@ -765,13 +763,6 @@ public function rewind() {
foreach ($this->destinationIdFields() as $field) {
$fields[] = $field;
}
// @todo Make this work.
/*
if (isset($this->options['itemlimit'])) {
$query = $query->range(0, $this->options['itemlimit']);
}
*/
$this->result = $this->getDatabase()->select($this->mapTableName(), 'map')
->fields('map', $fields)
->execute();
......@@ -823,7 +814,6 @@ public function next() {
* and FALSE to terminate it.
*/
public function valid() {
// @todo Check numProcessed against itemlimit.
return $this->currentRow !== FALSE;
}
......
......@@ -10,6 +10,8 @@
use Drupal\Core\Plugin\PluginBase;
use Drupal\migrate\Entity\MigrationInterface;
use Drupal\migrate\MigrateException;
use Drupal\migrate\MigrateExecutableInterface;
use Drupal\migrate\MigrateSkipRowException;
use Drupal\migrate\Plugin\MigrateIdMapInterface;
use Drupal\migrate\Plugin\MigrateSourceInterface;
use Drupal\migrate\Row;
......@@ -69,13 +71,6 @@ abstract class SourcePluginBase extends PluginBase implements MigrateSourceInter
*/
protected $originalHighWater;
/**
* List of source IDs to process.
*
* @var array
*/
protected $idList = array();
/**
* Whether this instance should cache the source count.
*
......@@ -130,8 +125,12 @@ abstract class SourcePluginBase extends PluginBase implements MigrateSourceInter
*/
protected $iterator;
// @TODO, find out how to remove this.
// @see https://www.drupal.org/node/2443617
/**
* @TODO, find out how to remove this.
* @see https://www.drupal.org/node/2443617
*
* @var MigrateExecutableInterface
*/
public $migrateExecutable;
/**
......@@ -152,10 +151,6 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
$this->originalHighWater = $this->migration->getHighWater();
}
if ($id_list = $this->migration->get('idlist')) {
$this->idList = $id_list;
}
// Don't allow the use of both highwater and track changes together.
if ($this->highWaterProperty && $this->trackChanges) {
throw new MigrateException('You should either use a highwater mark or track changes not both. They are both designed to solve the same problem');
......@@ -187,21 +182,31 @@ protected function getModuleHandler() {
* {@inheritdoc}
*/
public function prepareRow(Row $row) {
$result = TRUE;
$result_hook = $this->getModuleHandler()->invokeAll('migrate_prepare_row', array($row, $this, $this->migration));
$result_named_hook = $this->getModuleHandler()->invokeAll('migrate_' . $this->migration->id() . '_prepare_row', array($row, $this, $this->migration));
try {
$result_hook = $this->getModuleHandler()->invokeAll('migrate_prepare_row', array($row, $this, $this->migration));
$result_named_hook = $this->getModuleHandler()->invokeAll('migrate_' . $this->migration->id() . '_prepare_row', array($row, $this, $this->migration));
// We will skip if any hook returned FALSE.
$skip = ($result_hook && in_array(FALSE, $result_hook)) || ($result_named_hook && in_array(FALSE, $result_named_hook));
$save_to_map = TRUE;
}
catch (MigrateSkipRowException $e) {
$skip = TRUE;
$save_to_map = $e->getSaveToMap();
}
// We're explicitly skipping this row - keep track in the map table.
if (($result_hook && in_array(FALSE, $result_hook)) || ($result_named_hook && in_array(FALSE, $result_named_hook))) {
if ($skip) {
// Make sure we replace any previous messages for this item with any
// new ones.
$id_map = $this->migration->getIdMap();
$id_map->delete($this->currentSourceIds, TRUE);
$this->migrateExecutable->saveQueuedMessages();
$id_map->saveIdMapping($row, array(), MigrateIdMapInterface::STATUS_IGNORED, $this->migrateExecutable->rollbackAction);
$this->currentRow = NULL;
$this->currentSourceIds = NULL;
if ($save_to_map) {
$id_map->saveIdMapping($row, array(), MigrateIdMapInterface::STATUS_IGNORED, $this->migrateExecutable->rollbackAction);
$this->currentRow = NULL;
$this->currentSourceIds = NULL;
}
$result = FALSE;
}
elseif ($this->trackChanges) {
......@@ -302,25 +307,17 @@ public function next() {
$row->setIdMap($id_map);
}
// In case we have specified an ID list, but the ID given by the source is
// not in there, we skip the row.
$id_in_the_list = $this->idList && in_array(reset($this->currentSourceIds), $this->idList);
if ($this->idList && !$id_in_the_list) {
continue;
}
// Preparing the row gives source plugins the chance to skip.
if ($this->prepareRow($row) === FALSE) {
continue;
}
// Check whether the row needs processing.
// 1. Explicitly specified IDs.
// 2. This row has not been imported yet.
// 3. Explicitly set to update.
// 4. The row is newer than the current highwater mark.
// 5. If no such property exists then try by checking the hash of the row.
if ($id_in_the_list || !$row->getIdMap() || $row->needsUpdate() || $this->aboveHighwater($row) || $this->rowChanged($row) ) {
// 1. This row has not been imported yet.
// 2. Explicitly set to update.
// 3. The row is newer than the current highwater mark.
// 4. If no such property exists then try by checking the hash of the row.
if (!$row->getIdMap() || $row->needsUpdate() || $this->aboveHighwater($row) || $this->rowChanged($row) ) {
$this->currentRow = $row->freezeSource();
}
}
......
......@@ -111,76 +111,69 @@ protected function initializeIterator() {
$this->prepareQuery();
$high_water_property = $this->migration->get('highWaterProperty');
// Get the key values, for potential use in joining to the map table, or
// enforcing idlist.
// Get the key values, for potential use in joining to the map table.
$keys = array();
// The rules for determining what conditions to add to the query are as
// follows (applying first applicable rule)
// 1. If idlist is provided, then only process items in that list (AND key
// IN (idlist)). Only applicable with single-value keys.
if ($id_list = $this->migration->get('idlist')) {
$this->query->condition($keys[0], $id_list, 'IN');
}
else {
// 2. If the map is joinable, join it. We will want to accept all rows
// which are either not in the map, or marked in the map as NEEDS_UPDATE.
// Note that if high water fields are in play, we want to accept all rows
// above the high water mark in addition to those selected by the map
// conditions, so we need to OR them together (but AND with any existing
// conditions in the query). So, ultimately the SQL condition will look
// like (original conditions) AND (map IS NULL OR map needs update
// OR above high water).
$conditions = $this->query->orConditionGroup();
$condition_added = FALSE;
if (empty($this->configuration['ignore_map']) && $this->mapJoinable()) {
// Build the join to the map table. Because the source key could have
// multiple fields, we need to build things up.
$count = 1;
$map_join = '';
$delimiter = '';
foreach ($this->getIds() as $field_name => $field_schema) {
if (isset($field_schema['alias'])) {
$field_name = $field_schema['alias'] . '.' . $field_name;
}
$map_join .= "$delimiter$field_name = map.sourceid" . $count++;
$delimiter = ' AND ';
// 1. If the map is joinable, join it. We will want to accept all rows
// which are either not in the map, or marked in the map as NEEDS_UPDATE.
// Note that if high water fields are in play, we want to accept all rows
// above the high water mark in addition to those selected by the map
// conditions, so we need to OR them together (but AND with any existing
// conditions in the query). So, ultimately the SQL condition will look
// like (original conditions) AND (map IS NULL OR map needs update
// OR above high water).
$conditions = $this->query->orConditionGroup();
$condition_added = FALSE;
if (empty($this->configuration['ignore_map']) && $this->mapJoinable()) {
// Build the join to the map table. Because the source key could have
// multiple fields, we need to build things up.
$count = 1;
$map_join = '';
$delimiter = '';
foreach ($this->getIds() as $field_name => $field_schema) {
if (isset($field_schema['alias'])) {
$field_name = $field_schema['alias'] . '.' . $field_name;
}
$map_join .= "$delimiter$field_name = map.sourceid" . $count++;
$delimiter = ' AND ';
}
$alias = $this->query->leftJoin($this->migration->getIdMap()->getQualifiedMapTableName(), 'map', $map_join);
$conditions->isNull($alias . '.sourceid1');
$conditions->condition($alias . '.source_row_status', MigrateIdMapInterface::STATUS_NEEDS_UPDATE);
$condition_added = TRUE;
$alias = $this->query->leftJoin($this->migration->getIdMap()->getQualifiedMapTableName(), 'map', $map_join);
$conditions->isNull($alias . '.sourceid1');
$conditions->condition($alias . '.source_row_status', MigrateIdMapInterface::STATUS_NEEDS_UPDATE);
$condition_added = TRUE;
// And as long as we have the map table, add its data to the row.
$n = count($this->getIds());
// And as long as we have the map table, add its data to the row.
$n = count($this->getIds());
for ($count = 1; $count <= $n; $count++) {
$map_key = 'sourceid' . $count;
$this->query->addField($alias, $map_key, "migrate_map_$map_key");
}
if ($n = count($this->migration->get('destinationIds'))) {
for ($count = 1; $count <= $n; $count++) {
$map_key = 'sourceid' . $count;
$map_key = 'destid' . $count++;
$this->query->addField($alias, $map_key, "migrate_map_$map_key");
}
if ($n = count($this->migration->get('destinationIds'))) {
for ($count = 1; $count <= $n; $count++) {
$map_key = 'destid' . $count++;
$this->query->addField($alias, $map_key, "migrate_map_$map_key");
}
}
$this->query->addField($alias, 'source_row_status', 'migrate_map_source_row_status');
}
// 3. If we are using high water marks, also include rows above the mark.
// But, include all rows if the high water mark is not set.
if (isset($high_water_property['name']) && ($high_water = $this->migration->getHighWater()) !== '') {
if (isset($high_water_property['alias'])) {
$high_water = $high_water_property['alias'] . '.' . $high_water_property['name'];
}
else {
$high_water = $high_water_property['name'];
}
$conditions->condition($high_water, $high_water, '>');
$condition_added = TRUE;
$this->query->addField($alias, 'source_row_status', 'migrate_map_source_row_status');
}
// 2. If we are using high water marks, also include rows above the mark.
// But, include all rows if the high water mark is not set.
if (isset($high_water_property['name']) && ($high_water = $this->migration->getHighWater()) !== '') {
if (isset($high_water_property['alias'])) {
$high_water = $high_water_property['alias'] . '.' . $high_water_property['name'];
}
if ($condition_added) {
$this->query->condition($conditions);
else {
$high_water = $high_water_property['name'];
}
$conditions->condition($high_water, $high_water, '>');
$condition_added = TRUE;
}
if ($condition_added) {
$this->query->condition($conditions);
}
return new \IteratorIterator($this->query->execute());
......
<?php
/**
* @file
* Contains \Drupal\migrate\Tests\MigrateSkipRowTest.
*/
namespace Drupal\migrate\Tests;
use Drupal\migrate\Entity\Migration;
use Drupal\migrate\MigrateMessage;
use Drupal\migrate\Entity\MigrationInterface;
use Drupal\migrate\MigrateExecutable;
use Drupal\migrate\Plugin\MigrateIdMapInterface;
use Drupal\simpletest\KernelTestBase;
/**
* Tests row skips triggered during hook_migrate_prepare_row().
*
* @group migrate
*/
class MigrateSkipRowTest extends KernelTestBase {
/**
* Modules to enable.
*
* @var array
*/
public static $modules = ['migrate', 'migrate_prepare_row_test'];
/**
* Tests migration interruptions.
*/
public function testPrepareRowSkip() {
// Run a simple little migration with two data rows which should be skipped
// in different ways.
$config = [
'id' => 'sample_data',
'migration_tags' => ['prepare_row test'],
'source' => [
'plugin' => 'embedded_data',
'data_rows' => [
['id' => '1', 'data' => 'skip_and_record'],
['id' => '2', 'data' => 'skip_and_dont_record']
],
'ids' => [
'id' => ['type' => 'string'],
],
],
'process' => ['value' => 'data'],
'destination' => [
'plugin' => 'config',
'config_name' => 'migrate_test.settings',
],
'load' => ['plugin' => 'null'],
];
$migration = Migration::create($config);
$executable = new MigrateExecutable($migration, new MigrateMessage);
$result = $executable->import();
$this->assertEqual($result, MigrationInterface::RESULT_COMPLETED);
$id_map_plugin = $migration->getIdMap();
// The first row is recorded in the map as ignored.
$map_row = $id_map_plugin->getRowBySource([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]);
$this->assertFalse($map_row);
}
}
name: 'Migrate module prepareRow tests'
type: module
description: 'Support module for source plugin prepareRow testing.'
package: Testing
version: VERSION
core: 8.x
<?php
/**
* @file
* Test module for testing the migration source plugin prepareRow() exception
* handling.
*/
use Drupal\migrate\Entity\MigrationInterface;
use Drupal\migrate\MigrateSkipRowException;
use Drupal\migrate\Plugin\MigrateSourceInterface;
use Drupal\migrate\Row;
/**
* Implements hook_migrate_prepare_row().
*/
function migrate_prepare_row_test_migrate_prepare_row(Row $row, MigrateSourceInterface $source, MigrationInterface $migration) {
// Test both options for save_to_map.
$data = $row->getSourceProperty('data');
if ($data == 'skip_and_record') {
throw new MigrateSkipRowException('', TRUE);
}
elseif ($data == 'skip_and_dont_record') {
throw new MigrateSkipRowException('', FALSE);
}
}
......@@ -171,16 +171,6 @@ public function testPrepareRowFalse() {
$this->assertNull($source->current(), 'No row is available when prepareRow() is false.');
}
/**
* Test that the when a source id is in the idList, we don't get a row.
*/
public function testIdInList() {
$source = $this->getSource([], ['idlist' => ['test_sourceid1']]);
$source->rewind();
$this->assertNull($source->current(), 'No row is available because id was in idList.');
}
/**
* Test that $row->needsUpdate() works as expected.
*/
......
......@@ -21,7 +21,6 @@ abstract class VariableMultiRowTestBase extends MigrateSqlSourceTestCase {
// The fake Migration configuration entity.
protected $migrationConfiguration = array(
'id' => 'test',
'idlist' => array(),
'source' => array(
'plugin' => 'd6_variable_multirow',
'variables' => array(
......
......@@ -21,7 +21,6 @@ class VariableTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
'id' => 'test',
'highWaterProperty' => array('field' => 'test'),
'idlist' => array(),
'source' => array(
'plugin' => 'd6_variable',
'variables' => array(
......
......@@ -21,8 +21,6 @@ class NodeByNodeTypeTest extends MigrateSqlSourceTestCase {
// The fake Migration configuration entity.
protected $migrationConfiguration = array(
'id' => 'test',
// Leave it empty for now.
'idlist' => array(),
// The fake configuration for the source.
'source' => array(
'plugin' => 'd6_node',
......
......@@ -21,8 +21,6 @@ class NodeRevisionByNodeTypeTest extends MigrateSqlSourceTestCase {
// The fake Migration configuration entity.
protected $migrationConfiguration = [
'id' => 'test',
// Leave it empty for now.
'idlist' => [],
// The fake configuration for the source.
'source' => [
'plugin' => 'd6_node_revision',
......
......@@ -21,8 +21,6 @@ class NodeRevisionTest extends MigrateSqlSourceTestCase {
// The fake Migration configuration entity.
protected $migrationConfiguration = [
'id' => 'test',
// Leave it empty for now.
'idlist' => [],
// The fake configuration for the source.
'source' => [
'plugin' => 'd6_node_revision',
......
......@@ -24,8 +24,6 @@ class NodeTypeTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test_nodetypes',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_nodetype',
),
......
......@@ -24,8 +24,6 @@ class ViewModeTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'view_mode_test',
// Leave it empty for now.
'idlist' => array(),
'source' => array(
'plugin' => 'd6_field_instance_view_mode',
),
......
......@@ -21,7 +21,6 @@ class UrlAliasTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
'id' => 'test',
'highWaterProperty' => array('field' => 'test'),
'idlist' => array(),
'source' => array(
'plugin' => 'd6_url_alias',
),
......
......@@ -24,8 +24,6 @@ class MenuTest extends MigrateSqlSourceTestCase {
protected $migrationConfiguration = array(
// The ID of the entity, can be any string.
'id' => 'test',
// Leave it empty for now.
'idlist' => array(),
// This needs to be the identifier of the actual key: cid for comment, nid