Some source plugins produce duplicate rows

Had some discussion with @phenaproxima, who has dug into this area before. As I understand it, in Drupal 6 the widget_type affected the actual database representation, which was fine when it was creating separate tables, but in Drupal 8 the (well, some) distinctions between field types that D6 had at the widget level are now in the base field. So, we actually do need to account for the D6 widget type in creating the D8 field. The problem is if a D6 site used different widget types in different instances of the same field - there's only going to be one field in D8, so we can't apply both widget types. Right now, if I'm not mistaken, the last instance processed takes precedence.

So, the first pass at this, my plan is to take the instance join out of the base query and pull the "first" widget_type in prepareRow() - this fixes the isComplete() problem without making handling conflicts between instances any worse. I'll also detect the situation of conflicting instances and generate a message, so (unlike today) the migrator will get some clue that things aren't 100% hunky-dory. @phenaproxima suggests maybe trying to normalize conflicts to a lower-common-denominator field type - that may be best left to a follow-up issue. The question is, how many real-world D6 sites have instance conflicts that matter?

I presume this issue must have been dealt with in the D6 CCK->D7 field upgrade path, we should investigate that.

A fail patch to demonstrate the issues.

Yep, exactly the expected failures.

widget_settings isn't actually needed, only widget_type is used to determine the ultimate field type, I was removing it in my patch.

The advantage of pulling the widget_type from the instance in prepareRow(), rather than selecting an arbitrary type in the query, is that it allows us to warn the user that something's missing.

Removing widget_settings, which enables some simplification of the query.

After discussion with phenaproxima, I'm going to go with pulling the widget_type in prepareRow(), so we can produce a message when we detect a conflict in instance widget_types.

Here it is... The one thing it's lacking is a test for the message coming from conflicting widget_types, because in the current data the instances that were generating the extra source count differed only in widget_settings, they had the same widget_type. So, another instance needs to be added to the source data to be able to test the message...

So, the existing D6 dump has two instances of field_test (on the story and test_page node types), both with widget_type text_textfield. To test dealing with instances of one field with different widget_types, I created another instance on the test_planet node type, with widget_type optionwidgets_onoff. My test of this functionality succeeded - yay! But, now MigrateFieldInstanceTest fails when testing story's instance of field_test. That's because optionwidgets_onoff comes before text_textfield alphabetically, so that the base field_test becomes a boolean field - not at all what this test is expecting.

The problem here is a basic WTF of migrating D6 CCK to either D7 or D8 - why, here's what the D7 content_migrate module tells you if you have this situation:

        $field_value['messages'][] = '<span class="error">' . t("Caution: The '@field' field is a shared field that uses different widgets. The '@widget' widget is sometimes used to determine the destination field type for the new field. The migration may not work correctly if other widgets used by this shared field would create different results.", array('@field' => $field_value['field_name'], '@widget' => $row['widget_type'])) .'</span>';

I.e., if you used different widget types on different instances of a field on D6, you're fucked. Frankly, if you used a text widget for one instance of a field and a radio button/checkbox for another, you deserve it - the saving grace is that this is probably (I hope) a very rare condition.

Anyway, since MigrateFieldInstanceTest was not designed to be a demonstration of this problem, I need to use different test data that doesn't overlap with instances explicitly used by existing tests.

Finally... Also tweaked the message with feedback from phenaproxima.

Oh, of course, need to add another field table to the dump... Bah.

Or, I should say, add field columns to the node field table. Enough for me for today, I'll pick it up in the morning (unless someone else wants to jump in...)

Ugh, without a dump that could be used to run a real D6 site so I can manipulate the fields through the UI and have all the DB tables end up in the right place, testing this is damned near impossible. Maybe #2562695: migrate-db.sh skips uid 1 but shouldn't will rescue me?

With much struggling, managed to get the dumps files in the right place (I think).

Sure - let's just wait and make sure the test results don't require any other changes.

It caught me trying to hack the dump files to minimize the changes... Fine, whatever...

For want of a space, the test was lost...

Is that how we created the new dump file?

Ah, I vaguely recall the sandbox... I created a D6 environment of my own with the necessary contrib modules and managed to get it into good enough shape to add the instance and export.

Although performance probably isn't an issue with the field migration...

Yeah, I think it's clearer to pull them directly here. Overriding initializeIterator() would be an unusual pattern and a "wait, what?" for future reviewers.

Why an assert? Do we have docs on when to use these somewhere?

Taking advantage of https://www.drupal.org/node/2569701 - not sure how aggressively we're supposed to use them, couldn't resist the shiny new feature;).

Given a general policy on asserts has an issue at #2548671: [policy, no patch] Define best practices for using and testing assertions and document them before adding assertions to core, I don't think we need a migrate-specific policy (at least unless/until that policy is in place and we decide we should make exceptions).