Dummy map/message tables being created

While running test migrations for #2695297: Refactor EntityFile and use process plugins instead and working on #2603124: Sql::getRowBySource doesn't adhere to MigrateIdMapInterface::getRowBySource I noticed an over abundance of these dummy files, >700. Both of those are 8.2.x issues. So, I did some testing to see if this is just happening in 8.2.x.

Starting with a clean install of 8.1.x, I ran MigrateUpgrade6Test. That completed successfully with 9 dummy files. And then I switched to 8.2.x. Starting with a clean install, I ran MigrateUpgrade6Test. I didn't let it finish, I aborted the test when there were >400 dummy files.

Is this happening for anyone else, or is it just me?

This can quickly explode into very very very many tables very quickly. In my case, I've tracked it down to MigrationDeriverTrait::getSourcePlugin, which uses stub migrations to access source plugins. This gets called for migration derivers, e.g. D7NodeDeriver, on every definition cache clear.

I don't know if we really need to take this roundabout way of getting the source instance (probably not). We might also consider whether the fully functional SQL idMap is necessary for stub migrations (it probably is), or if we can at least check whether the migration seems legit (e.g. not null as the destination plugin).

As a workaround, I've added a custom null idMap plugin and use that in the stub definition. I've attached this approach as a patch. I am currently unsure whether this is the whole issue, or just a single situation when things go awry.

Setting to needs review. May the testing commence ;)

That seems like a pretty good fix and given it's green i'm presuming the map is not used with the fake source. Can we add a test for this?

I get a WSOD with this installed and trying to run a migration from /upgrade. It occurs after entering db credentials.

[Wed Oct 12 15:34:31.131866 2016] [:error] [pid 28620] [client 10.1.1.2:56685] PHP Fatal error:  Cannot use 'Null' as class name as it is reserved in /opt/sites/d8/core/modules/migrate/src/Plugin/migrate/id_map/Null.php on line 20, referer: http://d8.dev2.matai.vs/upgrade

Yeah that became reserved in PHP7, we can just rename the class to something like NullIdMap

Oh, good to know about the reserved name! I've changed the class name to the suggested NullIdMap.
However, I don't know how to go about writing tests, or even which cases we should test.

After a little more thinking, I would love if the SQL tables were initialized only when the migration is actually run. As far as I can tell that is the way migrate worked in Drupal 7. This would also prevent mapping and messages tables being created for migrations whose requirements are not met (e.g. D6 nodes when migrating from a D7 legacy system).

Does anyone have an idea if this approach is reasonable with the way migrate & Co. currently work?

I believe $this->init() is only called when something actually uses the database or the map table name?

Yes, it is. However, the database gets used frequently, e.g. whenever status information is requested, such as processed counts. This happens in migrate_tools drush ms my_migration_name or its migrations overview. I am currently trying to delay this initialization as long as possible but will open a new issue for that.

While it seems sensible, I've noticed that source and id_map plugins are still very much intertwined. So I'll stick to the NullIdMap solution for this issue for now. :)

Thx, ckaotik. I just ran a migration using d7_dump with the patch in #11 and didn't get WSOD or a fatal. There was one 'dummy' map and message table.

| menu_tree                                              |
| migrate_map_57ff429e55e18                              |
| migrate_map_block_content_body_field 

Which migrations did you run, @quietone? Do you use D6, D7 or something else as source system?

@ckaotik, I used db-tools.php to import the d7_dump test fixture to the db. I did a fresh install of D8 and then ran the full migration, from the UI.

If you haven't seen it already, Generating database fixtures for D8 Migrate tests has a little bit more info on working with the dumps.

That looks useful, indeed ;) Thanks for the link!

All right, I think I found the issue. migrate_drupal uses similar code for derived vocabularies. So do some source plugins from the user module. I've added these to the patch.

@ckaotik, your welcome.

I ran it again and still get one map/message table pair. The map has 3 sourceids, Both d7_field_instance and d7_block have 3 source ids. Hope that helps.

Hrm, I've searched and couldn't find where this originates from. The only place that seems sensible is from the user migration (it queries d7_field_instance), which I've adjusted with the previous patch. Could this be a leftover from your previous test run? If so, the table name would still be migrate_map_57ff429e55e18 .
If it's not leftover, some debugging output in Drupal\migrate\Plugin\migrate\id_map\Sql::ensureTables() might lead to the source. As I don't have a test system at hand, I can't really check right now :/

On a different topic, though: Do we need to adjust the existing tests to use the null map? Or can these be left unchanged?

I've also updated the patch to make sure the basic ArrayIterator is used.

No, not a leftover. Did a llittle debugging and the dummy table is created when the source plugin is d7_field_instance and the entity_type is user.

Thanks for checking! It was indeed the User migration.
The issue was just caused by a typo introduced in comment 18, when I used id_map instead of idMap. I've updated the patch accordingly (sorry, I have no interdiff at hand).

It would be great if you could verify that it finally works, @quietone :)

Just ran two migrations from the UI, one using d6_dump as the source and the other using d7_dump as the source. I am pleased to report that there are no dummy tables. Yea!

Just noticed one small thing.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/NullIdMap.php
@@ -0,0 +1,243 @@
+  public function getQualifiedMapTableName() {
...
+   */

Missing doc block for method.

I'm working on this..

Added @inheritdoc as mentioned in #26.

Changing to Needs review for testing.

Forgot this still needs tests, back to needs work.

Update IS

Here is a test and a fail patch. I don't like that it relies on MigrateDrupal6TestBase. I reckon it can be done by creating the tables needed, but I started with what I know. To take this further will someone provide some direction, example etc. Thanks.

No interdiff since the change is solely the addition of the test.

A namespace error in the test file. Let's try again.

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/NullIdMap.php
@@ -0,0 +1,246 @@
+   * @inheritdoc
...
+   * @inheritdoc

Nit: code comment standards has {} around these.

Is this at all related to #2817833: Delay sql map table creation?

Fixed doc blocks.

I don't think is related to the other issue. That one is about where in the flow the tables are created. This is only about the dummy tables created by derivers. As explained by ckaotik in #6.

This can quickly explode into very very very many tables very quickly. In my case, I've tracked it down to MigrationDeriverTrait::getSourcePlugin, which uses stub migrations to access source plugins. This gets called for migration derivers, e.g. D7NodeDeriver, on every definition cache clear.

This looks good. Only one nit:

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/NullIdMap.php
@@ -0,0 +1,246 @@
+  public function getRowBySource(array $source_id_values) {
+   return [];
+  }

Line indented incorrectly.

This is not enough to go back to NW, this can be fixed at commit, right?

1. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/NullIdMap.php
   @@ -0,0 +1,246 @@
   +class NullIdMap extends PluginBase implements MigrateIdMapInterface, ContainerFactoryPluginInterface {
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   +    return new static($configuration, $plugin_id, $plugin_definition);
   +  }
   

   Do we need to implement ContainerFactoryPluginInterface if we're not injecting anything?

   I don't think we do.

2. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/NullIdMap.php
   @@ -0,0 +1,246 @@
   +  /**
   +   * Implementation of Iterator::rewind().
   +   *
   +   * This is called before beginning a foreach loop.
   +   */
   ...
   +  /**
   +   * Implementation of Iterator::current().
   +   *
   +   * This is called when entering a loop iteration, returning the current row.
   +   */
   ...
   +  /**
   +   * Implementation of Iterator::key().
   +   *
   +   * This is called when entering a loop iteration, returning the key of the
   +   * current row. It must be a scalar - we will serialize to fulfill the
   +   * requirement, but using getCurrentKey() is preferable.
   ...
   +  /**
   +   * Implementation of Iterator::next().
   +   *
   +   * This is called at the bottom of the loop implicitly, as well as explicitly
   +   * from rewind().
   +   */
   ...
   +  /**
   +   * Implementation of Iterator::valid().
   +   *
   +   * This is called at the top of the loop, returning TRUE to process the loop
   +   * and FALSE to terminate it.
   +   */
   

   I think these can all use inheritdoc

3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/IdMapTableNoDummyTest.php
   @@ -0,0 +1,40 @@
   +    $this->assertSame(0, count($dummy_tables));
   

   We can use assertCount here

Can we fix #41 at the same time?

Thanks.

This looks like an annoying problem and an elegant solution - nice.

Fixes for #41 and #42.

Feedback addressed

Crediting @mikeryan for proposing the solution in #2

+++ b/core/modules/migrate/src/Plugin/migrate/id_map/NullIdMap.php
@@ -0,0 +1,225 @@
+use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
...
+use Symfony\Component\DependencyInjection\ContainerInterface;

nit: No longer used - sorry :(

updating issue credits to include myself for #42

Thanks for the review. Unused lines removed.

Committed 694b95b and pushed to 8.5.x - thanks.