The d6_node plugin should fetch CCK field values

re-roll.

I was going to change assertSame to assertIdentical and possibly add more tests. However, I'm not able to reach any breakpoint in the test. How do I run this test?

RIght, just learned that PHPUnit doesn't have assertIdentical.

assertSame() in phpunit is the same as assertIdentical() in simpletest.

Added some tests

As long as we can migrate nodes without the the CCK module we can combine those. Looks like it's been handled by the moduleExists() call?

For now, this functionality should be in core alongside the existing CCK handling, which will be removed when the builder plugin type lands.

What is meant by this in the IS?

+++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
@@ -115,10 +122,122 @@ public function prepareRow(Row $row) {
+    return @($this->fieldInfo[$node_type] ?: []);

What's the @ for?

The ?: operator will produce an E_NOTICE if $this->fieldInfo[$node_type] is undefined, so the @ is there to silence that.

I feared as much, I don't think this is a pattern we use in core.

Few small things:

1. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -115,10 +122,122 @@ public function prepareRow(Row $row) {
   +        // @TODO Massage the field values so that field_foo_value becomes
   +        // just 'value', and so forth.
   

   Does this need an issue?

2. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -115,10 +122,122 @@ public function prepareRow(Row $row) {
   +  /**
   ...
   +  protected function getFieldInfo($node_type) {
   

   Missing functiond oc.

3. +++ b/core/modules/node/tests/src/Unit/Plugin/migrate/source/d6/NodeTest.php
   @@ -108,6 +108,42 @@ class NodeTest extends MigrateSqlSourceTestCase {
   +        'global_settings' => 'a:6:{s:6:"prefix";s:3:"id-";s:6:"suffix";s:0:"";s:3:"min";s:3:"100";s:3:"max";s:3:"200";s:14:"allowed_values";s:0:"";s:18:"allowed_values_php";s:0:"";}',
   ...
   +        'db_columns' => 'a:1:{s:5:"value";a:3:{s:4:"type";s:5:"float";s:8:"not null";b:0;s:8:"sortable";b:1;}}',
   ...
   +        'widget_settings' => 'a:2:{s:13:"default_value";a:1:{i:0;a:2:{s:5:"value";s:3:"101";s:14:"_error_element";s:47:"default_value_widget][field_test_four][0][value";}}s:17:"default_value_php";N;}',
   +        'display_settings' => 'a:7:{s:6:"weight";s:1:"3";s:6:"parent";s:0:"";i:5;a:2:{s:6:"format";s:7:"default";s:7:"exclude";i:0;}s:5:"label";a:1:{s:6:"format";s:5:"above";}s:6:"teaser";a:2:{s:6:"format";s:7:"default";s:7:"exclude";i:0;}s:4:"full";a:2:{s:6:"format";s:7:"default";s:7:"exclude";i:0;}i:4;a:2:{s:6:"format";s:7:"default";s:7:"exclude";i:0;}}',
   

   Might be worth expanding these

So, this is dying due to memory consumption:

Memory usage is 272.14 MB (85% of limit 320 MB), reclaiming memory	
Memory usage is now 272.11 MB (85% of limit 320 MB), not enough reclaimed, starting new batch

In my local environment, also with a memory_limit of 320M, it works fine. Two obvious possibilities:

1. The patch is causing causing excessive memory usage (a loop repeatedly migrating stuff?).
2. The memory usage for entities in general has gone beyond the threshold where we can import the full D6 dump in one process.

I've submitted #2281393: D6->D8 Blocks - Custom titles not imported for retest as a control - if it succeeds, that suggests #1, if it fails it suggests #2. If it's #2, that raises the urgency of #2503999: Large volume entity migrations run out of memory.

Well #2281393: D6->D8 Blocks - Custom titles not imported failed, but with an entirely different error - MigrateDrupal6Test passed, no memory problem, which suggests that the patch here is somehow triggering more memory consumption. But, as I said, it works locally with the same memory_limit...

This is blocking #2549013: Remove load plugins which in turn blocks Comment/Taxonomy content migrations for D7 => D8.

When we added builders they were meant to be alongside load plugins, I don't see why we *have* to remove load plugins to do comment/taxonomy, the existing API should work to complete those D7/D8 migrations we still have LoadTermNode although as an example.

It's a shame we did the builders and attempted to remove load plugins in 8.0 that has taken a long time and held up many of the D7 migrations.

phenaproxima can speak to the specifics of how the load plugins are blocking the D7 migrations, but my understanding was always that the intention of builders was to get rid of the load plugins. The genesis of the builders was #2507607: [META] Replace Cthulhu-forsaken load plugins with migration builders, after all.

Sure, the plan is to get rid of load plugins but that's more work and if time is short we could not do that. builders transform the migrations before they're run, load plugins can still do that at run time today so they are both able to solve the term/cck problems in their own way. However, if there is time to get builders done and remove load plugins great.

1. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -115,10 +122,131 @@ public function prepareRow(Row $row) {
   +    if ($this->moduleExists('content') && $this->getModuleSchemaVersion('content') >= 6001) {
   

   Might be good to document the magic 6001 schema version here (presumably CCK changed its schema in such a way we don't want to try to support the previous version?).

2. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -115,10 +122,131 @@ public function prepareRow(Row $row) {
   +            if (strpos($column, $field) === 0) {
   ...
   +      foreach (array_keys($field['db_columns']) as $column) {
   

   This seems problematic if there's a field whose name is the prefix for another field name - this is an issue for migrate_d2d as well (#2078913: D5 to D7 migration fails if one source CCK field is a substring of another). See below...

3. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -115,10 +122,131 @@ public function prepareRow(Row $row) {
   +    $db = $this->getDatabase()->schema();
   

   $db sounds like it's referencing a database - perhaps $db_schema to be clear?

4. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -115,10 +122,131 @@ public function prepareRow(Row $row) {
   +        $query->addField('t', $field['field_name'] . '_' . $column);
   

   Can we use '__' as the separator here, and use that above to recognize when a column is prefixed by the right field?

It does help to have the CCK removal in here, so we can see that the node CCK code didn't come out of thin air, the logic has moved (and it also helps to see the loc-- benefit).

I've tested this with my own site using migrate_upgrade - my fields migrated successfully within the node migrations, without those separate field-only migrations, that's great! Code looks good - I'm ready to see this go in!

 9 files changed, 176 insertions(+), 728 deletions(-)

Hot damn!

Read through the non-deleted parts of this patch, nothing much stood out.

Asked a few minor questions in IRC, got them answered, this looks good to go, and unblocks all kinds of other stuff, so let's get 'er done!

Committed and pushed to 8.0.x. Thanks!

Awesome!

For the first ever my tiny d6 site, 5 nodes and some CCK fields, actually migrated. Previously, the fields would be created but not the value. Today I have values! And for a file field too!

For me, it feels like I can do some serious testing now.

Wow, that's awesome!! :D Great news, quietone, and thanks so much for all your help!

Seems I spoke to soon. File uploads are working but not file fields.