Please credit contributors to duplicate issue #2405017: Cannot migrate boolean fields of type text

Problem/Motivation

When migrating I'm seeing the following exceptions on what seems to be every insert for this field.

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22007]:          [error]
Invalid datetime format: 1366 Incorrect integer value: 'No' for
column 'field_featured_content_value' at row 1: INSERT INTO
{node__field_featured_content} (entity_id, revision_id, bundle,
delta, langcode, field_featured_content_value) VALUES
(:db_insert_placeholder_0, :db_insert_placeholder_1,
:db_insert_placeholder_2, :db_insert_placeholder_3,
:db_insert_placeholder_4, :db_insert_placeholder_5); Array
(
    [:db_insert_placeholder_0] => 31406
    [:db_insert_placeholder_1] => 31840
    [:db_insert_placeholder_2] => mytype
    [:db_insert_placeholder_3] => 0
    [:db_insert_placeholder_4] => und
    [:db_insert_placeholder_5] => No
)

Not 100% sure what the problem is. Obviously the datetime type check makes no sense. My guess is that its related to this field using the feature of allowed values that allows you to supply only keys and they will be translated into keys and values. Allowed values for this field:

No
Featured

Proposed resolution

?

Remaining tasks

?

User interface changes

?

API changes

?

Data model changes

None

Commit Credit

@phenaproxima, @neclimdul, @benjy, @svendecabooter

CommentFileSizeAuthor
#19 2588827-19.patch12.9 KBphenaproxima
#16 2588827-16.patch12.62 KBphenaproxima
#9 cck_migrations_seem_to-2588827-9.patch9.15 KBneclimdul
#9 interdiff.2588827-9.txt4.72 KBneclimdul
#7 2588827-7.patch17.07 KBphenaproxima
#5 2588827-5.patch18.36 KBphenaproxima
#2 cck_migrations_seem_to-2588827-2.patch14.01 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
FileSize
cck_migrations_seem_to-2588827-2.patch14.01 KB

Lets see if this triggers the error.

Has #2588421: CckLinks double unserialized attributes array changes because they'll conflict.

Status: Needs review » Needs work

The last submitted patch, 2: cck_migrations_seem_to-2588827-2.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Title: cck migrations seem to have trouble with some checkbox types » CCK migrations seem to have trouble with some checkbox types
Priority: Normal » Major
Issue tags: +Migrate critical

Exceptions, lost data, failed migrations, easily triggered. Going to bump to major.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2588827-5.patch18.36 KB

Hopefully this will help.

Status: Needs review » Needs work

The last submitted patch, 5: 2588827-5.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2588827-7.patch17.07 KB

Who wrote that annoying "one more time" song? The one with the auto-tune? Was it Cher? (It was Daft Punk. Thanks, @neclimdul)

Status: Needs review » Needs work

The last submitted patch, 7: 2588827-7.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Needs review
FileSize
interdiff.2588827-9.txt4.72 KB
cck_migrations_seem_to-2588827-9.patch9.15 KB

Interdiff is a little weird but just removing unrelated automated changes to the database. Things that happened as a result of making changes in the UI but where being asserted and aren't related to this patch.

Note, this is RTBC in my opinion. It fixed the failures in our migration.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Reviewed & tested by the community

I had hoped to test this manually with a D7 site that has a boolean user field, but unfortunately user fields are not yet supported in the D7 migration. Code looks good though, so RTBC.

The last submitted patch, 2: cck_migrations_seem_to-2588827-2.patch, failed testing.

The last submitted patch, 5: 2588827-5.patch, failed testing.

The last submitted patch, 7: 2588827-7.patch, failed testing.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I closed #2405017: Cannot migrate boolean fields of type text as a duplicate, it would be good if we could credit svendecabooter here for working on that issue.

Interdiff is a little weird but just removing unrelated automated changes to the database.

This is similar to what i meant on IRC, once we start manually manipulating the dumps, we can't truly say the database exactly represents what Drupal 6 could look like.

NW because we need some tests here.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2588827-16.patch12.62 KB

Re-added the test from #7.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented

Looks good, just one thing:

+++ b/core/modules/text/tests/src/Unit/Migrate/TextFieldTest.php
@@ -0,0 +1,120 @@
+    $migration->setProcessOfProperty(Argument::type('string'), Argument::type('array'))
+      ->will(function($arguments) use ($migration) {
+        $migration->getProcess()->willReturn($arguments[1]);
+      });
+

I think we could do without this block entirely?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

We need to keep that because we need some way to get the process array out of the mock migration (processCckFieldValues() alters the migration directly and does not return a value). Granted, we could just use a real migration entity instead of a prophesied mock, but since this is a unit test I'd rather reduce the number of x-factors that could potentially influence the outcome. :)

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
FileSize
2588827-19.patch12.9 KB

Added a comment to the test explaining the slightly strange thing we're doing with the mock. (Since no actual code was changed, I'm leaving out the interdiff due to changes merged into the D6 fixture.)

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

Awesome!

The last submitted patch, 2: cck_migrations_seem_to-2588827-2.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Removing some files from the active list so they don't get tested.

The last submitted patch, 5: 2588827-5.patch, failed testing.

The last submitted patch, 7: 2588827-7.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue summary: View changes

  • webchick committed 7a62888 on 8.0.x 
    Issue #2588827 by phenaproxima, neclimdul, svendecabooter, benjy,...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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