Update cck and field migration plugin

Tests only patch. Hopefully this shows that tests here are failing. Which justifies a critical priority.

OK, this should show a test failure.

Are we testing against core 8.5.x HEAD? I cannot tell. When I ran the tests locally against 8.5.x HEAD, they failed for the patch in #6.

Functionally applies and the tests speak for themselves. Definitely confusing to the use the term "cck" (a very D6 term) in regards to a very D7 module, "addressfield." I can only surmise this is because we're dealing with ubercart or we're defining something that can cross both D6 and D7 paths.

Just be aware, in D6 world "addressfield" is meaningless.

I'm not familiar with the migrate code, so I don't want to commit any changes.

Overall the code looks good to me, but there are a few flaws:

--- a/src/Plugin/migrate/cckfield/AddressField.php
+++ b/src/Plugin/migrate/cckfield/AddressField.php
@@ -13,8 +13,10 @@ use Drupal\migrate_drupal\Plugin\MigrateCckFieldInterface;
* id = "addressfield",
* core = {7},
* type_map = {
- * "addressfield" = "address"
- * }
+ * "addressfield" = "address",
+ * },
+ * source_module = "addressfield",
+ * destination_module = "address"

You have add a comma to this line:

* "addressfield" = "address"

That is a mistake.

@@ -15,6 +15,8 @@ use Drupal\migrate\Plugin\MigrationInterface;
class AddressFieldTest extends KernelTestBase {

/**
+ * Modules to enable.
+ *
* @var array
*/
public static $modules = [
@@ -28,9 +30,10 @@ class AddressFieldTest extends KernelTestBase {
*/
public function testPlugin() {
$migration = $this->prophesize(MigrationInterface::class)->reveal();
- $field_plugin = $this->container
- ->get('plugin.manager.migrate.field')
- ->createInstance('addressfield', [], $migration);
+ $field_plugin_manager = $this->container
+ ->get('plugin.manager.migrate.field');
+ $field_plugin_manager->getDefinition('addressfield');
+ $field_plugin = $field_plugin_manager->createInstance('addressfield', [], $migration);
$this->assertInstanceOf(AddressField::class, $field_plugin);
}

In the above code you have added

$field_plugin_manager->getDefinition('addressfield');

As far as I can tell, this doesn't do anything and is not used for anything. If the call to getDefinition is done to test that some internal things are not broken, you should add a comment about what you are doing here, otherwise as I read the code you can simply delete this line. The same is done in the other test case.

#15 points are all addressed in this patch. The trailing annotation comma was out of scope code cleanup. I've added an assert not empty on the definition. We are testing that the plugin discovery process is possible and we don't throw any exceptions. But let's assert not empty too to make it more clear. Discovery would throw an exception well before it is ever empty, but this is more obvious.

Committed, thanks!