Problem/Motivation

Text fields in D6 have text formats. These fields are migrating with the cck_field_values migration but the formats are not converted to the new D8 machine_name as generated by the d6_filter_formats migration.

Proposed resolution

Add a text field process plugin.

Remaining tasks

Write patch and tests.

User interface changes

API changes

CommentFileSizeAuthor
#4 2394571-4.patch3.13 KBbenjy
#1 2394571-1.patch3.19 KBbenjy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjy’s picture

benjy CreditAttribution: benjy commented
Status: Active » Needs review
FileSize
2394571-1.patch3.19 KB

See what bot says, patch should make sense, only weird thing is i'm removing nid and type from fieldData(). Don't think they're needed.

Status: Needs review » Needs work

The last submitted patch, 1: 2394571-1.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCckFieldValuesTest.php
@@ -146,7 +146,7 @@ protected function setUp() {
-    $this->assertEqual($node->field_test->format, 1, "Shared field storage field with multiple columns is correct.");
+    $this->assertEqual($node->field_test->format, 'filtered_html');

Does that mean we actually need a more functional level test coverage, so ensure that the migrated data can be actually rendered as expected?

benjy’s picture

benjy CreditAttribution: benjy commented
Status: Needs work » Needs review
FileSize
2394571-4.patch3.13 KB

Forgot to make an interdiff, looks like this:

-     return $this->getSourceFieldInfo($this->configuration['bundle']);
+    $field_info = $this->getSourceFieldInfo($this->configuration['bundle']);
+    $field_info['nid'] = ['type' => 'number'];
+    $field_info['type'] = ['type' => 'varchar'];
+    return $field_info;

@dawenher, yes, maybe an approach where we visited the rendered content and the edit pages in the admin after migrating? In this instance visiting the node would have shown a fatal error. Same can be said for #2394571: Filter formats on cck text fields are not looked up in the idMap and visiting the node edit here #2394567: File field need associated metadata during cck_field migration.

chx’s picture

chx CreditAttribution: chx commented

That's a long slippery slope. One could argue that after MigrateDrupal6Test you need to rerun every integration test... I have described this in blog posts in the past.

benjy’s picture

benjy CreditAttribution: benjy commented

One could argue that after MigrateDrupal6Test you need to rerun every integration test

That's a great idea, we should at least do it once as a bug finding exercise.

chx’s picture

chx CreditAttribution: chx commented

erm, that would involve first rewriting the integration tests so they assert the things migratedrupal6test creates; unlikely to happen.

benjy’s picture

benjy CreditAttribution: benjy commented

We could add integration tests that simply views/edits a few nodes and users etc after the full migration but that would get out of hand quickly. It would catch many of the fatal errors we're seeing now when doing common tasks like editing nodes but I believe many of these issues will be fixed quickly once a few more people are using the Migrate API.

How about a review on the patch from #4?

chx’s picture

chx CreditAttribution: chx commented

Remind me, why do we map 0 to NULL? SkipProcessOnEmpty does if (!$value) { so it makes no difference there at least.

benjy’s picture

benjy CreditAttribution: benjy commented

We do that because in D6, the default format was 0 but in D8 it's NULL. We want to skip the process because we don't do any lookups for the default filter format.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Ah right! It's skip process not skip row. Early morning befuddling my brains.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Migration changes are unfrozen during beta. Committed d644718 and pushed to 8.0.x. Thanks!

  • alexpott committed d644718 on 8.0.x 
    Issue #2394571 by benjy: Filter formats on cck text fields are not...

Status: Fixed » Closed (fixed)

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