D6 text fields that have "Number of values:" set to anything other than 1 do not import.

When I did this with an integer text field, this is what the results were:

Running d6_node_type                                                                                                                                                                   [ok]
Running d6_filter_format                                                                                                                                                               [ok]
Running d6_field_settings                                                                                                                                                              [ok]
Running d6_field                                                                                                                                                                       [ok]
Running d6_field_instance                                                                                                                                                              [ok]
Running d6_field_instance_widget_settings                                                                                                                                              [ok]
Running d6_view_modes                                                                                                                                                                  [ok]
Running d6_field_formatter_settings                                                                                                                                                    [ok]
Running d6_node_settings                                                                                                                                                               [ok]
Running d6_node                                                                                                                                                                        [ok]
Running d6_node_revision                                                                                                                                                               [ok]
Running d6_cck_field_values:image                                                                                                                                                      [ok]
Running d6_cck_field_values:page                                                                                                                                                       [ok]
Running d6_cck_field_values:story                                                                                                                                                      [ok]
Migration failed with source plugin exception: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your [error]
MariaDB server version for the right syntax to use near 'FROM 
content_type_story f
WHERE  (vid = '3')' at line 1: SELECT 
FROM 
{content_type_story} f
WHERE  (vid = :db_condition_placeholder_0) ; Array
(
    [:db_condition_placeholder_0] => 3
)

Running d6_cck_field_revision:image                                                                                                                                                    [ok]
Migration d6_cck_field_revision:image did not meet the requirements                                                                                                                    [error]
Running d6_cck_field_revision:page                                                                                                                                                     [ok]
Migration d6_cck_field_revision:page did not meet the requirements                                                                                                                     [error]
Running d6_cck_field_revision:story                                                                                                                                                    [ok]
Migration d6_cck_field_revision:story did not meet the requirements                                                                                                                    [error]

When I did this with an textfield text field, this is what the results were:

Running d6_node_type                                                                                                                                                                   [ok]
Running d6_filter_format                                                                                                                                                               [ok]
Running d6_field_settings                                                                                                                                                              [ok]
Running d6_field                                                                                                                                                                       [ok]
Running d6_field_instance                                                                                                                                                              [ok]
Running d6_field_instance_widget_settings                                                                                                                                              [ok]
Running d6_view_modes                                                                                                                                                                  [ok]
Running d6_field_formatter_settings                                                                                                                                                    [ok]
Running d6_node_settings                                                                                                                                                               [ok]
Running d6_node                                                                                                                                                                        [ok]
Running d6_node_revision                                                                                                                                                               [ok]
Running d6_cck_field_values:page                                                                                                                                                       [ok]
Running d6_cck_field_values:story                                                                                                                                                      [ok]
Migration failed with source plugin exception: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your [error]
MariaDB server version for the right syntax to use near 'FROM 
content_type_story f
WHERE  (vid = '1')' at line 1: SELECT 
FROM 
{content_type_story} f
WHERE  (vid = :db_condition_placeholder_0) ; Array
(
    [:db_condition_placeholder_0] => 1
)

Running d6_cck_field_revision:page                                                                                                                                                     [ok]
Migration d6_cck_field_revision:page did not meet the requirements                                                                                                                     [error]
Running d6_cck_field_revision:story                                                                                                                                                    [ok]
Migration d6_cck_field_revision:story did not meet the requirements                                                                                                                    [error]

In both cases, the field was migrated and "Allowed number of values" was set the correct value, but neither had any of the entered content migrated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ultimike’s picture

I was able to reproduce this issue (where the field values aren't getting migrated), but not with the same error output that was reported above:

D6 source setup

  • Added a multi-valued integer field (text field widget) to the "story" content type, set number of values to 4.
  • Created a new "story" node with 4 values in the new integer field.

D8 results

  • Field configuration was migrated fine.
  • Node was migrated, but all 4 values of the multi-valued field were empty.

oadaeh and I started looking at updating the MigrateCckFieldValuesTest with a similar configuration that fails, we made a little progress, and oadaeh will continue to work this issue in Austin.

Note - we should also update the node_load in testCckFields in the MigrateCckFieldValuesTest class to Node::load(1) (and "use Drupal\node\Entity\Node;").

-mike

ultimike’s picture

I took a shot at updating the MigrateCckFieldValuesTest class with a new test field that fails.

-mike

oadaeh’s picture

Assigned: Unassigned » oadaeh

I discovered why adding to, changing, and/or removing from the database dump wasn't changing anything. It's because the dump isn't being included in the test. I'm working on changing that to see if I can get that part fixed, and then I'll move on from there.

ultimike’s picture

@oadaeh,

Any progress on this? Ping me on IRC and I'll help you get this moving in the right direction again.

Thanks,
-mike

oadaeh’s picture

Hello Mike.

In my mind, I thought I had updated this issue since my last update.

I got the test to work with a database dump, but I had to create a new one that would work for this test, as using the existing one caused integrity errors (duplicate tables and/or records were created). However, once I had it all working there, I still had the problem from before, where the test would pass, regardless of the values in the dump. So, I think there's something wrong with either that test set or with Simpletest itself.

Unfortunately, that's as far as I got before I had to get back on track with paid client work. I will try to at least get a patch in this issue to correct the data dump part this week, and hopefully make some progress on the tests themselves some time next week.

oadaeh’s picture

Here's the patch I've been working on.

There is a major problem with the test, in that I have no idea where it's getting it's data to test. It's not using the dump file, and there's nothing in the set up that adds data to the fields.

I've been going over this test and other related tests to see how they are configured, and I cannot figure out where the problem lies.

To see what I mean, after applying the patch, simply comment out the part that loads the data dump. The test will pass just fine, w/o putting any data in the database to pull from.

oadaeh’s picture

Status: Active » Needs review

By the way, it's basically the same problem as before, except now the test is configured to load a data dump, whereas before it was not.

The last submitted patch, 2: 2279323-multivaluedtextfields-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2279323-6-multi-value-fields.patch, failed testing.

oadaeh’s picture

That's interesting. It wasn't doing that on my computer, but I remember running into it in Austin, and the fix was to change the data import to start with id 2 or 3, further convincing me that it's loading data from some other source. I will look at that tomorrow or Saturday.

oadaeh’s picture

Status: Needs work » Needs review
FileSize
6.13 KB

I see that I set all the nids and vids to 1 in my data dump. I'm still not sure why the test passed before, but when I tried them this morning, they failed, and starting with a nid/vid of 1 gave me the same error, so this patch has them starting at 2.

oadaeh’s picture

I've commented out the part that loads the test data to see what the testbot thinks of the test.

oadaeh’s picture

For this patch, I have re-enabled the data load, but I set the values that it loads to be incorrect.

oadaeh’s picture

I believe the last two patches should have had failed tests, the first for a lack of data and the second for incorrect data.

As they did not, my assumption is that there are one or more problems with one of these items:

  1. The setup of the tests
  2. The tests themselves
  3. Simpletest

Since to a degree, 2 and 3 kind of imply each other, and since I think it would be well known if Simpletest were not working, and since my understanding of setting up and creating tests is limited, I'm going to assume the problem lies with item 1.

However, the test set up follows the same format as many of the tests already in (at least) Migrate, so if this one is broken, probably many of the others are as well.

Anyone may feel free to correct me, as I'm not exactly sure in which direction to go from here.

ultimike’s picture

oadaeh,

Ok - I just spent a few minutes refreshing my mind on this issue, let's you and I take this step-by-step.

We actually don't need to include the Drupal6Node.php dump file in MigrateCckFieldValuesTest because this test class actually extends MigrateNodeTestBase:

class MigrateCckFieldValuesTest extends MigrateNodeTestBase {

MigrateNodeTestBase loads 3 dump files:

      $this->getDumpDirectory() . '/Drupal6Node.php',
      $this->getDumpDirectory() . '/Drupal6NodeType.php',
      $this->getDumpDirectory() . '/Drupal6FieldInstance.php',

So, I don't think we need to create a new dump file, we can just edit Drupal6FieldInstance.php with (hopefully) data that causes the test to fail.

Looking at Drupal6FieldInstance.php, it looks like the "widget_module" and "widget_enabled" fields were left out of the dump for the content_node_field_instance table. I'm attaching a patch that adds these 2 fields and the appropriate data for the test fields. This doesn't solve anything yet, but gets us going in the right direction. I'll keep working this issue. The first task needs to be figuring out why the current dump data doesn't fail, and then modify/add to it to get a failing test.

Thanks,
-mike

ultimike’s picture

Status: Needs review » Needs work
ultimike’s picture

Hmm - I just did a manual test (see details below) with some success on an up-to-date version of D8 with no patches(!)

Multi-valued integer field (with text field widget)

  • Configuration migrated properly.
  • Content in nodes migrated properly.

Multi-valued text field (with text field widget)

  • Configuration migrated properly.
  • Content in nodes migrated properly.

Multi-valued decimal field (with text field widget)

  • Configuration migrated properly.
  • Content in nodes migrated properly.

@odaeah - will you redo your manual tests to see if you see the same thing? Perhaps there was a patch committed in the past few days that fixed this?

Regardless, I think the test improvements in patch 15 are probably a good idea to improve the test.

Thanks,
-mike

oadaeh’s picture

I just updated to the latest code in 8.x-dev and ran the tests. None of my field data was migrated, and I still get this error:

Migration failed with source plugin exception: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your [error]
MariaDB server version for the right syntax to use near 'FROM 
content_type_story f
WHERE  (vid = '1')' at line 1: SELECT 
FROM 
{content_type_story} f
WHERE  (vid = :db_condition_placeholder_0) ; Array
(
    [:db_condition_placeholder_0] => 1
)

Regarding your comment in #15, I was initially working with the Drupal6FieldInstance.php, but I couldn't seem to make any progress, and I thought adding the other dump file would help, but obviously, it did not, and now I know why.

ultimike’s picture

@oadaeh (Jason),

Thanks for sending me a copy of your D6 DB. Unfortunately, I got the exact same error as you did in comment 18. Let's try a sanity check first. Would you mind creating a fresh D6 site with just one or two multi-valued fields and run the migration again?

Thanks,
-mike

oadaeh’s picture

I spent a few hours last night and a few hours this morning testing and distilling this. I continue to get that error, and it seems to be reliably reproducible (at least on my computer).

Here are the final steps I took this morning in as few steps as possible to reproduce this:

  1. I created a new database for a new installation of both Drupal 6 and Drupal 8
  2. In a new, empty directory, I downloaded a new copy of Drupal 6 (drush dl drupal-6), which gave me drupal-6.31
  3. In the same directory, I downloaded a new copy of Drupal 8 (drush dl --dev drupal-8), which gave me drupal-8.x-dev (Normally, I would use Git for D8, but in this case, I wanted to make it as simple and easy to reproduce.)
  4. I installed Drupal 6 (drush -y si --db-url=mysql://$username:$password@$host:$port/$d6_database_name --account-name=$uid1name --account-pass=$uid1pass)
  5. I downloaded the latest version of CCK for Drupal 6 (6.x-2.9) (drush dl cck) and enabled the Content and Text modules (drush -y en text)
  6. I modified the Story content type by adding a single Text field with a Text widget, changing only the Number of values property to 2
  7. I created a Story content and only set the title (However, adding content to the text field does not change the situation.)
  8. I installed Drupal 8 (drush -y si --db-url=mysql://$username:$password@$host:$port/$d8_database_name --account-name=$uid1name --account-pass=$uid1pass)
  9. I enabled the Migrate and Migrate Drupal modules (drush en migrate_drupal -y)
  10. I created a manifest file (listed below)
  11. I ran a content migration (drush migrate-manifest manifest --db-url=mysql://$username:$password@$host:$port/$d6_database_name)

Here is my manifest file:

- d6_node
- d6_node_revision
- d6_node_type
- d6_view_modes
- d6_filter_format
- d6_field_instance_per_form_display
- d6_field_instance_widget_settings
- d6_field_formatter_settings
- d6_field_instance
- d6_field
- d6_field_settings
- d6_node_settings
- d6_cck_field_values:*
- d6_cck_field_revision:*

(I also created three scripts to automate as much of that as possible, leaving only adding the field and creating the content to manual operation.)

The content type and content get migrated, but the content of the field does not.

oadaeh’s picture

I maybe should also add that I'm now getting errors with revisions on the migration that I did not get last week with the same configuration:

...
Running d6_cck_field_revision:page                             [ok]
Migration d6_cck_field_revision:page did not meet the requirements                                     [error]
Running d6_cck_field_revision:story                            [ok]
Migration d6_cck_field_revision:story did not meet the requirements                                    [error]
ultimike’s picture

@oadaeh,

I just duplicated your setup (comment 20) and ran the migration.

First off, don't worry about "Migration d6_cck_field_revision:page did not meet the requirements" error message. This is just letting you know that the node(s) that you migrated didn't have any revisions, so the revision migrations didn't have anything to migrate and throw this error. Misleading, I know.

I received the same error you reported in comment 18 (on the "d6_cck_field_values:story" migration), I'm going to turn on my debugger and start digging into that now. Also, similarly, while the multi-valued text field did migrate successfully, the values did not.

Thanks,
-mike

ultimike’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
1.18 KB

Ok, I think I made some progress on this issue.

I believe the issue that we've been seeing occurs when the only field attached to a content type is a multi-valued field (this explains why it was difficult to reproduce sometimes). When this is the case, then the $query->execute() around line 108 of Drupal\migrate_drupal\Plugin\migrate\source\d6\CckFieldValues->prepareRow() is called, but without any single-valued fields defined resulting in the error @oadaeh and I were running into (see comment 18).

I've attached a patch that wraps the $query->execute() with an if-statement that checks to see if any single-valued fields have been added to the query.

I've tested it with several different content types with various combinations of multi-valued and non-multivalued fields with good results.

@oadaeh - please apply this patch and give it a whirl.

Thanks,
-mike

ultimike’s picture

ultimike’s picture

Whoops - I mistakenly attached the same patch twice in comment 23 - sorry!

-mike

oadaeh’s picture

Sorry about taking so long to get back to this. My life has been rather random lately.

I tested the patch, and it does apply cleanly and does appear to fix the problem with migrating fields with multiple values. I did not test it on every possible combination of multivalue field. I will try to do that later.

Running the test in the UI passes. I'm not sure if it's still correct or not, considering it was passing before. Maybe we need more test suites, rather than just more tests.

Running the test on the CLI fails with this error:

PHP Fatal error:  Call to undefined method Drupal\migrate_drupal\Tests\d6\MigrateCckFieldValuesTest::getInfo() in /home/jason/.composer/vendor/drush/drush/commands/core/test.drush.inc on line 201

Here is how I ran it:

drush test-run "Drupal\migrate_drupal\Tests\d6\MigrateCckFieldValuesTest" --uri=http://localhost/migrate/8.x/
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/CckFieldValues.php
@@ -105,12 +105,14 @@ public function prepareRow(Row $row) {
+          // We diff the results because the extra will be all the field columns.

The comment is now one character too long. Also, I think the comment is not as clear as it could be.

ultimike’s picture

FileSize
1.22 KB
928 bytes

@oadaeh,

Thanks for taking a look at the patch.

I've updated the patch based on your comment about the comment.

I never run tests using "drush test-run". For example, I ran the test for this patch using:

php core/scripts/run-tests.sh --url 'http://drupal8.dev/' --class 'Drupal\migrate_drupal\Tests\d6\MigrateCckFieldValuesTest'

Testing for this particular patch might require a brand new test - basically a clone of MigrateCckFieldValuesTest, but with a single multi-valued field (instead of the three fields currently used in the test). I'm not convinced this is necessary, but I'll defer to benjy and/or chx on whether or not it is necessary.

Thanks,
-mike

chx’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks for catching and patching this.

Given if (count($query->getFields())) why do we need $new_fields = array_diff($results, $source);? Why do we add fields to the query if we are not using them?

And also, please add a test. Thanks!

ultimike’s picture

FileSize
9.81 KB

chx,

I've struggled for way too many hours trying to make some headway on a failing test, and while I've made progress, I've been banging my head for a couple of hours now.

I'm attaching a patch that adds 2 failing tests by adding a new "field_multivalue" decimal field to the existing "test_page" content type that via the dump files listed in MigrateNodeTestBase.php.

The problem is that the CckFieldValues migration isn't running for the test_page content type and I can't figure out why (so the tests I added are failing for all the wrong reasons). Can you apply the patch and see if you can see why I'm doing wrong?

I know that the CckFieldValues migration isn't running for the "test_page" content type because I can put a stop at this foreach (in MigrateCckFieldValuesTest) and it isn't listed as one of the migrations to run ("story" and "test_event" are):

    $migrations = entity_load_multiple('migration', array('d6_cck_field_values:*'));
    foreach ($migrations as $migration) {
      $executable = new MigrateExecutable($migration, $this);
      $executable->import();
    }

Thanks,
-mike

chx’s picture

Good lord, what a mess what we created. What follows is HEAD, not this patch.

Drupal6NodeType adds to Drupal 6 test_page, test_story, test_event and ... story.

Migrate6NodeTestBase emulates the d6_node_type migration already ran for test_story.

LoadEntity::loadMultiple runs $bundle_migration->getSourcePlugin()->getIterator() which only returns migrations not yet ran. In this case, this means that the sub_ids will be story and test_event.

This is why your test is not picking up page: because you added test_page to the migrations already ran. You need to add that id mapping after the entity_load but before import.

benjy’s picture

Status: Needs work » Needs review
FileSize
416 bytes
12.39 KB
12.33 KB

To get around #31 I introduced a new content type. Patch attached is a combination of the work from #27 and #29.

The interdiff, is a pseudo diff of the two patches, so you can easily see the fix.

Status: Needs review » Needs work

The last submitted patch, 31: 2279323-31-PASS.patch, failed testing.

The last submitted patch, 31: 2279323-31-FAIL.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
13.09 KB
785 bytes

Fix for MigrateNodeBundleSettingsTest

benjy’s picture

Assigned: oadaeh » Unassigned
chx’s picture

Status: Needs review » Needs work

I am very sorry but

      // If we have fields on the query then add them to the source.
      if (count($query->getFields())) {

this is a classic example of a totally useless comment. Yes the code needs a comment but I can see quite well what (count($query->getFields())) does but what is the cause of not having fields?? I read the issue and the explanation is this:

I believe the issue that we've been seeing occurs when the only field attached to a content type is a multi-valued field (this explains why it was difficult to reproduce sometimes). When this is the case, then the $query->execute() around line 108 of Drupal\migrate_drupal\Plugin\migrate\source\d6\CckFieldValues->prepareRow() is called, but without any single-valued fields defined resulting in the error @oadaeh and I were running into (see comment 18).

I've attached a patch that wraps the $query->execute() with an if-statement that checks to see if any single-valued fields have been added to the query.

Please put something about this in the code. I read the patch and for my life I couldn't figure out why that count was useful or what it's doing something to the tones of "$query only contains single value CCK fields and so when the only CCK field attached to a content type is a multi-valued CCK field then this query would be invalid".

benjy’s picture

Status: Needs work » Needs review
FileSize
13.3 KB
938 bytes

Good point, updated the comment.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Much better, thanks for the quick fix!

The last submitted patch, 29: 2279323-29-FAIL.patch, failed testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 9ac85b0 on 8.0.x
    Issue #2279323 by benjy, oadaeh, ultimate, chx: Fixed Data for fields...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.