Problem/Motivation

We added migration for Drupal 6 node translation to Drupal 8 content translation in #2225775: Migrate Drupal 6 core node translation to Drupal 8. The situation with Drupal 7 is very similar in a way because Drupal 7 core also uses node copies for translations, but it is different because Drupal 7 also has configurable fields in core and some fields that were built-in in Drupal 6 are configurable in Drupal 8.

Also, Drupal 7 has per field translation support which facilitates the Entity Translation contributed module but not used in core for node translation, so that should be dealt with carefully when doing the migration.

Proposed resolution

  • Migrate field storage config to default to translatable as is with Drupal 8 default fields.
  • Migrate field instance config to set translatable based on whether the parent content type was translatable (based on the language_content_type_{bundle} variable from D7.
  • Make the node migration sources include only the default translation (same as with the D6 migration).
  • Add auxiliary migrations to bring in the translations (same as with the D6 migration).
  • Add new nodes for testing both in the migrate dump and in the kernel tests.

Remaining tasks

Commit :)

User interface changes

None.

API changes

The Drupal\node\Plugin\migrate\D7NodeDeriver plugin gets a new boolean $translations argument for its constructor to derive translation sources as well (same change as with D6NodeDeriver earlier).

The Drupal\node\Plugin\migrate\source\d7\D7Node migrate source plugin also takes a module handler in its constructor, so it can migrate source language information on translations when content translation module is enabled on Drupal 8 (same as with the D6 node source plugin earlier)

MigrateFieldInstanceTest::assertEntity() gets a new required argument at the end to assert for translatability.

Data model changes

None.

Files: 
CommentFileSizeAuthor
#104 2669964-104.patch43 KBGábor Hojtsy
#98 2669964-98.patch43.05 KBGábor Hojtsy
#98 interdiff.txt606 bytesGábor Hojtsy
#96 interdiff.txt838 bytesquietone
#95 2669964-95.patch43.06 KBquietone
#93 interdiff.txt1.61 KBquietone
#93 2669964-93.patch43.05 KBquietone
#92 interdiff-83-92.txt931 bytesquietone
#92 2669964-92.patch42.99 KBquietone
#87 interdiff.txt1003 bytesGábor Hojtsy
#87 2669964-87.patch42.93 KBGábor Hojtsy
#86 2669964-86.patch42.89 KBGábor Hojtsy
#83 2669964-83.patch42.95 KBJo Fitzgerald
#83 interdiff_82-83.txt918 bytesJo Fitzgerald
#82 interdiff.txt16.15 KBquietone
#82 2669964-82.patch43.05 KBquietone
#77 interdiff.txt10.38 KBquietone
#77 2669964-77.patch41.6 KBquietone
#74 interdiff.txt1.28 KBGábor Hojtsy
#74 2669964-74.patch41.92 KBGábor Hojtsy
#71 2669964-71.patch43.02 KBquietone
#65 interdiff.txt681 bytesGábor Hojtsy
#65 2669964-65.patch41.34 KBGábor Hojtsy
#60 interdiff.txt1.79 KBquietone
#60 2669964-60.patch41.43 KBquietone
#58 interdiff.txt9.92 KBquietone
#58 2669964-58.patch41.76 KBquietone
#54 interdiff.txt4.91 KBquietone
#54 2669964-54.patch35.14 KBquietone
#52 2669964-52.patch34.86 KBquietone
#50 interdiff.txt15.29 KBquietone
#50 2669964-50.patch34.66 KBquietone
#47 interdiff.txt1.78 KBquietone
#47 2669964-47.patch19.97 KBquietone
#44 interdiff-27-44.patch17.7 KBquietone
#44 2669964-44.patch18.87 KBquietone
#32 interdiff.txt445 bytesquietone
#32 2669964-32.patch28.23 KBquietone
#30 interdiff.txt6.22 KBquietone
#30 2669964-30.patch27.65 KBquietone
#27 interdiff.txt396 bytesquietone
#27 2669964-27.patch21.08 KBquietone
#25 interdiff.txt932 bytesquietone
#25 2669964-25.patch21.08 KBquietone
#22 interdiff.txt1.19 KBGábor Hojtsy
#22 2669964-22.patch20.41 KBGábor Hojtsy
#20 2669964-20.patch20.44 KBquietone
#11 SaveBasicPage.png39.14 KBquietone
#11 EditBasicPage.png39.14 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone’s picture

Issue tags: +migrate-d7-d8

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Status: Active » Postponed

This is dependent (at the very least) on the entity destination support for adding translations which is included in #2225775: Migrate Drupal 6 core node translation to Drupal 8.

mikeryan’s picture

Status: Postponed » Active
Gábor Hojtsy’s picture

Title: Migrate D7 i18n nodes » Migrate Drupal 7 core node translations to Drupal 8
Issue tags: -i18n-migrate drupal7 Migrate critical D8MI +i18n-migrate, +Migrate critical, +D8MI

Since the title said i18n module, I assume this is about Drupal 7 core translation. The technology for that should be the same as #2225775: Migrate Drupal 6 core node translation to Drupal 8. I don't think we need to solve the entity_translation issue here, but we need one opened for that too. As per #2225775-188: Migrate Drupal 6 core node translation to Drupal 8 we don't have such an issue yet.

Gábor Hojtsy’s picture

Issue tags: +drupal7
quietone’s picture

A first step is to add translations and perhaps another language to the test fixture. But an error is given when trying to enable multilingual support on the article, page or test content type. The error is in 'An illegal choice has been detected error' and that is on the Default parent item. Also, when editing content the 'Menu settings' option is not displayed even though the admin permissions are correct.

So, it does appear that something is amiss with the menus. If that is indeed the case, that should be addressed in a new issue.

Gábor Hojtsy’s picture

@quietone: did you experiment with that test? it sounds like from your report. It would be nice if you could post it so we see those fails you are talking about :) Thanks!

quietone’s picture

FileSize
39.14 KB
39.14 KB

@Gábor Hojtsy, sure.

  1. Bring up a D7 site using the test fixture as the db.
  2. Go to /admin/structure/types/manage/page.
  3. Click Save.

I tried various combinations of adding a new content type and new menus and changing the 'Default parent item'. All attempts result in the error.

quietone’s picture

Found the case. There is a D7 variable, 'menu_override_parent_selector' which is set to true. Setting it to false allows the content types to be modified via the UI. Note that MigrateMenuSettingsTest will need to be changed as well.

quietone’s picture

Added one translation for node/3 and the resulting patch is 1.3M.

Sigh.

Gábor Hojtsy’s picture

@quietone: would it make sense to post that in an issue and get committed so we can focus here on the logic patch itself?

quietone’s picture

As suggested a new issue has been created, #2785241: Add a translated node to d7_dump.

Gábor Hojtsy’s picture

Status: Active » Postponed

Marking postponed then.

Gábor Hojtsy’s picture

Status: Postponed » Active

Well, I guess reading that issue its true that only the testing of this is postponed on that, so we don't need to postpone this in fact.

Gábor Hojtsy’s picture

#2785241: Add a translated node to d7_dump landed. I think this needs a yaml file similar to the D6 yaml file and a test similar to the D6 test, since the data structures are the same for translations.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Status: Active » Needs review
FileSize
20.44 KB

Ok, let's make some progress here. I've pretty much copy/pasted the D6 node translation code to D7. Got most of the new/modified tests working except D7/MigrateNodeTest where the Icelandic translation is saved to the English node. I'm going to park the issue for a while and in the meantime, let's see what else testbot finds.

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
20.41 KB
1.19 KB

Two minor things fixed: D6 to D7 in the d7 code and a missing newline. Fixing these directly to avoid making noise about them :) No other changes.

Status: Needs review » Needs work

The last submitted patch, 22: 2669964-22.patch, failed testing.

Gábor Hojtsy’s picture

@quietone: several fails in MigrateUpgrade7Test too.

quietone’s picture

Status: Needs work » Needs review
FileSize
21.08 KB
932 bytes

This adds d7_node_translation to the migrate_drupal_ui test.

Status: Needs review » Needs work

The last submitted patch, 25: 2669964-25.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
21.08 KB
396 bytes

Oops, typed in the wrong screen.

Status: Needs review » Needs work

The last submitted patch, 27: 2669964-27.patch, failed testing.

quietone’s picture

Just a list of observations and questions:

  • On a fresh D8 site the storage for a body field, and a newly created field, has translatable = true. And each field instance can be set translatable true or false.
  • The D7 field_config table in the fixture has a translatable column and it is '0' for all fields. Is this correct? The body field is translated. There is also a translatable item in the data array in the field_config table. What is that used for?
  • Ran a test migration with this patch, which of course fails. Translatable is set false on the body field, and all the other field. But, what if these need to be changed to translatable at some time in the future? Didn't seem possible via the UI. But by changing the storage to translatable true, it was possible. So, maybe it would be better to make the storage for all fields translated (as per D8 default)?
  • Ran a test migration, with this patch and forcing translatable to be true in both d7_field and d7_field_instance and the results look correct in in node tables. But navigating to /is/node/N results in the English version of node N.

I think I'll bring up a fresh D7, create some translations, and get a better understanding of the field tables. And double check the fixture, esp for the translatable column.

quietone’s picture

Status: Needs work » Needs review
FileSize
27.65 KB
6.22 KB

Added the changes to EntityConfigBase.php and EntityFieldStorageConfig.php from #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields.

Status: Needs review » Needs work

The last submitted patch, 30: 2669964-30.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
28.23 KB
445 bytes

And finally, update the entity counts.

penyaskito’s picture

The D7 field_config table in the fixture has a translatable column and it is '0' for all fields. Is this correct? The body field is translated. There is also a translatable item in the data array in the field_config table. What is that used for?

I didn't know about the details, but installed a D7 site and played around it. I couldn't find any usage of that one in Content translation module in core, and if you set up entity translation it sets that up to true if there is a field marked as translatable with entity translation.
So I'm guessing this was added in Drupal core for allowing contrib modules as entity translation to allow this option.

Git annotate on the field.install schema method doesn't provide useful information for when and why this column was added.

penyaskito’s picture

Ran a test migration with this patch, which of course fails. Translatable is set false on the body field, and all the other field. But, what if these need to be changed to translatable at some time in the future? Didn't seem possible via the UI. But by changing the storage to translatable true, it was possible. So, maybe it would be better to make the storage for all fields translated (as per D8 default)?

I would say that, as content translation creates different nodes for each translation, if a content type is set as "content translation translatable" in D7, we should make all field instances (and their storages) translatable. And by all I mean all, not the D8 assumed defaults. E.g. we would need to create copies of file in images fields, as they may be different between different translations. I think we cannot do better than this.

penyaskito’s picture

Issue tags: +Dublin2016

From the translations point of view, the source code makes sense to me. Someone with more exposure to the APIs should check it though.
I haven't tested it yet.

And probably needs to wait for #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields to be committed, but don't want to mark this postponed yet.

About #33 and #34, I'm sure we should do that, but may not be the right issue. Fields, field instances and their settings should be migrated first, as stated:

+++ b/core/modules/node/migration_templates/d7_node_translation.yml
@@ -0,0 +1,36 @@
+  optional:
+    - d7_field_instance

Are we handling that in a different issue?

Gábor Hojtsy’s picture

@penyaskito: why would this be postponed on #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields? This is a pure core-core data migration while that migrates a contrib module's translation data to core. Why would that be a pre-requisite?

quietone’s picture

@Gábor Hojtsy, because the changes to the destination plugins, EntityConfigBase.php and EntityFieldStorageConfig.php in #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields were added to this in #30.

And note that the points raised in #34 and #35 still need to be addressed.

Gábor Hojtsy’s picture

All, right why do we need config translation coupled in here for our content translation then? These are not dependent on each other in Drupal 8 and Drupal 7 did not even have ways to translate anything related to nodes that I remember. (It could still be that my memory is short mind you :) What makes this require config translation support?

quietone’s picture

In D7 the body is a field and thus in D8 uses config for storage and field info.

Gábor Hojtsy’s picture

Ok, I am afraid that is still not enough information for me to tell why we need this. In which case would the node body field be saved as a translation of the body field? Where would its langcode come from to be identified as a translation? What I missing from the patch?

quietone’s picture

If a content type is set as "content translation translatable" in D7, we should make all field instances (and their storages) translatable.

Yes. And further for all content type the storage for all fields must be created with translatable true. Right now the D7 translatable value (from field_config) is used for both the storage and the instance. Presumably the field destination plugin could do this work.

@Gábor Hojtsy, the translation information is in node__body. Does that answer your question?

>select * from node__body;

+---------+---------+-----------+-------------+----------+-------+---------------------------------------------------------------------------+--------------+---------------+
| bundle  | deleted | entity_id | revision_id | langcode | delta | body_value                                                                | body_summary | body_format   |
+---------+---------+-----------+-------------+----------+-------+---------------------------------------------------------------------------+--------------+---------------+
| article |       0 |         2 |           2 | en       |     0 | ...is that it's the absolute best show ever. Trust me, I would know.      | NULL         | filtered_html |
| article |       0 |         2 |           2 | is       |     0 | is - ...is that it's the absolute best show ever. Trust me, I would know. | NULL         | filtered_html |
+---------+---------+-----------+-------------+----------+-------+---------------------------------------------------------------------------+--------------+---------------+
Gábor Hojtsy’s picture

But making the field instances or field storages translatable (resides in their respective original YAML files) does not require translating the configuration YAML files themselves, in fact the translations are not supposed to override anything about the translatability settings of the field storage or field instance configuration themselves. I think what you are referring to is that the field instance/storage config should configure the field to be translatable, not that the field instance/storage ITSELF should be translatable, which is not a requirement at all in Drupal 8 for content translation. You can enable content translation and never have config translation and still do all the content translation you want (much more than in Drupal 7). Therefore my lack of understanding why we need support for translation of the config itself in this patch. Drupal 8 does not require the translatability of the config for said config to configure a field as translatable, these are in two different subsystems.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima
quietone’s picture

This needs to be rerolled due #2807917: Convert Node's Migrate source tests to new base class. I chose to start again from the patch in #27, thus removing the changes to EntityConfigBase.php and EntityFieldStorageConfig.php. The updated files are the d7 tests, NodeTest.php and NodeTranslationTest.php.

Status: Needs review » Needs work

The last submitted patch, 44: interdiff-27-44.patch, failed testing.

The last submitted patch, 44: 2669964-44.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
19.97 KB
1.78 KB

Now, force the field storage to have translatable true and update the entity count in the migrate_drupal_ui MigrateUpgrade7Test. This works locally on the MigrateNode test, but lets see what the testbot finds.

Status: Needs review » Needs work

The last submitted patch, 47: 2669964-47.patch, failed testing.

Gábor Hojtsy’s picture

We discussed this on the migrate meeting. Indeed, the field translatability settings should be entirely ignored for node types that use core node translation in Drupal 7, because Drupal 7's core node translation just stores entirely separate copies of the node, so all fields may be translated. It is fine to force all the fields migrated to be set to be translatable.

The catch is in Drupal 7 core translatability vs. entity translation (contrib module) translatability may be different per content type. In core, http://cgit.drupalcode.org/drupal/tree/modules/translation/translation.m... decides which ones are core supported. So this variable should be inspected and this migration should only be run for node types where this would return TRUE :)

/**
 * Returns whether the given content type has support for translations.
 *
 * @return
 *   TRUE if translation is supported, and FALSE if not.
 */
function translation_supported_type($type) {
  return variable_get('language_content_type_' . $type, 0) == TRANSLATION_ENABLED;
}

If this would return TRUE then the migration in this patch should apply to nodes of that type and the field translatability settings in Drupal 7's field settings should be ignored entirely and migrated to translatable in Drupal 8 instead. If this would return FALSE for a node, then that node should not be attempted to be migrated with this migration, it should not have translations using the core method this patch supports.

quietone’s picture

Status: Needs work » Needs review
FileSize
34.66 KB
15.29 KB

@Gábor Hojtsy, thanks, that really helped

So, this patch modifies the field and field instance migrations. The field migration no longer migrates 'translatable' so that the default D8 value of true is used. This alone is sufficient for the MigrateNodeTest to pass. But translatable for the field instances should be set correctly. The field instance migration will migrate the translatable value from the field_config table to the field instance, unless it is a node. In that case translatable is forced to true in fieldInstance::prepareRow. Tests updated as well.

If this would return FALSE for a node, then that node should not be attempted to be migrated

I think this would result in data loss. language_content_type_ appears to be just a flag indicating if this node type is translatable or not. The translatability can be disabled after creating translated nodes. To be sure, I just tried this using the d7 dump database. Setting multilingual support to 'disabled', changes language_content_type to "0" and, as expected, the node and the icelandic translation are still there.

Status: Needs review » Needs work

The last submitted patch, 50: 2669964-50.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
34.86 KB

Looks like I wasn't working from HEAD. :-( Just needed to update d7_node.yml and d7_node_translation.yml

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Yay, great progress, some mostly minor / docs findings:

  1. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
    @@ -81,6 +82,19 @@ public function prepareRow(Row $row) {
    +    // If node translations are enabled for this content type, set translatable
    +    // to true.
    +    $translatable = FALSE;
    +    if ($row->getSourceProperty('entity_type') == 'node') {
    +      if ($this->variableGet('language_content_type_' . $row->getSourceProperty('bundle'), 0) == 2) {
    +        $translatable = TRUE;
    +      }
    +    }
    +    else {
    +      $data = unserialize($row->getSourceProperty('data_fc'));
    +      $translatable = $data['translatable'];
    

    Move the top comment into the condition IMHO and document the else clause too, so its clear what are the two paths someone may get translatable.

  2. +++ b/core/modules/node/migration_templates/d7_node_translation.yml
    @@ -26,9 +27,14 @@ process:
         - d7_field_instance
    +provider: migrate_drupal
    

    Why is this the provider if d7_node was not provided by migrate_drupal? (Both are in node module)

  3. +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
    @@ -39,33 +39,58 @@ class D7NodeDeriver extends DeriverBase implements ContainerDeriverInterface {
    +   * D7NodeDeriver constructor.
        * @param string $base_plugin_id
    

    Empty line lost between these.

  4. +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
    @@ -39,33 +39,58 @@ class D7NodeDeriver extends DeriverBase implements ContainerDeriverInterface {
       /**
    -   * {@inheritdoc}
    +   * Gets the definition of all derivatives of a base plugin.
    +   *
    +   * @param array $base_plugin_definition
    +   *   The definition array of the base plugin.
    +   *
    +   * @return array
    +   *   An array of full derivative definitions keyed on derivative id.
    +   *
    +   * @see \Drupal\Component\Plugin\Derivative\DeriverBase::getDerivativeDefinition()
        */
       public function getDerivativeDefinitions($base_plugin_definition) {
    

    This still implements the interface, we can/should leave as inheritdoc IMHO.

  5. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
    @@ -144,9 +147,15 @@ public function testNode() {
    +    $this->assertTrue($node->hasTranslation('is'), "Node 2 has an Icelandic translation");
    

    Can we assert some data (title, body to cover a base and a configurable field) about the translation too?

quietone’s picture

Status: Needs work » Needs review
FileSize
35.14 KB
4.91 KB

1. Fixed. (I wish I could find an example in the coding standards for this).
2. Is this related to #2560795: Source plugins have a hidden dependency on migrate_drupal. And since that is fixed it can be removed from d7_node_translation.yml and d6_node_translation.yml?
3. Fixed
4. Removed.
5. Tests added, but not for a field. Is that getting out of scope? The D6 core node translation didn't do any checking on fields.

And still to do:
I added the test from the D6 version which gets the source language of the translation and checks that it is what is expected. For D7 the source language returned is 'und' instead of the expected 'en'.

Gábor Hojtsy’s picture

@quietone:

re #54/2 I don't know, it is odd though that d7_node does not need it, why would this one need it?
re #54/5 While the D6 migration may not have tested fields, D7 fields are more complicated (as we have seen here :D), so testing the title and body IMHO would be nice to cover a sample base field and a sample configurable field
re #54/+1 does that have something to do with #2746293: Migrate content_translation_source when migrating node translations?

quietone’s picture

@Gábor Hojtsy, yes I can confirm that copy/pasting code from #2746293: Migrate content_translation_source when migrating node translations fixes this. And still to add from that patch is a test that content_translation_source for a source other than English is correct. That will involve modifying the dump, which is not something I look forward to, so I'll take a long break first.

quietone’s picture

Status: Needs review » Needs work

Forgot to change the status.

quietone’s picture

Status: Needs work » Needs review
FileSize
41.76 KB
9.92 KB

This adds a new node to the dump with language Icelandic, then an English translation of that node and tests the source language of that translation.

Status: Needs review » Needs work

The last submitted patch, 58: 2669964-58.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
41.43 KB
1.79 KB

Remove changes to unnecessary changes to tracker_node table in the dump.

Gábor Hojtsy’s picture

Yay. So I thin #54/5 is the only remaining piece? Or am I missing something?

quietone’s picture

Can we assert some data (title, body to cover a base and a configurable field) about the translation too?

I don't know if something is missing. Assertions exist on the body and title of the translated node. But I'm not really sure what exactly is meant by a configurable field and 'non configurable field. If another field needs to be added, what type? That node type has title, body, tags, image and link fields.

mikeryan’s picture

+++ b/core/modules/field/migration_templates/d7_field.yml
@@ -34,7 +34,10 @@ process:
+  # If translatble is false in field storage then the field can not be

translatble?

Gábor Hojtsy’s picture

+++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
@@ -144,9 +148,31 @@ public function testNode() {
+    $this->assertTrue($node->hasTranslation('is'), "Node 2 has an Icelandic translation");
+
+    $translation = $node->getTranslation('is');
+    $this->assertSame('is', $translation->langcode->value);
+    $this->assertSame("is - ...is that it's the absolute best show ever. Trust me, I would know.", $translation->body->value);
+    $this->assertSame('is - The thing about Deep Space 9', $translation->title->value);
+    $this->assertSame('internal:/', $translation->field_link->uri);
+    $this->assertSame('Home', $translation->field_link->title);

Oh, how did I miss all the assertions on the Icelandic translation? Those were the ones I thought were missing :)

So this looks complete then.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
41.34 KB
681 bytes

Fixing the typo Mike found. And with that I think this is now RTBC :)

heddn’s picture

+1 on RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: 2669964-65.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Random unrelated fail.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: 2669964-65.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +Needs reroll

Does not apply anymore unfortunately.

quietone’s picture

Status: Needs work » Needs review
FileSize
43.02 KB

Rerolled.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, thanks! Still looks good.

Gábor Hojtsy’s picture

Issue summary: View changes

Fully updated issue summary to help reviewers.

Gábor Hojtsy’s picture

Issue tags: -Needs reroll
FileSize
41.92 KB
1.28 KB

Also removing two db dump snippets that I found to be extra in the review I did for updating the issue summary. Data on node visits and two new locale strings. Neither has anything to with the node translation migration. The update is cosmetic so keeping at RTBC.

Gábor Hojtsy’s picture

Priority: Normal » Major
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. Lots of test coverage - nice work since multi-lingual is super complex.
  2. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
    @@ -26,6 +26,7 @@ public function query() {
    +    $query->addField('fc', 'data', 'data_fc');
     
         $query->innerJoin('field_config', 'fc', 'fci.field_id = fc.id');
         $query->addField('fc', 'data', 'field_data');
    
    @@ -86,6 +87,22 @@ public function prepareRow(Row $row) {
    +      $data = unserialize($row->getSourceProperty('data_fc'));
    

    From \Drupal\field\Plugin\migrate\source\d7\FieldInstance::query() - I don't see why we need to add the fc.data field twice... that's just confusing - no? If we do then a comment is necessary. I think that we could just do:
    $data = unserialize($row->getSourceProperty('field_data'));

  3. +++ b/core/modules/field/tests/src/Kernel/Plugin/migrate/source/d7/FieldInstanceTest.php
    @@ -94,6 +94,7 @@ public function providerSource() {
    +        'data_fc' => 'a:6:{s:12:"entity_types";a:1:{i:0;s:4:"node";}s:12:"translatable";b:0;s:8:"settings";a:0:{}s:7:"storage";a:4:{s:4:"type";s:17:"field_sql_storage";s:8:"settings";a:0:{}s:6:"module";s:17:"field_sql_storage";s:6:"active";i:1;}s:12:"foreign keys";a:1:{s:6:"format";a:2:{s:5:"table";s:13:"filter_format";s:7:"columns";a:1:{s:6:"format";s:6:"format";}}}s:7:"indexes";a:1:{s:6:"format";a:1:{i:0;s:6:"format";}}}',
    

    If we remove data_fc I think this needs changing to 'field_data'.

quietone’s picture

Status: Needs work » Needs review
FileSize
41.6 KB
10.38 KB

This patch addresses issues in #76.

Not sure why the interdiff includes this error, "interdiff impossible; taking evasive action".

phenaproxima’s picture

Status: Needs review » Needs work

I found nothing but nitpicks with this patch. Looks great!

  1. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
    @@ -86,6 +86,22 @@ public function prepareRow(Row $row) {
    +      if ($this->variableGet('language_content_type_' . $row->getSourceProperty('bundle'), 0) == 2) {
    

    What does 2 signify? There should be a comment about that. Better yet, let's use an interface constant if available.

  2. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldTest.php
    @@ -123,7 +121,8 @@ public function testFields() {
    +    $this->assertIdentical($migration->getSourcePlugin()
    

    This should be assertSame().

  3. +++ b/core/modules/node/migration_templates/d7_node_translation.yml
    @@ -0,0 +1,41 @@
    +  # the nid and vid fields to allow incremental migrations.
    

    The vid field is not in the process pipeline...

  4. +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
    @@ -65,7 +77,13 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
    +    if ($base_plugin_definition['id'] == 'd7_node_translation' && !$this->includeTranslations) {
    

    I'd prefer to use migration_tags instead of hard-coding the migration ID.

  5. +++ b/core/modules/node/src/Plugin/migrate/D7NodeDeriver.php
    @@ -65,7 +77,13 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
    +    $fields = array();
    

    Nit: Should be [].

  6. +++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
    @@ -14,8 +20,36 @@
    +   * @var \Drupal\Core\Extension\ModuleHandler
    

    Should be ModuleHandlerInterface.

  7. +++ b/core/modules/node/src/Plugin/migrate/source/d7/Node.php
    @@ -14,8 +20,36 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, StateInterface $state, EntityManagerInterface $entity_manager, ModuleHandler $module_handler) {
    

    This should be ModuleHandlerInterface.

  8. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeDeriverTest.php
    @@ -0,0 +1,50 @@
    +class MigrateNodeDeriverTest extends MigrateDrupal7TestBase {
    +  /**
    

    Nit: There should be an extra blank line here.

  9. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
    @@ -14,10 +14,12 @@
     class MigrateNodeTest extends MigrateDrupal7TestBase {
     
       public static $modules = array(
    +    'content_translation',
    

    I'm just now noticing this, but $modules should have an @inheritdoc comment.

  10. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
    @@ -144,9 +148,31 @@ public function testNode() {
    +    $this->assertSame('The thing about Deep Space 9', $node->title->value);
    

    Let's assert against $node->label() instead.

  11. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
    @@ -144,9 +148,31 @@ public function testNode() {
    +    $this->assertSame('is - The thing about Deep Space 9', $translation->title->value);
    

    Use $translation->label()

  12. +++ b/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d7/NodeTest.php
    @@ -77,7 +77,40 @@ public function providerSource() {
    +        // Node fields.
    

    Let's kill these comments in arrays where there is no mixing of node and node revision fields.

masipila’s picture

Hi,

Comment #7 clearly states that this issue will NOT contain migration path for D7 contrib entity_translation and its field translations. I perfectly understand the rationale for this since field level translations is a complete different story than the core translations.

According to #7, there was no issue for migrating entity_translation field translations to D8 six months ago. I've been trying to search one but haven't found yet. Could you guys who are familiar with the existing i18n migrate issues point me to the right issue if one exists already?

Cheers,
Markus

Gábor Hojtsy’s picture

@masipila: I think #2073467: Entity Translation: Upgrade path to D8? is the issue for that. I think it may be feasible to move that issue to core, since other contrib things that moved to core have their migration path in core, but I'm not the expert in placement of migrations :)

masipila’s picture

Thanks!

quietone’s picture

Status: Needs work » Needs review
FileSize
43.05 KB
16.15 KB

Fixed nits in #78. However, not sure the modified comment is sufficient for #1. And not sure what is meant by using migration tags in #4. Note this same code is used in the D6NodeDeriver.

Jo Fitzgerald’s picture

Expanded on the comment addressing #78.1 - hopefully that helps explain what the "2" means. Unfortunately there is not a relevant interface constant.

phenaproxima’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
@@ -88,9 +88,10 @@ public function prepareRow(Row $row) {
+      // If this bundle has Content Translation enabled (Drupal 7 Multilingual
+      // Support option "Enabled, with translation") then the row is
+      // translatable.

I don't think we need to mention "Drupal 7 Multilingual Support option 'Enabled, with translation'" here -- it's confusing. Let's just mention that 2 is a Drupal 7 constant that signifies that translation is enabled for this bundle.

It also doesn't look like #78.3 was fixed, according to the interdiff...

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@phenaproxima I don't necessarily agree that it is superfluous to add that given there are different translation options (if entity translation is also enabled). I think there were still outstanding things in #82, so moving to needs review for that.

Gábor Hojtsy’s picture

83 did not apply anymore to D7NodeDeriver, so here is a reroll.

Gábor Hojtsy’s picture

As per the migrate meeting, @phenaproxima suggested we have a list of the potential values for that constant instead. Here it goes.

Gábor Hojtsy’s picture

@phenaproxima: re #78.3, I think that was resolved in #82, just look at the patch itself. The interdiff appears to be rolling back the file entirely but the patch has the file as a copy/rename of another file and has limited changes there. I guess git decided to optimize that diff :D

So I think #78.4 and the response to it in #82 are the only thing still to be done?

The last submitted patch, 86: 2669964-86.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 87: 2669964-87.patch, failed testing.

Gábor Hojtsy’s picture

Duh, I rolled against 8.3, of course it did not apply to 8.3.x. Should this be against 8.2 still primarily though?

quietone’s picture

Status: Needs work » Needs review
FileSize
42.99 KB
931 bytes

I've added Gábor Hojtsy changes in #87 to the patch in #83 to test against 8.2.x.

Yes, #78.4 still needs to be done.

quietone’s picture

How is this for #78.4?

Status: Needs review » Needs work

The last submitted patch, 93: 2669964-93.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
43.06 KB
1.88 KB

Fix typo.

Ignore this interdiff, the correct one is in the next comment.

quietone’s picture

FileSize
838 bytes

This is the correct interdiff for the typo fix.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I looked it over and nothing problematic leaps out at me. All makes sense, and seems pretty nicely tested. Except this one tiny thing:

+++ b/core/modules/node/tests/src/Kernel/Plugin/migrate/source/d7/NodeTranslationTest.php
@@ -0,0 +1,70 @@
+  public function providerSource() {
+
+    // Get the source data from parent.

Nit: There's an extra blank line here. Can be fixed on commit.

Gábor Hojtsy’s picture

That's too easy ;)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

With the tnid migrated to nid, what happens with node references/entity references that point to the old nid? Does this need a follow-up? Or should it be addressed here (assuming it's not).

I also opened another follow-up here #2850085: Redirects for translation set migration path in Drupal 6 and 7 - doesn't need fixing with this issue but don't think I've seen it covered elsewhere.

Gábor Hojtsy’s picture

@catch: Drupal 6 had the same problem, we have #2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs as a META for that, given that path aliases, comments posted on nodes, node ratings, nodequeues, whatnot would all need to react on this migration (additionally to entity references that you mentioned). We can make that meta apply to both Drupal 6 and 7 or open another one for 7.

Path aliases in particular have been resolved in Drupal 6 with #2827644: Fix path alias migration of translated nodes [D6] and would be very similar for Drupal 7. I think we don't need to open every possible followup for the changed IDs because we don't know the extent of the problem and it applies to contrib migrations as well. I can expand #2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs as a meta for both Drupal 6 and 7.

Gábor Hojtsy’s picture

Updated #2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs to deal with both Drupal 6 and 7 and @catch moved it to critical.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs a re-roll for 8.4.x:

error: patch failed: core/modules/node/src/Plugin/migrate/D7NodeDeriver.php:65
error: core/modules/node/src/Plugin/migrate/D7NodeDeriver.php: patch does not apply

Gábor Hojtsy’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Reviewed & tested by the community
FileSize
43 KB

Straight reroll against 8.4.x. It was just a comment moved and updated in 8.4.x that made this not apply.

  • catch committed 69c0ead on 8.4.x
    Issue #2669964 by quietone, Gábor Hojtsy, Jo Fitzgerald, phenaproxima:...

  • catch committed e9d7b6d on 8.3.x
    Issue #2669964 by quietone, Gábor Hojtsy, Jo Fitzgerald, phenaproxima:...
catch’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

xjm’s picture

Issue tags: +8.3.0 release notes

Let's mention this one in the release notes/changelog since it's not shipping in any patch release and it is a big deal.

Status: Fixed » Closed (fixed)

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