The d7_dump fixture needs at least one translated node for #2669964: Migrate Drupal 7 core node translations to Drupal 8. This is enough for one issue because working with the dumps is prone to unexpected problems. And it will allow the discussion of the logic of migrating D7 translations to continue without interruption.

The D7 dump already has a language installed, Icelandic. I added a translation of an existing node and created a patch. The resulting patch is 1.2M in size, the same size of the fixture. I was put off by the size and didn't run any tests on that. Any ideas on improving that are welcome.

CommentFileSizeAuthor
#22 interdiff.txt746 bytesquietone
#22 d7_dump_2785241-22.patch9.81 KBquietone
#20 interdiff.txt1.09 KBquietone
#20 d7_dump_2785241-20.patch10.3 KBquietone
#17 d7_dump_2785241-17.patch11.16 KBquietone
#12 d7_dump_2785241-12.patch11.73 KBquietone
#8 d7_dump_2785241-8.patch265.05 KBquietone
#6 d7_dump_2785241-6.patch309.87 KBquietone
#3 d7_dump_2785241-3.patch333.86 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Do you have the patch that you can share so we can take a look at what changed? Its hard to provide ideas without seeing what you have.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
d7_dump_2785241-3.patch333.86 KB

Got over my initial shock and started again. This one is much smaller and I suspect that I ran an update on the previous attempt and that might be the reason for the size.

Status: Needs review » Needs work

The last submitted patch, 3: d7_dump_2785241-3.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

The three failures are from tests that limit the node migrations to the test_content_type. But the translated node is of the type article. So, two choices. One, add 'd7_node:article' to those tests which should fix the tests. And two, change the fixture to have a translation of the existing test_content_type node. Right now I prefer the later, after all it is the test_content_type and also it has additional fields. But it is late, and I'll look at this tomorrow.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
d7_dump_2785241-6.patch309.87 KB
  • Changed menu_override_parent_selector to FALSE
  • Set Multilingual support on the test_content type to 'Enabled, with translation'
  • Added a body field to the test_content_type
  • Moved the body field to the top of the display
  • Added text to the body of node/1
  • Added a translation of node/1. The title and body are simply preceded with 'is - ' to indicate Icelandic. The translated node is node/3.

Made the patch and removed changes not needed, like changes to variables and items in actions and aggregator, login times etc. Probably could remove more?

Are there any more changes needed?

Status: Needs review » Needs work

The last submitted patch, 6: d7_dump_2785241-6.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
d7_dump_2785241-8.patch265.05 KB

The path of using the test_content_type required too many changes. I'd prefer to keep it simple. So back to having the translated node be of type article.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Needs work
Issue tags: +D8MI, +language-node, +sprint, +i18n-migrate 
  1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
@@ -38923,7 +39095,7 @@
-  'info' => 'a:15:{s:4:"name";s:4:"Node";s:11:"description";s:66:"Allows content to be submitted to the site and displayed on pages.";s:7:"package";s:4:"Core";s:7:"version";s:4:"7.40";s:4:"core";s:3:"7.x";s:5:"files";a:2:{i:0;s:11:"node.module";i:1;s:9:"node.test";}s:8:"required";b:1;s:9:"configure";s:21:"admin/structure/types";s:11:"stylesheets";a:1:{s:3:"all";a:1:{s:8:"node.css";s:21:"modules/node/node.css";}}s:7:"project";s:6:"drupal";s:9:"datestamp";s:10:"1444866674";s:5:"mtime";i:1444866674;s:12:"dependencies";a:0:{}s:3:"php";s:5:"5.2.4";s:9:"bootstrap";i:0;}',
+  'info' => 'a:13:{s:4:"name";s:4:"Node";s:11:"description";s:66:"Allows content to be submitted to the site and displayed on pages.";s:7:"package";s:4:"Core";s:7:"version";s:8:"7.43-dev";s:4:"core";s:3:"7.x";s:5:"files";a:2:{i:0;s:11:"node.module";i:1;s:9:"node.test";}s:8:"required";b:1;s:9:"configure";s:21:"admin/structure/types";s:11:"stylesheets";a:1:{s:3:"all";a:1:{s:8:"node.css";s:21:"modules/node/node.css";}}s:5:"mtime";i:1456021382;s:12:"dependencies";a:0:{}s:3:"php";s:5:"5.2.4";s:9:"bootstrap";i:0;}',

    There is a whole bunch of unrelated changes like this with version numbers. We should not do this, we don't need this for translations. 

  2. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
@@ -39949,6 +40132,39 @@
 ->values(array(
+  'filename' => 'sites/all/modules/devel/devel.module',
+  'name' => 'devel',
+  'type' => 'module',

@@ -39960,6 +40176,281 @@
 ->values(array(
+  'filename' => 'sites/all/modules/entity/entity.module',
+  'name' => 'entity',
+  'type' => 'module',
+  'owner' => '',
+  'status' => '0',

@@ -39982,6 +40473,83 @@
 ->values(array(
+  'filename' => 'sites/all/modules/variable/variable.module',
+  'name' => 'variable',

    There are a whole bunch of new contrib modules added here, those should not be added either.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

Removing those extra things would I think shave off two thirds of the patch if not more.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@Gábor Hojtsy. thanks for spotting those! I was just shutting down when I got your IRC msg. I did mean to mention that it will still need some cleanup and that it is too late and I'm too bleary eyed to do more. Seems I was too tired to even write that.

Busy day tomorrow, so maybe on the weekend....

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
d7_dump_2785241-12.patch11.73 KB

Seems I do have some time since the errands this morning all went smoothly. This time, more aggressive at removing unnecessary changes in the dump.

Status: Needs review » Needs work

The last submitted patch, 12: d7_dump_2785241-12.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: d7_dump_2785241-12.patch, failed testing.

The last submitted patch, 12: d7_dump_2785241-12.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
d7_dump_2785241-17.patch11.16 KB

thx to phenaproxima for pointing out the error in the test output that I missed 3x!

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Right, that is better.

For review check that

  1. the fixture has everything needed to test migrating D7 translation nodes. Right now there is one translated node.
  2. If anything else can be removed from the test fixture.
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Needs work

Looks good other than:

  1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
@@ -8631,6 +8739,22 @@
 ))
+->values(array(
+  'lid' => '49',
+  'location' => 'misc/ajax.js',
+  'textgroup' => 'default',
+  'source' => 'The response failed verification so will not be processed.',
+  'context' => '',
+  'version' => 'none',
+))
+->values(array(
+  'lid' => '50',
+  'location' => 'misc/ajax.js',
+  'textgroup' => 'default',
+  'source' => 'The callback URL is not local and not trusted: !url',
+  'context' => '',
+  'version' => 'none',
+))

    These locale strings are definitely not needed. 

  2. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
@@ -40910,7 +41100,7 @@
 ->values(array(
   'name' => 'additional_settings__active_tab_article',
-  'value' => 's:15:"edit-submission";',
+  'value' => 's:13:"edit-workflow";',
 ))

    Why is this needed?

Also i18n blog settings seems to be moved around which is also not needed right?

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
d7_dump_2785241-20.patch10.3 KB
interdiff.txt1.09 KB

Thx, those are removed.

Status: Needs review » Needs work

The last submitted patch, 20: d7_dump_2785241-20.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
d7_dump_2785241-22.patch9.81 KB
interdiff.txt746 bytes

Hmm, sorry that was sloppy. (But the baby is changed and not fretting anymore).

Anyway, for the points in #19.
1. Removed
2. Doesn't seem to be needed. Removed

i18n blog settings put back where it was. That can be addressed the next time the dump is updated.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Tests are updated accordingly. Will make good use of this in #2669964: Migrate Drupal 7 core node translations to Drupal 8 :)

alexpott’s picture

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

Committed and pushed 437ebf7 to 8.3.x and 5115c5b to 8.2.x and c5ce862 to 8.1.x. Thanks!

  • alexpott committed 437ebf7 on 8.3.x 
    Issue #2785241 by quietone, Gábor Hojtsy: Add a translated node to...

  • alexpott committed 5115c5b on 8.2.x 
    Issue #2785241 by quietone, Gábor Hojtsy: Add a translated node to...

  • alexpott committed c5ce862 on 8.1.x 
    Issue #2785241 by quietone, Gábor Hojtsy: Add a translated node to...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -sprint

Yay, thanks!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

The change here to menu_override_parent_selector in drupal7.php does not make sense for other tests, see #2794699: MigrateMenuSettingsTest.php is not in the correct location.

Discovered while running Drupal core phpunit tests on Travis CI https://travis-ci.org/klausi/drupal/jobs/157292466

Status: Fixed » Closed (fixed)

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