Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#37 | interdiff.txt | 938 bytes | benjy |
#37 | 2279323-37.patch | 13.3 KB | benjy |
#34 | interdiff.txt | 785 bytes | benjy |
#34 | 2279323-34.patch | 13.09 KB | benjy |
#31 | 2279323-31-PASS.patch | 12.33 KB | benjy |
Comments
Comment #1
ultimikeI 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
D8 results
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
Comment #2
ultimikeI took a shot at updating the MigrateCckFieldValuesTest class with a new test field that fails.
-mike
Comment #3
oadaeh CreditAttribution: oadaeh commentedI 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.
Comment #4
ultimike@oadaeh,
Any progress on this? Ping me on IRC and I'll help you get this moving in the right direction again.
Thanks,
-mike
Comment #5
oadaeh CreditAttribution: oadaeh commentedHello 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.
Comment #6
oadaeh CreditAttribution: oadaeh commentedHere'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.
Comment #7
oadaeh CreditAttribution: oadaeh commentedBy 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.
Comment #10
oadaeh CreditAttribution: oadaeh commentedThat'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.
Comment #11
oadaeh CreditAttribution: oadaeh commentedI 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.
Comment #12
oadaeh CreditAttribution: oadaeh commentedI've commented out the part that loads the test data to see what the testbot thinks of the test.
Comment #13
oadaeh CreditAttribution: oadaeh commentedFor this patch, I have re-enabled the data load, but I set the values that it loads to be incorrect.
Comment #14
oadaeh CreditAttribution: oadaeh commentedI 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:
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.
Comment #15
ultimikeoadaeh,
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:
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
Comment #16
ultimikeComment #17
ultimikeHmm - 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)
Multi-valued text field (with text field widget)
Multi-valued decimal field (with text field widget)
@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
Comment #18
oadaeh CreditAttribution: oadaeh commentedI 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:
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.
Comment #19
ultimike@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
Comment #20
oadaeh CreditAttribution: oadaeh commentedI 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:
drush dl drupal-6
), which gave me drupal-6.31drush 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.)drush -y si --db-url=mysql://$username:$password@$host:$port/$d6_database_name --account-name=$uid1name --account-pass=$uid1pass
)drush dl cck
) and enabled the Content and Text modules (drush -y en text
)drush -y si --db-url=mysql://$username:$password@$host:$port/$d8_database_name --account-name=$uid1name --account-pass=$uid1pass
)drush en migrate_drupal -y
)drush migrate-manifest manifest --db-url=mysql://$username:$password@$host:$port/$d6_database_name
)Here is my manifest file:
(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.
Comment #21
oadaeh CreditAttribution: oadaeh commentedI 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:
Comment #22
ultimike@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
Comment #23
ultimikeOk, 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
Comment #24
ultimikeComment #25
ultimikeWhoops - I mistakenly attached the same patch twice in comment 23 - sorry!
-mike
Comment #26
oadaeh CreditAttribution: oadaeh commentedSorry 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:
Here is how I ran it:
The comment is now one character too long. Also, I think the comment is not as clear as it could be.
Comment #27
ultimike@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
Comment #28
chx CreditAttribution: chx commentedThanks 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!
Comment #29
ultimikechx,
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):
Thanks,
-mike
Comment #30
chx CreditAttribution: chx commentedGood 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.
Comment #31
benjy CreditAttribution: benjy commentedTo 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.
Comment #34
benjy CreditAttribution: benjy commentedFix for MigrateNodeBundleSettingsTest
Comment #35
benjy CreditAttribution: benjy commentedComment #36
chx CreditAttribution: chx commentedI am very sorry but
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: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".
Comment #37
benjy CreditAttribution: benjy commentedGood point, updated the comment.
Comment #38
chx CreditAttribution: chx commentedMuch better, thanks for the quick fix!
Comment #40
webchickCommitted and pushed to 8.x. Thanks!