Add bundle to the sourceIDs to FieldOptionTranslation source plugin [#3103031]

Problem/Motivation

Noticed that a test is missing that should have been in #3008029: Migrate D7 i18n field option translations. It is the test of the migration of the field option translations for booleans, like checkboxes.

Then the fix caused an error with Postgres and that exposed that the bundle was not included in the source IDs.

Proposed resolution

This adds data to the fixture and a test of the field option translation for a checkbox.

Add 'bundle' to the IDs in the source plugin. Without that only the first row for field_boolean was being processed and that happened to be 'test_content_type' for mysql and sqlite and 'blog' for 'postgres. Now, all bundles will be processed.

Note that adding the bundle as a key resulted in a key that was too long resulting in 'Syntax error or access violation: 1071 Specified key was too long; max key length is 3072' for MySQl. That was fixed in #3098282: [backport] SQL error if migration has too many ID fields and the key can simply be added now.

Comment	File	Size	Author
#53	3103031-53.patch	15.5 KB	quietone
#53	9.3.x: PHP 8.0 & MySQL 5.7 28,637 pass
#53	diff-51-53.txt	2.84 KB	quietone
#51	3103031-51.patch	15.36 KB	quietone
#51	9.2.x: PHP 7.3 & MySQL 5.7 28,184 pass PHP 7.3 & PostgreSQL 10.12 28,184 pass PHP 7.3 & SQLite 3.28 28,184 pass
#51	diff-49-51.txt	2.43 KB	quietone
#50	3103031-49.patch	15.35 KB	quietone
#50	9.2.x: PHP 7.3 & MySQL 5.7 28,059 pass PHP 7.3 & PostgreSQL 10.12 28,059 pass PHP 7.3 & SQLite 3.28 28,059 pass
#49	3103031-49.txt	15.35 KB	quietone
#49	interdiff-47-49.txt	473 bytes	quietone
#47	3103031-47.patch	14.71 KB	quietone
#47	9.2.x: PHP 7.3 & MySQL 5.7 28,058 pass, 1 fail
#47	diff-46-47.txt	2.21 KB	quietone
#45	3103031-46.patch	14.56 KB	quietone
#45	9.2.x: PHP 7.3 & MySQL 5.7 27,823 pass PHP 7.3 & PostgreSQL 10.12 27,823 pass PHP 7.3 & SQLite 3.28 27,823 pass
#45	interdiff-44-46.txt	2.11 KB	quietone
#44	3103031-44.patch	12.45 KB	quietone
#44	9.2.x: PHP 7.3 & MySQL 5.7 CI aborted
#44	interdiff-42-44.txt	794 bytes	quietone
#42	3103031-42.patch	12.44 KB	quietone
#42	9.2.x: PHP 7.3 & MySQL 5.7 27,821 pass, 2 fail
#42	diff-32-42.txt	4.4 KB	quietone
#32	3103031-32.patch	13.13 KB	quietone
#32	8.9.x: PHP 7.1 & MySQL 5.7 28,307 pass
#32	interdiff-26-32.txt	843 bytes	quietone
#26	interdiff-24-26.txt	763 bytes	quietone
#26	3103031-26.patch	12.59 KB	quietone
#26	8.9.x: PHP 7.1 & MySQL 5.7 28,169 pass PHP 7.1 & PostgreSQL 9.1 28,167 pass, 1 fail PHP 7.1 & SQLite 3.15 28,167 pass, 1 fail
#24	3103031-24.patch	12.38 KB	quietone
#24	8.9.x: PHP 7.1 & MySQL 5.7 28,166 pass, 3 fail PHP 7.1 & PostgreSQL 9.1 28,169 pass PHP 7.1 & SQLite 3.15 28,169 pass
#24	interdiff-21-24.txt	2.19 KB	quietone
#12	3103031-12.patch	11.45 KB	quietone
#12	8.9.x: PHP 7.1 & MySQL 5.7 28,141 pass PHP 7.1 & PostgreSQL 9.1 28,155 pass, 1 fail PHP 7.1 & SQLite 3.15 28,156 pass
#12	interdiff-10.12.txt	1.28 KB	quietone
#10	3103031-10.patch	11.5 KB	quietone
#10	8.9.x: PHP 7.1 & MySQL 5.7 28,133 pass
#10	interdiff-8-10.txt	562 bytes	quietone
#8	3103031-8.patch	11.43 KB	quietone
#8	8.9.x: PHP 7.1 & MySQL 5.7 28,132 pass, 1 fail
#8	interdiff-6-8.txt	7.98 KB	quietone
#6	3103031-6.patch	3.32 KB	quietone
#6	8.9.x: PHP 7.1 & MySQL 5.7 28,131 pass, 2 fail
#6	interdiff-4-6.txt	633 bytes	quietone
#4	interdiff-2-4.txt	2.24 KB	quietone
#4	3103031-4.patch	10.81 KB	quietone
#4	8.9.x: PHP 7.1 & MySQL 5.7 28,132 pass, 1 fail
#2	3103031-2.patch	8.85 KB	quietone
#2	8.9.x: PHP 7.1 & MySQL 5.7 28,114 pass, 15 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

25 December 2019 at 09:15

quietone created an issue. See original summary.

Comment #2

quietone CreditAttribution: quietone as a volunteer commented 25 December 2019 at 09:17

Status:

Active

» Needs review

File	Size
3103031-2.patch	8.85 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,114 pass, 15 fail

Comment #3

25 December 2019 at 10:24

Status:

Needs review

» Needs work

The last submitted patch, 2: 3103031-2.patch, failed testing. View results

Comment #4

quietone CreditAttribution: quietone as a volunteer commented 25 December 2019 at 12:32

Status:

Needs work

» Needs review

File	Size
3103031-4.patch	10.81 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,132 pass, 1 fail
interdiff-2-4.txt	2.24 KB

Add missing field_revision_field_checkbox table.

Comment #5

25 December 2019 at 13:35

Status:

Needs review

» Needs work

The last submitted patch, 4: 3103031-4.patch, failed testing. View results

Comment #6

quietone CreditAttribution: quietone as a volunteer commented 26 December 2019 at 04:28

Status:

Needs work

» Needs review

File	Size
interdiff-4-6.txt	633 bytes
3103031-6.patch	3.32 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,131 pass, 2 fail

Yes, there is another field config so increase the entity count

Comment #7

26 December 2019 at 05:35

Status:

Needs review

» Needs work

The last submitted patch, 6: 3103031-6.patch, failed testing. View results

Comment #8

quietone CreditAttribution: quietone as a volunteer commented 26 December 2019 at 06:09

Status:

Needs work

» Needs review

File	Size
interdiff-6-8.txt	7.98 KB
3103031-8.patch	11.43 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,132 pass, 1 fail

Well, that was a bad patch, it didn't have the changes to the test fixture.

Comment #9

26 December 2019 at 07:12

Status:

Needs review

» Needs work

The last submitted patch, 8: 3103031-8.patch, failed testing. View results

Comment #10

quietone CreditAttribution: quietone as a volunteer commented 26 December 2019 at 07:21

Status:

Needs work

» Needs review

File	Size
interdiff-8-10.txt	562 bytes
3103031-10.patch	11.5 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,133 pass

And update the number of field_storage_config entities.

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 26 December 2019 at 09:46

Title:	Add missig test of field option translations for booleans	» Add missing test of field option translations for booleans
Status:	Needs review	» Needs work

+++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceOptionTranslationTest.php
@@ -0,0 +1,81 @@
+  public static $modules =
+    [
+      'comment',
+      'config_translation',
+      'datetime',
+      'file',
+      'image',
+      'language',
+      'link',
+      'locale',
+      'menu_ui',
+      'node',
+      'system',
+      'taxonomy',
+      'telephone',
+      'text',
+    ];

🔎 Weird indentation going on here.

+++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceOptionTranslationTest.php
@@ -0,0 +1,81 @@
+   * Tests migration of file variables to file.settings.yml.

🤔 I don't think this comment is accurate?

Comment #12

quietone CreditAttribution: quietone as a volunteer commented 26 December 2019 at 23:16

Status:

Needs work

» Needs review

File	Size
interdiff-10.12.txt	1.28 KB
3103031-12.patch	11.45 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,141 pass PHP 7.1 & PostgreSQL 9.1 28,155 pass, 1 fail PHP 7.1 & SQLite 3.15 28,156 pass

@Wim Leers, thanks. Should be fixed now.

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 27 December 2019 at 12:15

This looks good to me now, but I don't know enough about these particular source + destination to RTBC this. Hopefully my review in #11 and the iteration in #12 make it easier for the next person to RTBC this :)

Comment #14

NickDickinsonWilde

He/Him/His

English

Victoria, BC (T'So-uke lands)

CreditAttribution: NickDickinsonWilde commented 9 January 2020 at 19:30

Status:

Needs review

» Reviewed & tested by the community

Looks good to me, but never done any D7 multilingual stuff only D8, so not the highest confidence. But 2 somewhat hesitant +1s is somewhat equal to a RTBC I figure.

Comment #15

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Thunder commented 13 January 2020 at 17:25

Status:

Reviewed & tested by the community

» Fixed

New test looks good to me.

Committed and pushed 677f9d42d8 to 9.0.x and e4220e5ce6 to 8.9.x. Thanks!

Comment #16

13 January 2020 at 17:26

alexpott committed 677f9d4 on 9.0.x

Issue #3103031 by quietone, Wim Leers: Add missing test of field option...

Comment #17

13 January 2020 at 17:26

alexpott committed e4220e5 on 8.9.x

Issue #3103031 by quietone, Wim Leers: Add missing test of field option...

Comment #18

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Thunder commented 14 January 2020 at 21:31

Status:

Fixed

» Needs work

Unfortunately this broke Postgres so reverting.

Comment #19

14 January 2020 at 21:32

alexpott committed 9f121c8 on 8.9.x

Revert "Issue #3103031 by quietone, Wim Leers: Add missing test of field...

Comment #20

14 January 2020 at 21:32

alexpott committed 89bdd89 on 9.0.x

Revert "Issue #3103031 by quietone, Wim Leers: Add missing test of field...

Comment #21

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 15 January 2020 at 13:56

Just a reminder that anything interacting directly with the DB layer (including test fixtures) should always be tested on Postgres and SQLite before commit. Thanks!

Comment #22

quietone CreditAttribution: quietone as a volunteer commented 16 January 2020 at 21:13

Assigned:

Unassigned

» quietone

I should be able to get to this this week.

Comment #23

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 16 January 2020 at 21:43

Issue tags:

+PostgreSQL

Comment #24

quietone CreditAttribution: quietone as a volunteer commented 18 January 2020 at 01:23

Status:

Needs work

» Needs review

File	Size
interdiff-21-24.txt	2.19 KB
3103031-24.patch	12.38 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,166 pass, 3 fail PHP 7.1 & PostgreSQL 9.1 28,169 pass PHP 7.1 & SQLite 3.15 28,169 pass

Added 'bundle' to the IDs in the source plugin. Without that only the first row for field_boolean was being processed and that happened to be 'test_content_type' for mysql and sqlite and 'blog' for 'postgres. Now, all bundles will be processed.

The test is updated to test both 'test_content_type' and 'blog'.

Comment #25

18 January 2020 at 02:29

Status:

Needs review

» Needs work

The last submitted patch, 24: 3103031-24.patch, failed testing. View results

Comment #26

quietone CreditAttribution: quietone as a volunteer commented 18 January 2020 at 05:21

Status:

Needs work

» Needs review

File	Size
3103031-26.patch	12.59 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,169 pass PHP 7.1 & PostgreSQL 9.1 28,167 pass, 1 fail PHP 7.1 & SQLite 3.15 28,167 pass, 1 fail
interdiff-24-26.txt	763 bytes

Hmm. I thought I ran the test with mysql, must have made a mistake. And new errors as well.

OK, here is a fix. The source IDs in FieldOptionTranslation now have a max_length that is the same as that in the d7 schema. This time I am sure I tested locally with Mysql.

Comment #27

quietone CreditAttribution: quietone as a volunteer commented 18 January 2020 at 07:22

The sqlite fail is unrelated it is in, Media_library.Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest. See #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest).

Comment #28

quietone CreditAttribution: quietone as a volunteer commented 18 January 2020 at 07:23

Assigned:

quietone

» Unassigned

Ready for review.

Comment #29

NickDickinsonWilde

He/Him/His

English

Victoria, BC (T'So-uke lands)

CreditAttribution: NickDickinsonWilde commented 18 January 2020 at 19:19

Status:

Needs review

» Reviewed & tested by the community

LGTM

Comment #30

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Thunder commented 20 January 2020 at 13:59

Status:

Reviewed & tested by the community

» Needs review

+++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldOptionTranslation.php
@@ -62,8 +62,15 @@ public function fields() {
-        'language' => ['type' => 'string'],
+        'language' => [
+          'type' => 'string',
+          'max_length' => '12',
+        ],
         'property' => ['type' => 'string'],
+        'bundle' => [
+          'type' => 'string',
+          'max_length' => '32',
+        ],

This is the only place where we've done this. I'm concerned that these might not be correct in some rare instance. How about adding an order by bundle in \Drupal\field\Plugin\migrate\source\d7\FieldOptionTranslation::query()?

Comment #31

quietone CreditAttribution: quietone as a volunteer commented 6 February 2020 at 14:32

Assigned:

Unassigned

» quietone

I'll try to respond to this in a few days.

Comment #32

quietone CreditAttribution: quietone as a volunteer commented 7 February 2020 at 11:34

File	Size
interdiff-26-32.txt	843 bytes
3103031-32.patch	13.13 KB
8.9.x: PHP 7.1 & MySQL 5.7 28,307 pass

@alexpott. Thanks for the review.

The problem here is not about ordering. The source query needs to have 5 sourceIDs so that all the needed rows are processed but with 5 sourceIDs the key length is too long. Without the 5 IDs SourcePluginBase::next() will only process the first row of a set that where the first 4 sourceIDs are the same, because it looks at the map table and sees that a row with these source keys has been processed. The bundle really needs to be a key.

Typically, we just add the ID and use the default length. That is what was done in #24 which results in the error, "SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 3072 bytes: "

Are there any other ways to solve this?

Also, I was getting a duplicate column error so the query is modified for that. I didn't run the test locally (I'm off to bed) but it worked fine with drush.

Comment #33

quietone CreditAttribution: quietone as a volunteer commented 8 February 2020 at 10:14

Assigned:

quietone

» Unassigned

Comment #34

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 13 February 2020 at 22:31

Adding a new column into the id map has BC implications for existing migrations. Do we need to handle this gracefully for sites that might be doing incremental migrations?

Comment #35

quietone CreditAttribution: quietone as a volunteer commented 14 February 2020 at 01:55

Issue summary:	View changes
Status:	Needs review	» Needs work

Good point. I think some hands on testing to be sure about incrementals is needed.

Comment #36

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 14 February 2020 at 14:18

Category:	Task	» Bug report
Issue tags:		+Needs title update

And this seems to have extended beyond just adding test coverage and seems to be fixing a bug. Needs a better title.

Comment #37

quietone CreditAttribution: quietone as a volunteer commented 14 February 2020 at 22:49

Title:	Add missing test of field option translations for booleans	» Add bundle to the sourceIDs to FieldOptionTranslation source plugin
Issue tags:	-Needs title update

Yes it does

Comment #38

quietone CreditAttribution: quietone as a volunteer commented 28 February 2020 at 13:50

Found another issue reporting the same problem with the key length being greater that 3072bytes, #3098282: [backport] SQL error if migration has too many ID fields

Comment #39

28 February 2020 at 13:50

Version:

8.9.x-dev

» 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Comment #40

quietone CreditAttribution: quietone as a volunteer commented 3 July 2020 at 01:43

Issue summary:	View changes
Status:	Needs work	» Postponed

Comment #41

3 July 2020 at 01:43

Version:

9.1.x-dev

» 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Comment #42

quietone CreditAttribution: quietone as a volunteer commented 23 November 2020 at 11:09

Issue summary:	View changes
Status:	Postponed	» Needs review

File	Size
diff-32-42.txt	4.4 KB
3103031-42.patch	12.44 KB
9.2.x: PHP 7.3 & MySQL 5.7 27,821 pass, 2 fail

No longer postponed since #3098282: [backport] SQL error if migration has too many ID fields was committed.

And a reroll.

Comment #43

23 November 2020 at 12:07

Status:

Needs review

» Needs work

The last submitted patch, 42: 3103031-42.patch, failed testing. View results

Comment #44

quietone CreditAttribution: quietone as a volunteer commented 23 November 2020 at 12:36

Status:

Needs work

» Needs review

File	Size
interdiff-42-44.txt	794 bytes
3103031-44.patch	12.45 KB
9.2.x: PHP 7.3 & MySQL 5.7 CI aborted

Silly me. Forgot to add these fixes.

Comment #45

quietone CreditAttribution: quietone as a volunteer commented 23 November 2020 at 13:13

File	Size
interdiff-44-46.txt	2.11 KB
3103031-46.patch	14.56 KB
9.2.x: PHP 7.3 & MySQL 5.7 27,823 pass PHP 7.3 & PostgreSQL 10.12 27,823 pass PHP 7.3 & SQLite 3.28 27,823 pass

Update MigrateFieldOptionTranslationTest

Comment #46

quietone CreditAttribution: quietone as a volunteer commented 13 December 2020 at 08:41

Issue summary:

View changes

Updating IS

Comment #47

quietone CreditAttribution: quietone as a volunteer commented 14 January 2021 at 11:44

File	Size
diff-46-47.txt	2.21 KB
3103031-47.patch	14.71 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,058 pass, 1 fail

Needed a reroll.

Comment #48

14 January 2021 at 12:44

Status:

Needs review

» Needs work

The last submitted patch, 47: 3103031-47.patch, failed testing. View results

Comment #49

quietone CreditAttribution: quietone as a volunteer commented 14 January 2021 at 20:27

Status:

Needs work

» Needs review

File	Size
interdiff-47-49.txt	473 bytes
3103031-49.txt	15.35 KB

FieldOptionTranslation needs another change, after #3187463: Fix "d7_field_option_translation" process plugin.

Comment #50

quietone CreditAttribution: quietone as a volunteer commented 14 January 2021 at 20:28

File	Size
3103031-49.patch	15.35 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,059 pass PHP 7.3 & PostgreSQL 10.12 28,059 pass PHP 7.3 & SQLite 3.28 28,059 pass

Naming the patch correctly will definitely help.

Comment #51

quietone CreditAttribution: quietone as a volunteer commented 8 March 2021 at 08:32

File	Size
diff-49-51.txt	2.43 KB
3103031-51.patch	15.36 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,184 pass PHP 7.3 & PostgreSQL 10.12 28,184 pass PHP 7.3 & SQLite 3.28 28,184 pass

Needed a reroll

Comment #52

8 March 2021 at 08:32

Version:

9.2.x-dev

» 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #53

quietone CreditAttribution: quietone as a volunteer commented 6 May 2021 at 08:22

Issue tags:

+Bug Smash Initiative

File	Size
diff-51-53.txt	2.84 KB
3103031-53.patch	15.5 KB
9.3.x: PHP 8.0 & MySQL 5.7 28,637 pass

Needed another reroll.

Comment #54

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 4 June 2021 at 06:35

Status:

Needs review

» Reviewed & tested by the community

+++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceOptionTranslationTest.php
@@ -0,0 +1,86 @@
+      'off_label' => 'fr - Stop',

surely this would be arrêter 😉

This looks good to me, but can only go into the current branches because #3098282: [backport] SQL error if migration has too many ID fields isn't backported yet

Comment #55

22 June 2021 at 14:34

catch committed d88f334 on 9.3.x

Issue #3103031 by quietone, alexpott, heddn, Wim Leers: Add bundle to...

Comment #56

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 22 June 2021 at 14:34

Version:	9.3.x-dev	» 9.2.x-dev
Status:	Reviewed & tested by the community	» Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Comment #57

22 June 2021 at 14:35

catch committed 60bd72f on 9.2.x

Issue #3103031 by quietone, alexpott, heddn, Wim Leers: Add bundle to...

Comment #58

6 July 2021 at 14:39

Status:

Fixed

» Closed (fixed)

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

Add bundle to the sourceIDs to FieldOptionTranslation source plugin

Problem/Motivation

Proposed resolution

Comments

Related issues

Referenced by