D6->D8 CCK Single On/Off Checkbox, Checkboxes/Radio buttons, and Select formatters

Here are some tests.

Here is a fix for the issue that involves greatly expanding the field type map. Another option would be to create a process plugin to handle the field types.

I, also modified several of the tests so that they provide better feedback.

Ok, let's try this again.

I forgot to add the unsupported_field_handler class to my patch. I, also, had to change some of the test data because a few of the new fields that I added to the test were miss-formatted.

Ok, here is an updated patch with the requested changes. It turns out that it wasn't parsing the allowed values from Drupal 6 but was just passing it in as a string like 'key1|value1\r\nkey2|value2', and that was causing the white screens.

There is still one place where it white screens though. When I create a field that gets migrated over as a Boolean and then go to the field instance settings page, I get the following in my Apache log followed by a stack trace:
Fatal error: Call to a member function form() on a non-object in /var/www/drupal8/core/lib/Drupal/Core/Field/FieldItemList.php on line 319

There is also an undefined offset error when I run the tests that I haven't been able to figure out.

If anyone knows where I should look to fix those, that would be great.

Oops, didn't mean to change the status.

First Rule Of Migration Debug: Never try to debug MigrateDrupal6Test! That way only lies burnout.

That's why we have the small tests, to have something debuggable. You got lucky cos both tests fail the same.

Drupal\Core\Config\Schema\Mapping->get('settings.allowed_values.1.2')
Drupal\Core\Config\StorableConfigBase->castValue('settings.allowed_values.1.2', '1.2') 

Well, CMI uses dot as an array delimiter so what this try to is $config['settings']['allowed_values'][1][2] = '1.2';

Does that help? I hope it does.

Also Drupal codign standards put the else on a separate line

}
else {

thanks.

Ok, I've changed it so that it replaces the . with an _ in the allowed values key.

I've, also, moved the else's onto their own lines.

I'm a little confused. The regex stuff is already in a process function. Is it possible to call another process function from within a process function? If so, is that a good idea? That would mean that we would be using multiple process functions, but you would not be able to tell that by simply looking at the .yml file for that migration.

I do think the new regex in the other issue looks much nicer though.

Here is the new patch with the cleaner regex.

Just rerolling for now and testing. It is a reroll of #29 and resolving conflicts against #2287727: Rename FieldConfig to FieldStorageConfig.

I have been wondering why we are introducing a UnsupportedFieldHandler plugin here. If I understand it correctly, this plugin will skip the row if the map has missed transforming the value. Isn't that the behaviour if we just remove bypass from static_map? I am attaching a patch for this to see what the testbot thinks.

Thanks, that's a great idea, I was overcomplicating things.

I missed out a couple of references to field_config from the old patch. I think I got them all now.

UPDATE: This builds on the patch in #33 with UnsupportedFieldHandler removed.

One of the test failures is due to a typo in field name in the test. I have fixed that with this patch. The other four failures are due to the float in keys being changed to underscore, as @ultimike mentioned in #26. From the current discussion in #2293773: Field allowed values use dots in key names - not allowed in config, we probably won't have to change the '.' to '_' once that issue is fixed. I have added a TODO in the patch for that (probably not required, but anyway).

I also reviewed the patch again, particularly changes to process/FieldSettings.php and simplified code a bit:

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldSettings.php
   @@ -48,6 +48,42 @@ public function getSettings($field_type, $global_settings, $widget_settings) {
   +      if ($field_type == 'list_text' || $field_type == 'list_integer') {
   

   I replaced this entire if-elseif-else block with a switch.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldSettings.php
   @@ -48,6 +48,42 @@ public function getSettings($field_type, $global_settings, $widget_settings) {
   +          if (isset($value[1])) {
   +            $allowed_values[$value[0]] = $value[1];
   +          }
   +          else {
   +            $allowed_values[$value[0]] = $value[0];
   +          }
   

   I removed this (and one other) if/else blocks in favour of ternary operators.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldSettings.php
   @@ -48,6 +48,42 @@ public function getSettings($field_type, $global_settings, $widget_settings) {
   +          $allowed_values_key = implode('_', explode('.', $value[0]));
   

   I changed the implode('_', explode('.', ...)) bit to a simple str_replace.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldSettings.php
   @@ -48,6 +48,42 @@ public function getSettings($field_type, $global_settings, $widget_settings) {
   +        $counter = 0;
   +        foreach ($d6_allowed_values as $value) {
   +          $allowed_values[$counter++] = $value;
   +        }
   +        unset($counter);
   +      }
   

   In the last else in that block of code, I removed the entire foreach loop and simply assigned the value to $allowed_values.

I posted this bit because the interdiff might not show these in an obvious way (because of additional indentation in switch block).

There should be 8 total failures with this patch now, which are related to the decimal point in allowed values key and is dealt with in #2293773: Field allowed values use dots in key names - not allowed in config. There is no point fixing that here (or even changing it to underscore, as that might be a regression?) We'll just have to wait until that fix gets in.

Thanks @ultimike.

I went through the comment in #26.2 and ran a manual test. I created a Integer field with Select list widget on a D6 site and devel generated 50 nodes. I ran a migration using the manifest in #2222375-6: Manual Testing: Nodes and found that the nodes in destination had the correct value set in both node edit form and node view page.

Am I missing something here? Can you please post exact steps to reproduce the problem. If it helps, I ran the test with the patch from #39 and #2306049-1: D6->D8 node migration - Handle nodes with format = 0 ? (which shouldn't matter here, but I needed it to migrate devel generated nodes).

On another note, #2293773: Field allowed values use dots in key names - not allowed in config is close to being RTBC'd, which is good news for this issue too. I think once that gets in, we won't need to make any changes here, just retest it as it is to fix those test failures (unless we have to reroll it).

#2293773: Field allowed values use dots in key names - not allowed in config is in, which means it's time to retest this.

Identical failures. I will continue debugging this.

@ultimike: Just to confirm, the issue with select list is not reproducible, right?

Awesome! I think I only have one question left. Given D6

function number_field_formatter_info() {
  return array(
    'default' => array('label' => '9999',            'multiple values' => CONTENT_HANDLE_CORE, 'field types' => array('number_integer', 'number_decimal', 'number_float')),
    'us_0'    => array('label' => '9,999',           'multiple values' => CONTENT_HANDLE_CORE, 'field types' => array('number_integer', 'number_decimal', 'number_float')),

why are we adding us_0 to text when the formatter only applies to number field types? Is this something somehow CCK allowed for real??

I agree with @chx in #51. I am trying to remove it and see what happens. I don't think there will be any failures.

I did a grep for us_0 in cck module in a D6 site and could not find any other usage apart from the one in number.module as listed above. I think this was a typo introduced in #15. This line was not present before the patch in that comment.

Thanks @chx, for pointing out the field instances that need updating. As discussed on IRC, I have removed unformatted from text field formatter mapping as well.

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCckFieldValuesTest.php
@@ -87,12 +97,13 @@ protected function setUp() {
+    $node = \Drupal\node\Entity\Node::load(1);

Can we add a use statement?

Thanks! This is almost ready. There's one more thing I would like to discuss (sorry for not catching it earlier): $d6_allowed_values = preg_split("/[\r\n,]+/", $global_settings['allowed_values']);. Where does that preg_split expression come from? I looked for validating with allowed_values and if I am not mistaken then http://drupalcontrib.org/api/drupal/contributions%21cck%21modules%21text... this is it and it calls http://drupalcontrib.org/api/drupal/contributions!cck!content.module/fun... which does

    $list = explode("\n", $field['allowed_values']);
    $list = array_map('trim', $list);
    $list = array_filter($list, 'strlen');

which we might actually want to keep. But first, where is that preg from? #17 says

It turns out that it wasn't parsing the allowed values from Drupal 6 but was just passing it in as a string like 'key1|value1\r\nkey2|value2', and that was causing the white screens.

I would recommend adopting the D6 code instead of inventing our own especially because our own picked up a comma from somewhere, I'm not quite sure where. Although I seem to remember having this discussion over at profile too and that was wrong, too. Also, I am reasonably sure that key|value_line1\rvalue_line2 will be completely valid in the D6 code because \r wasn't a delimiter (it was removed from the end of the line after explode by trim but it wasn't a delimiter).

There are multiple things I want to bring up here:

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldSettings.php
   @@ -48,6 +49,25 @@ public function getSettings($field_type, $global_settings, $widget_settings) {
   +      $d6_allowed_values = preg_split("/[\r\n,]+/", $global_settings['allowed_values']);
   

   I am trying to figure out the comma, but can't find it. Like @chx says, we should just remove it and use the strategy of content_allowed_values which seems to be called from all modules.

   More to the point, the comma might actually break tests if the allowed values key or label had a comma.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldSettings.php
   @@ -48,6 +49,25 @@ public function getSettings($field_type, $global_settings, $widget_settings) {
   +      switch ($field_type) {
   +        case 'list_text':
   +        case 'list_integer':
   +        case 'list_float':
   +          foreach ($d6_allowed_values as $value) {
   +            $value = explode("|", $value);
   +            $allowed_values[$value[0]] = isset($value[1]) ? $value[1] : $value[0];
   +          }
   +          break;
   +
   

   In D6 CCK, allowed values were a valid setting for text even if the widget did not allow selection. Typing in a value not present in the allowed values list would throw a validation error when creating the node. Does this make sense for Drupal 8 now? If so, we should migrate it. If not, we should document that any field using allowed values but not using one of those widgets would lose that ability.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/FieldSettings.php
   @@ -60,8 +80,22 @@ public function getSettings($field_type, $global_settings, $widget_settings) {
   +    $settings = ListItemBase::settingsToConfigData($settings);
   

   Is this necessary? The code does nothing in this case. Consider the function:

     public static function settingsToConfigData(array $settings) {
       if (isset($settings['allowed_values'])) {
         $settings['allowed_values'] = static::structureAllowedValues($settings['allowed_values']);
       }
       return $settings;
     }
   

   The if condition would always fail for the $settings array in our case. But the tests still work. I think the reason is we don't have to worry about calling this methods. Field storage takes care of that.

Elaborating more on #57.2, I don't think we can use allowed values for other field types. We have to document that users would lose this on migration. If they wish to avoid that, they have to change the widget on D6 site before upgrading.

Attaching patch with fixes for issues raised in comments #55, #56 and #57.

Particularly, I am removing the line $settings = ListItemBase::settingsToConfigData($settings); as I believe it doesn't do anything in this case. We have tests for checking this and if this line is necessary, the test would fail. So, let's see.

I should warn about the two interdiffs. I forgot to remove the use declaration for ListItemBase much later and I didn't have enough energy to start again. However, the patch is final with both interdiffs applicable.

I think we are good to go. As for #57.2 that's a bad joke. We can add it to the known issues pile but seriously, making you fail on text input without giving you even a hint of what you can type? That's the worst UX possible... and I doubt anyone ever used it. Anyways, I do not think we have the ability any more nor we should have it.

I agree about the UX part. It doesn't make sense and it is good that we don't have it anymore. But since this could be counted as data loss, I think we should at least document this somewhere (I am not sure where).

https://www.drupal.org/node/2167633

Committed 267202b and pushed to 8.x. Thanks!