Problem/Motivation

The d6_field migration triggers a few notices. Not every list field has allowed_values, because CCK in D6 has another settings for more dynamic options called allowed_values_php which contains php code.

Upgrading d6_field

Undefined variable: allowed_values FieldSettings.php:75                 [notice]
Undefined variable: allowed_values FieldSettings.php:78                 [notice]
Undefined variable: allowed_values FieldSettings.php:81                 [notice]
Undefined variable: allowed_values FieldSettings.php:84                 [notice]
Undefined variable: allowed_values FieldSettings.php:75                 [notice]
Undefined variable: allowed_values FieldSettings.php:78                 [notice]
Undefined variable: allowed_values FieldSettings.php:81                 [notice]
Undefined variable: allowed_values FieldSettings.php:84                 [notice]

Proposed resolution

It is out of scope to migrate fields with allowed_values_php, but we can fix at least the the PHP notices and log something.

Remaining tasks

User interface changes

API changes

Data model changes

Migrate so RC Eligible.

CommentFileSizeAuthor
#11 2590105-11.patch3.17 KBphenaproxima
#9 2590105-9.patch3.24 KBphenaproxima
#7 2590105-7.patch3.07 KBsnehi
#4 2590105-4.patch3.25 KBwebflo
#3 2590105-3.patch1.04 KBwebflo
#2 2590105-2.patch1.04 KBwebflo

Comments

webflo created an issue. See original summary.

webflo’s picture

webflo commented
StatusFileSize
new 2590105-2.patch1.04 KB
webflo’s picture

webflo commented
Status: Active » Needs review
StatusFileSize
new 2590105-3.patch1.04 KB

String is the wrong default value.

webflo’s picture

webflo commented
StatusFileSize
new 2590105-4.patch3.25 KB
neclimdul’s picture

neclimdul
He/Him
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +rc eligible

like it

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/d6/FieldSettingsTest.php
@@ -0,0 +1,82 @@
+  /**
+   * Provides field settings for testGetSettings().
+   */
+  public function getSettingsProvider() {
+    return [
+      [
+        'list_integer',
+        [
+          'allowed_values' => implode("\n", [
+            '1|One',
+            '2|Two',
+            '3'
+          ]),
+        ],
+        [
+          '1' => 'One',
+          '2' => 'Two',
+          '3' => '3',
+        ],
+      ],
+      [
+        'list_string',
+        [
+          'allowed_values' => null,
+        ],
+        [],
+      ],
+      [
+        'list_float',
+        [
+          'allowed_values' => "",
+        ],
+        []
+      ],
+      [
+        'boolean',
+        [],
+        []
+      ]
+    ];
+  }

I think this is almost unreadable. Any chance each dataset can be on one line.

snehi’s picture

snehi commented
Status: Needs work » Needs review
StatusFileSize
new 2590105-7.patch3.07 KB

Done as suggested.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work 
+++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/d6/FieldSettingsTest.php
@@ -0,0 +1,64 @@
+  public function getSettingsProvider() {
+    return [
+      [
+        'list_integer',
+        [
+          'allowed_values' => implode("\n", [ '1|One', '2|Two', '3' ]),
+        ],
+        [ '1' => 'One', '2' => 'Two', '3' => '3', ],
+      ],
+      [ 'list_string',
+        [
+          'allowed_values' => null, ], [], ],
+      [
+        'list_float',
+        [
+          'allowed_values' => "", ], [] ],
+      [
+        'boolean', [], [] ]
+    ];
+  }

This still hard to read -- the indentation seems off towards the end of the function.

It would probably be more readable to change this to array() syntax rather than []. I usually prefer [] but I'm forced to admit that, when defining large arrays, array() is more readable.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2590105-9.patch3.24 KB

Changed that data provider to array() syntax and fixed the formatting -- I think it's more readable now.

Status: Needs review » Needs work

The last submitted patch, 9: 2590105-9.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2590105-11.patch3.17 KB

Blah.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Seeing as how all I changed was a bit of formatting, I think I'm OK re-RTBCing this.

The last submitted patch, 4: 2590105-4.patch, failed testing.

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

The last submitted patch, 9: 2590105-9.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2590105-11.patch, failed testing.

The last submitted patch, 4: 2590105-4.patch, failed testing.

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

The last submitted patch, 11: 2590105-11.patch, failed testing.

The last submitted patch, 9: 2590105-9.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Reviewed & tested by the community

The last submitted patch, 9: 2590105-9.patch, failed testing.

webchick’s picture

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

Nice! A notice fix with a test. :) Looks like Alex's feedback has been addressed.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 4d9ce53 on 8.0.x 
    Issue #2590105 by webflo, phenaproxima, snehi, alexpott: Fix undefined...

Status: Fixed » Closed (fixed)

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