Migration of nodes with cck nodereferrer fields fails (SQL error)

I'm not sure if this was an out-and-out bug in nodereferrer, or if this was a practice in other CCK fields... At any rate, bulletproofing wouldn't hurt:

if (empty($columns)) {
  return [];
}

I am going to work on it in Drupal Dev Days Seville

Sorry, but finally I didn't work in this issue. I was working in translation of the User Guide to Spanish.

Adding comment#4

Adding comment#4

Looks good, but we add a test for this - include a test-only patch which should fail, and a full patch which should succeed.

I took a crack at this to try to get it moving. The patch changed slightly after applying and generating due to #2833206: Convert Migrate's cckfield plugins to use the new field plugins, but the patch in #9 still applies okay to 8.4.x.

I added test source_data for nodereferrer for this, but let me know if it's in the wrong place. My local test run of NodeTest failed with "Undefined offset: 0", and the NodeTest passed after applying the full patch attached here. An interdiff against #9 is also attached.

I guess I attached the patches in #13 in the wrong order. Changing the status back to Needs Review since the test failure was expected. Also clearing the Needs tests tag.

Assigning for review.

This looks good, I don't see anything wrong. The test only patch fails as expected. Thanks RytoEX!

1. +++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
   @@ -270,6 +270,9 @@ protected function getFieldData(array $field, Row $node) {
   +      if (empty($columns)) {
   +        return [];
   +      }
   

   This needs an explanation of why we're just returning empty and not logging anything.

2. +++ b/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d6/NodeTest.php
   @@ -38,6 +38,18 @@ public function providerSource() {
          ],
   +      [
   +        'field_name' => 'field_test_node_referrer',
   +        'type' => 'nodereferrer',
   +        'global_settings' => 'a:6:{s:14:"referrer_types";a:10:{s:7:"article";i:0;s:7:"company";i:0;s:8:"employee";i:0;s:5:"forum";i:0;s:10:"test_event";i:0;s:9:"test_page";i:0;s:11:"test_planet";i:0;s:10:"test_story";i:0;s:7:"sponsor";i:0;s:5:"story";i:0;}s:15:"referrer_fields";a:1:{s:13:"field_company";i:0;}s:23:"referrer_nodes_per_page";s:1:"0";s:22:"referrer_pager_element";s:1:"0";s:14:"referrer_order";s:9:"TITLE_ASC";s:21:"referrer_translations";i:0;}',
   +        'required' => '0',
   +        'multiple' => '0',
   +        'db_storage' => '1',
   +        'module' => 'nodereferrer',
   +        'db_columns' => 'a:0:{}',
   +        'active' => '1',
   +        'locked' => '0',
   +      ],
   

   Without reading the issue summary I got confused by nodereferrer vs. nodereference. Could we use something more explanatory like 'missing_cck_module'?

1. Added comment.
2. Simplified the new test data, removing all references to nodereferrer.

+++ b/core/modules/node/src/Plugin/migrate/source/d6/Node.php
@@ -270,6 +270,10 @@ protected function getFieldData(array $field, Row $node) {
       $columns = array_keys($field['db_columns']);
+      if (empty($columns)) {
+        // If the field db_columns is empty then there are no values to return.
+        return [];
+      }

I don't find this very clear. I don't think 'db_columns' is a field. Looking at the code I think this snippet is when there's no columns to fetch from a field table.

Also, there's no mention about why we are returning nothing and not logging something.

Closed #3196867: Pseudo-fields without "db columns", like noderelationships field_backref_* break node migration" as a duplicate. Adding credit for those who helped over there.

Updated IS.

Updated the patch and made a fail patch.

Closed #3062062: Ignore nodereferrer field data in D6 Migration as a duplicate, adding credit.

Great work @all and especially @quiteone!

As we've been using this fix for months now, I'll set this RTBC! :)

Committed and pushed 81174ad778 to 9.3.x and 89d028223d to 9.2.x. Thanks!