Problem/Motivation

Even after we get the last couple multilingual issues resolved, there are still many unknowns and multilingual areas could get improved. Rather than rushing and stating all multilingual migrations are ready because we just need to get things out the door, let’s take off the pressure. Give us a little more time to do things properly. Having a separate marking module let’s us mark multilingual stable when it is really more stable.

Proposed resolution

  • Add a new migrate_drupal_multilingual module
  • In all i18n and entity translation migrations add a tag 'Multilingual'
  • In checkRequirements() check if the 'Multilingual' tag exists and if so add a requirement that the 'migrate_drupal_multilingual' module is enabled.
  • If the module doesn’t exist (i.e. it isn’t installed) then it fails requirements and will not be available for migrations.
  • Then mark migrate_drupal_multilingual as experimental. And mark migrate_drupal as stable.
  • Profit

The original proposed resolution was limited to i18n migrations and for those, the i18n source plugins would have a requirement of the new module. It was then pointed out in #78 that entity translations also needed to be included. After a migrate meeting it was proposed to add a tag for entity translation, see #80. Then in both #81> and #86 it was questioned why two methods (testing the source plugin and using a tag) was necessary. Finally, in #87 the use of just the tag was agreed and implemented.

https://www.drupal.org/project/drupal/issues/2953360#comment-12652339

Remaining tasks

  • Roll a patch.
  • Discuss if this can land as Experimental into 8.6. That way we could mark migrate drupal and the UI as stable sooner.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#157 2953360-157.patch738 bytesjcnventura
#145 interdiff_141-145.txt1.45 KBheddn
#145 2953360-145.patch47.79 KBheddn
#141 2953360-141.patch46.94 KBquietone
#141 interdiff.txt1.38 KBquietone
#137 interdiff.txt1.41 KBquietone
#137 2953360-137.patch46.98 KBquietone
#134 interdiff.txt20.38 KBquietone
#134 2953360-134.patch46.98 KBquietone
#132 interdiff-128-131.txt36.96 KBquietone
#131 2953360-131.patch46.93 KBquietone
#128 interdiff-127-128.txt576 bytesquietone
#128 2953360-128.patch60.85 KBquietone
#127 2953360-127.patch60.83 KBquietone
#127 interdiff-124-127.txt12.19 KBquietone
#124 interdiff-122-124.txt2.5 KBquietone
#124 2953360-124.patch59.07 KBquietone
#122 interdiff-119-122.txt1.34 KBquietone
#122 2953360-122.patch59.41 KBquietone
#120 interdiff-117-119.txt1.35 KBquietone
#120 2953360-119.patch59.4 KBquietone
#118 interdiff-115-117.txt5.08 KBquietone
#118 2953360-117.patch59.08 KBquietone
#115 interdiff-112-115.txt2.01 KBquietone
#115 2953360-115.patch59.25 KBquietone
#112 interdiff-110-112.txt14.62 KBquietone
#112 2953360-112.patch59.45 KBquietone
#110 2953360-110.patch46.88 KBquietone
#110 interdiff-108-110.txt1.01 KBquietone
#108 interdiff-105-108.txt14.94 KBquietone
#108 2953360-108.patch46.81 KBquietone
#105 interdiff-102-105.txt10.36 KBquietone
#105 2953360-105.patch35.98 KBquietone
#102 2953360-102.patch35.09 KBquietone
#102 interdiff-100-102.txt6.04 KBquietone
#100 interdiff-98-100.patch10.09 KBquietone
#100 2953360-100.patch34.37 KBquietone
#100 2953360-100-fail.patch33.4 KBquietone
#99 interdiff-93-98.txt1.35 KBquietone
#98 2953360-98.patch26.93 KBquietone
#93 interdiff.txt3.37 KBquietone
#93 2953360-93.patch25.58 KBquietone
#91 interdiff-90-91.patch3.78 KBquietone
#91 2953360-91.patch22.21 KBquietone
#90 2953360-90.patch19.76 KBquietone
#88 2953360-88.patch18.51 KBquietone
#85 interdiff.txt587 bytesquietone
#85 2953360-85.patch18.12 KBquietone
#83 interdiff.txt2.78 KBquietone
#83 2953360-83.patch17.84 KBquietone
#81 interdiff.txt10.87 KBquietone
#81 2953360-81.patch16.12 KBquietone
#71 interdiff.txt1.68 KBquietone
#71 2953360-71.patch14.5 KBquietone
#69 2953360-69.patch16.17 KBquietone
#59 interdiff-2953360-56-59.txt1.03 KByogeshmpawar
#59 2953360-59.patch17.44 KByogeshmpawar
#56 interdiff.txt683 bytesquietone
#56 2953360-56.patch16.33 KBquietone
#54 interdiff.txt690 bytesquietone
#54 2953360-54.patch16.61 KBquietone
#52 interdiff.txt572 bytesquietone
#52 2953360-52.patch16.68 KBquietone
#47 interdiff.txt644 bytesquietone
#47 2953360-47.patch14.56 KBquietone
#35 interdiff.txt3.85 KBquietone
#35 2953360-35.patch13.93 KBquietone
#33 2953360-33.patch12.2 KBquietone
#31 2953360-31.patch12.2 KBquietone
#29 interdiff-21-29.txt6.84 KBquietone
#29 2953360-29.patch12.2 KBquietone
#25 interdiff.txt676 bytesquietone
#25 2953360-25.patch48.87 KBquietone
#21 2953360-21.patch13.45 KBquietone
#21 interdiff.txt903 bytesquietone
#19 interdiff.txt1.35 KBquietone
#19 2953360-19.patch13.17 KBquietone
#16 2953360-16.patch12.36 KBquietone
#16 2953360-16-fail.patch7.89 KBquietone
#13 interdiff.txt1.3 KBquietone
#13 2953360-13.patch20.02 KBquietone
#11 interdiff.txt3.73 KBquietone
#11 2953360-11.patch18.31 KBquietone
#9 2953360-9.patch14.2 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

This approach was discussed on several calls within the Migrate maintainers and the core committers. Since it means we can unblock migrate_drupal and the UI, it received the go-ahead. For that reason, I'm not tagging as needing various approvals.

heddn’s picture

Issue summary: View changes
heddn’s picture

We should also plan to not add any logic into this experimental module so we can easily remove it if/when we decide to remove the requirements check.

quietone’s picture

I agree with #4, this should be a module that is removed when the i18n migrations are complete.

As maxocub pointed out on the node translation migrations use the same source plugin as the node migrations. I was curious how many other migration there are that share a source plugin with a non translation migration and this is what I found.

migration source plugin
d6_taxonomy_term_translation.yml d6_taxonomy_term
d6_node_translation.yml d6_node
d7_node_translation.yml d7_node
heddn’s picture

Priority: Normal » Critical

This issue would unblock stability of migrate_drupal. So bumping to critical as per agreement with committers anything blocking stability for migrate is critical.

catch’s picture

I agree with #4, this should be a module that is removed when the i18n migrations are complete.

I don't think we can remove it, or at least it would have to be two stage - i.e. an update to uninstall it, mark it hidden in one minor, then in the next minor release remove it from the code base.

quietone’s picture

Assigned: Unassigned » quietone

A search for migrations with 'translation' in the name return these:

  1. ./core/modules/config_translation/migrations/d6_user_profile_field_instance_translation.yml
  2. ./core/modules/config_translation/migrations/d6_user_mail_translation.yml
  3. ./core/modules/config_translation/migrations/d6_user_settings_translation.yml
  4. ./core/modules/config_translation/migrations/d6_system_maintenance_translation.yml
  5. ./core/modules/config_translation/migrations/d6_system_site_translation.yml
  6. ./core/modules/content_translation/migrations/d6_taxonomy_term_translation.yml
  7. ./core/modules/menu_link_content/migrations/node_translation_menu_links.yml *
  8. ./core/modules/node/migrations/d6_node_translation.yml
  9. ./core/modules/node/migrations/d7_node_translation.yml
  10. ./core/modules/statistics/migrations/statistics_node_translation_counter.yml *
  11. ./core/modules/taxonomy/migrations/d6_taxonomy_vocabulary_translation.yml

1-5 and 11 use a unique translation source plugin and are therefor easy to identify. 6-10 Share a source plugin with a non translation migration. Of those 6, 8 and 9 use the key 'translations' in the source plugin so they can be identified. That leaves 7, menu_link, and 10, node_counter, with nothing in the source plugin or the source configuration to identify that it is being used in an i18n migration.

Since 'translations: true' is already in use we could add it to the source configurations for translation migrations as needed. Then checkRequirements can check if migrate_drupal_i18n is enabled when the source plugin is one of the shared ones (d6_node, d7_node, menu_link, node_counter, d6_term) and 'translations' is set or it is not one of the shared ones. is there a better way to do this?

Assigning to myself as I'm partly through making a patch.

quietone’s picture

Assigned: quietone » Unassigned
Status: Active » Needs review
FileSize
14.2 KB

I'm not too keen on having a static list of modules but let's make a start.

All the source plugins used for translations use a new checkRequirements method that will throw an exception if migrate_drupal_i18n is not installed. If the source plugin is used in a translation migration and a non-translation migration then translation migration must have 'translations: true'.

Status: Needs review » Needs work

The last submitted patch, 9: 2953360-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
18.31 KB
3.73 KB

Enable migrate_drupal_i18n in more tests.

Status: Needs review » Needs work

The last submitted patch, 11: 2953360-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
20.02 KB
1.3 KB

Still missed some tests. And add migrate_drupal_i18n to core/composer.json.

Status: Needs review » Needs work

The last submitted patch, 13: 2953360-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

+++ b/core/modules/migrate_drupal_i18n/src/Plugin/migrate/source/RequirementsTranslationTrait.php
@@ -0,0 +1,43 @@
+      if (isset($this->configuration['translations']) || (!in_array($this->pluginId, static::$sharedSourcePlugin))) {

Surely we don't require i18n if someone is migrating d6 nodes and doesn't have i18n content on the source?

quietone’s picture

Status: Needs work » Needs review
FileSize
7.89 KB
12.36 KB

Took a tangent that included all translations. This now sticks to just the i18n ones. There is also a fail patch to show the i18n migrations fail when migrate_drupal_i18n is not enabled. I haven't run all the tests locally so let's see what happens

The last submitted patch, 16: 2953360-16-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 16: 2953360-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
13.17 KB
1.35 KB

Add hook help.

Status: Needs review » Needs work

The last submitted patch, 19: 2953360-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
903 bytes
13.45 KB

Seems it needs a link to documentation. I'm having trouble running this test locally, it can use up all the disk space allocated to the container.

quietone’s picture

Now there is a passing test and a failing test, #16. That leaves creating documentation and updating the module link.

+++ b/core/modules/migrate_drupal_i18n/migrate_drupal_i18n.module
@@ -0,0 +1,26 @@
+          [':migrate' => 'https://www.drupal.org/upgrade/migrate']) . '</p>';

Needs a link to new documentation

quietone’s picture

tagging for documentation

heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6ReviewPagei18nTest.php
@@ -2,30 +2,22 @@
+   * Modules to enable.
+   *
+   * @var array

inheritdoc here.

quietone’s picture

Status: Needs work » Needs review
FileSize
48.87 KB
676 bytes

Fix for #24

quietone’s picture

Where to put the necessary documentation? A note on the known issues page?

Maybe something like this:

Internationalization Translations (i18n)
If you are upgrading a site with internationalization enabled you must enable the Migrate Drupal i18n module on your new site.

heddn’s picture

Status: Needs review » Needs work

The last submitted patch, 25: 2953360-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
12.2 KB
6.84 KB

Looks like I wasn't working from HEAD when I made the last patch. So, starting over from the patch in #21.

Status: Needs review » Needs work

The last submitted patch, 29: 2953360-29.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
12.2 KB

Needed a reroll.

Status: Needs review » Needs work

The last submitted patch, 31: 2953360-31.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
12.2 KB

Oops, try again.

Status: Needs review » Needs work

The last submitted patch, 33: 2953360-33.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
13.93 KB
3.85 KB

Fixed a comment and hopefully the tests.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Let's see what happens when this goes to RTBC.

alexpott’s picture

Issue tags: +Needs change record

I think given that suddenly working migrations will not be there I think we should have a change record.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php
@@ -103,6 +103,15 @@ public static function create(ContainerInterface $container, array $configuratio
+      // For i18n translations migrate_drupal_i18n must be enabled.
+      if (strpos($this->getSourceModule(), 'i18n') !== FALSE) {
+        if (!\Drupal::service('module_handler')
+          ->moduleExists('migrate_drupal_i18n')) {
+          throw new RequirementsException("The module migrate_drupal_i18n is not enabled on the new site.");
+        }
+      }

Are we concerned about this suddenly breaking custom migrations?

heddn’s picture

Status: Reviewed & tested by the community » Needs work

Back to NW for CR. Breaking existing migrations is a BC break. However, it also lets us mark migrate drupal as stable, so I think it is worth it to solve a critical release blocker.

quietone’s picture

Draft change record started. It is pretty terse, what more needs to be added?

heddn’s picture

Let's add the implications to the CR w/ some examples. I'd add the examples myself, but I don't know them off the top of my head. It also might help to give some example yml files i.e. migrations that this will effect. And explain the reason why the module is introduced and how it should (obviously) be enabled to have access to all the i18n migrations again.

masipila’s picture

Regarding the relevant handbook pages.

Let's mention this in both the Known Issues page
https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d...

and on the Preparing an upgrade page
https://www.drupal.org/docs/8/upgrade/preparing-a-site-for-upgrade-to-dr...

I've been working my butt of lately so haven't been able to contribute as much as I would like to :(

Cheers,
Markus

quietone’s picture

@heddn, thanks. And ARG! I am not a writer but I have updated the CR with all that you suggest.

heddn’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -drupal.org documentation, -Needs change record

Removing tags, we have d.o docs (see below) and a change record. Back to RTBC?

For https://www.drupal.org/docs/8/upgrade/known-issues-when-upgrading-from-d..., create a new target called: Internationalization Module (i18n) upgrades.

For https://www.drupal.org/docs/8/upgrade/preparing-a-site-for-upgrade-to-dr..., directly after the first bullet.

Use the following wording for those sections:
All translation migrations from the Drupal 6 and Drupal 7 Internationalization Module (i18n) now require that the Migrate Drupal i18n Module (migrate_drupal_i18n) is installed on the Drupal 8 site.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think we need a hook_update_N to install the new module if migrate_drupal is installed. That's because we shouldn't break existing sites.

quietone’s picture

Don't know about hook_update. Will this go into migrate_drupal_i18n? Will it run when migrate_drupal_i18n is not enabled?

alexpott’s picture

@quietone it would go in migrate_drupal... basically if that is already enabled on a site then we should install migrate_drupal_i18n. hook_update()s are run on new installs so if you install migrate_drupal after it has been added you won't get migrate_drupal_i18n installed.

quietone’s picture

Status: Needs work » Needs review
FileSize
14.56 KB
644 bytes

@alexpott, thanks. As usual after I wrote that it all started to make sense.

I've been all thumbs making this patch, let's hope it is something useful.

quietone’s picture

The documentation pages referenced in #43 have been updated. That includes a phrase that installing this module becomes a requirement when this is fixed. Of course, that needs to be removed when this is comitted.

heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+++ b/core/modules/migrate_drupal/migrate_drupal.install
@@ -14,3 +14,10 @@ function migrate_drupal_update_8501() {
+ * Install migrate_drupal_i18n if migrate_drupal is installed.

Super nit: Let's drop the if and say 'since migrate_drupal is installed'. And we probably need a test of the update hook, no?

heddn’s picture

Issue tags: -Needs tests +Needs reroll, +Novice

Considering the easiness to change the one line in the comment, and needing a re-roll... tagging as Novice. However, better act quick if you want your name on this issue because this is also critical and will probably get picked up by someone soon.

heddn’s picture

Also, dropping the tests tag. There's not much to test here and I was just trying to read a committer's mind. It probably isn't necessary.

quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.68 KB
572 bytes

Reroll and fix together in one patch, since it was a one line fix.

Status: Needs review » Needs work

The last submitted patch, 52: 2953360-52.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
16.61 KB
690 bytes

Now update the test.

Status: Needs review » Needs work

The last submitted patch, 54: 2953360-54.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
16.33 KB
683 bytes

Adjust entity count.

Status: Needs review » Needs work

The last submitted patch, 56: 2953360-56.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
17.44 KB
1.03 KB

Maybe this patch will solve the test fails & also added an interdiff.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Shall we all say RTBC together? Great work everyone.

maxocub’s picture

Issue tags: -Novice +Migrate critical

Tagging to reflect that this is a (the only remaining) blocker for Migrate Drupal stability: #2905736: Mark Migrate Drupal as stable.

maxocub’s picture

Issue tags: +Migrate Drupal

Forgot a tag.

catch’s picture

This looks great. I think we need a follow-up to remove the dependency, disable the module, hide it from the UI, when migrate is fully stable, then a further follow-up to remove it altogether.

catch’s picture

Also another question:

- all the known entity translation migrations are actually committed now, so does this need to be experimental except for the possibility of unforeseen bugs?

heddn’s picture

catch’s picture

Correction to #64, all the ones tagged migrate critical are done, but there is still #2073467: Migrate Drupal 7 Entity Translation settings to Drupal 8 and possibly others outstanding.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: 2953360-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Issue tags: +i18n-migrate

This should also be tagged i18n-migrate.

quietone’s picture

Status: Needs work » Needs review
FileSize
16.17 KB

This needed a reroll.

Status: Needs review » Needs work

The last submitted patch, 69: 2953360-69.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
14.5 KB
1.68 KB

Adjust the tests for recent changes.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Gábor Hojtsy’s picture

We discussed this on the core committer call just now with @webchick, @alexpott, @effulgentsia, @larowlan and myself in attendance. Nobody had objections to go this way and we acknowledged the value this would bring in providing clarity and stability for the Drupal upgrade path and migrations in general.

Since there are various other core committers who were not there (notably neither release managers unfortunately), leaving this open for their chance to follow up.

Gábor Hojtsy’s picture

BTW said committer meeting publicly documented at #2978215: [Core Committers] Meeting notes 2018/06/07

catch’s picture

I'm happy with this one going in. There was a moment where we thought all the i18n patches had landed, but some entity translation migrations came out of the woodwork since so I think it makes sense to do this.

We still need a follow-up for #63 though - this module can't just be nuked once we don't need it, it needs to be uninstalled on every single site it's on, prevented from being reinstalled, then removed either in an even later 8.x release or in 9.x, so two follow-ups in practice.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
maxocub’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs followup
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

All right, reviewed the code then. This snippet is interesting because we have a bunch of source modules that would be multilingual, eg. content translation, i18n_*, entity_translation, etc. some of which are not done / complete yet. Would those still have i18n as a source module even if there is no requirement for i18n (such as in the case of entity_translation?). Would it be a more generic way to name the module "Migrate Drupal Multilingual" or so with a matching machine name since i18n is not an exclusive source module?

+      // For i18n translations migrate_drupal_i18n must be enabled.
+      if (strpos($this->getSourceModule(), 'i18n') !== FALSE) {
+        if (!\Drupal::service('module_handler')
maxocub’s picture

That's true, Multilingual core migrations are not limited to i18n. We need to find another way.

heddn’s picture

Discussed in maintainers meeting today with @maxocub and @masipila and @Gábor Hojtsy:
The consensus was to: use either a tag in the migration (for entity translation) or the source module for i18n (currently).

quietone’s picture

Status: Needs work » Needs review
FileSize
16.12 KB
10.87 KB

As per #80, this adds a tag, 'Multilingual', to identify entity translation migrations and the tag is tested for DrupalSqlBase::checkRequirements(). And the name of module has changed from migrate_drupal_i18n to migrate_drupal_multilingual, as suggested in #78.

Just one question, why not use the tag for both the i18n and the entity translations?

Status: Needs review » Needs work

The last submitted patch, 81: 2953360-81.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
17.84 KB
2.78 KB

Change the requirements test to handle a NULL value for tags. That may only be the test that has a NULL value as $migration_tags is defined as an empty array but it may prevent an error in the future.

Moved 'entity_translation' to the missing paths lists in the MigrateUpgrade7 tests.

Status: Needs review » Needs work

The last submitted patch, 83: 2953360-83.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
18.12 KB
587 bytes

Adjust language content settings entity count because d7_entity_translation isn't running.

Gábor Hojtsy’s picture

Title: Experimental migrate_drupal_i18n module » Experimental migrate_drupal_multilingual module
Issue summary: View changes
Issue tags: +Needs issue summary update

Updated title and issue summary of issue, but the proposed resolution still needs updating. Why do we need to check both the tag and the module name? I would expect if the module name is i18n that should also has the tag?

heddn’s picture

Status: Needs review » Needs work

Back to NW for #86. Let use the tag everywhere seems to be the concern. Instead of checking the source module is i18n.

quietone’s picture

Status: Needs work » Needs review
FileSize
18.51 KB

This needs a reroll, starting with that first.

Status: Needs review » Needs work

The last submitted patch, 88: 2953360-88.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
19.76 KB

Enable 'migrate_drupal_multilingual' for the two recently committed test.

quietone’s picture

I'm assuming that will pass. Here are the changes for #87

Status: Needs review » Needs work

The last submitted patch, 91: interdiff-90-91.patch, failed testing. View results

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
25.58 KB
3.37 KB

Enabled migrate_drupal_multilingual to MigrateTaxonomyTermTranslationTest and added the new tag to more i18n migrations. And updated the proposed resolution in the IS.

The following are migrations handling translations (has 'translation' in the id).

Tagged 'Multilingual'

  • d6_block_translation.yml
  • d6_custom_block_translation.yml
  • d6_menu_links_translation.yml
  • d6_taxonomy_term_translation.yml
  • d7_entity_translation_settings.yml
  • d6_taxonomy_vocabulary_translation.yml
  • d6_user_profile_field_instance_translation.yml
  • d6_user_mail_translation.yml
  • d6_user_settings_translation.yml
  • d6_system_maintenance_translation.yml
  • d6_system_site_translation.yml

Not tagged 'Multilingual' because these haven't been discussed in the issue.

  • node_translation_menu_links.yml
  • d6_entity_reference_translation.yml
  • d7_entity_reference_translation.yml
  • d6_node_translation.yml
  • d7_node_translation.yml
  • statistics_node_translation_counter.yml

I did add the new tag to 'd6_node_translation' and ran MigrateNodeTest.php to see what would happen. And it fails with 'Migration d6_node_translation:article did not meet the requirements. The module migrate_drupal_multilingual is not enabled on the new site.
Failed asserting that false is true.' Do we want to tag these as well?

Question: What is the name of the new tag? It is currently 'Multilingual' since it covers both i18n and entity. But then, it exclude the node translations.

webchick’s picture

We're bumping right up into 8.6.0 feature freeze in a few days... is it about time to get this sucker to RTBC? Or are we close with the i18n fixes?

maxocub’s picture

Sadly, we are not close to fixing all i18n issues. This issue should be our main priority.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Why do we exclude the node translations and entity translations? I would expect all translation stuff under here if this is the multilingual module.

maxocub’s picture

Discussed on slack with @phenaproxima and @heddn.

The goal of this issue was to keep multilingual migration as experimental and to mark Migrate Drupal as stable. Unfortunately, we don't feel ready to go stable until #2631698: Fix sub-optimal DX in MigrateFieldInterface gets in to fix some bad method names in Migrate Drupal API.

quietone’s picture

Status: Needs work » Needs review
FileSize
26.93 KB

This adds the multilingual tag to just d6_entity_reference_translation.yml and d7_entity_reference_translation.yml. And then I expected the test of those migrations, FollowUpMigrationTest, to fail but it doesn't. I'll have to look into what the EntityReferenceTranslationDeriver actually does.

quietone’s picture

FileSize
1.35 KB

Here is the interdiff

quietone’s picture

Just needed to move the requirements check from the DrupalSqlBase::checkRequirements() to Migration::checkRequirements() which is where it should have been for checking tags. Now all 17 of the translation migrations have the Multilingual tag. I'm not sure that all the tests needing the new module have been updated yet but it is time to stop. Adding a fail test as well.

Status: Needs review » Needs work

The last submitted patch, 100: interdiff-98-100.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
6.04 KB
35.09 KB

A few improvements.

  • Set d6_taxonomy_term_translation destination_module to content_translation and d6_taxonomy_vocabulary_translation.yml to config_translation, this is perhaps out of scope but the display is correct and it's a bit easier to work with.
  • Enabled migrate_drupal_multilingual in the d6 and d7 FollowUpMigrationTest, so they should be working now.
  • Adjusted the Available and Missing paths.

Status: Needs review » Needs work

The last submitted patch, 102: 2953360-102.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Instead of putting the validation in Migration.php I think it should be in MigrationConfigurationTrait::checkRequirements and it has a todo.

        // @todo https://drupal.org/node/2681867 We should be able to validate
        //   the entire migration at this point.

So, I have made a patch for that over in #2681867: Drupal Upgrade UI should validate the entire migration, not the source and destination individually.

Now that many migrations are not available for the full migration test it is making updating the MigrateUpgrade tests challenging.

quietone’s picture

Status: Needs work » Needs review
FileSize
35.98 KB
10.36 KB

Apologies, I've only got time to upload the patch now. I'll come back later to comment more fully.

Status: Needs review » Needs work

The last submitted patch, 105: 2953360-105.patch, failed testing. View results

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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

Version: 8.7.x-dev » 8.6.x-dev
Status: Needs work » Needs review
FileSize
46.81 KB
14.94 KB

Still a work in progress. No need to review. I just want to see what the testbot finds.

Status: Needs review » Needs work

The last submitted patch, 108: 2953360-108.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
46.88 KB

This should fix the test, although not yet ready for a review. I hope to find time tonight to review it and do some cleanup and add comments.

quietone’s picture

No patch this time but a summary of the changes so far.

  • Multilingual tag is on all 17 migrations related to translations, that include i18n, entity and node translations.
  • Added destination_module to d6_taxonomy_term_translation. Potentially out of scope but it means the correct D8 modules is displayed on the MigrateUpgrade Review form. And it did help with testing to see the correct thing displayed.
  • The requirement check for the multilingual module is in MigrationConfigurationTrait.
  • There is a new test MigrateUpgrade6I18nTest.php that tests the full migration without the translations enabled.

TODO:
Create a full migration test for D7, MigrateUpgrade7I18nTest
Coding standard fixes
check the test using external_translated_test_node_translation.yml to see if it requires changes.

quietone’s picture

Add a test for Drupal 7 upgrade without translation modules enabled.

quietone’s picture

Issue summary: View changes

Remove a duplicate block of text from the IS

Status: Needs review » Needs work

The last submitted patch, 112: 2953360-112.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
59.25 KB
2.01 KB

Fix entity count in the MigrateUpgrade7I18nTest and reverse two introduced mistakes.

Status: Needs review » Needs work

The last submitted patch, 115: 2953360-115.patch, failed testing. View results

phenaproxima’s picture

Found some nits, but I think I need to read the tests more thoroughly.

  1. +++ b/core/modules/migrate_drupal/migrate_drupal.install
    @@ -24,3 +24,11 @@ function migrate_drupal_update_8502() {
    +  $moduleInstaller = \Drupal::service('module_installer');
    +  $moduleInstaller->install(['migrate_drupal_multilingual']);
    

    No need for this to be two lines. Let's just collapse it into \Drupal::service('module_installer')->install(['migrate_drupal_multilingual']);

  2. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
    @@ -113,6 +113,13 @@ protected function getMigrations($database_state_key, $drupal_version) {
    +        // Multilingual migrations require the module migrate_drupal_multilingual.
    

    Nit: Longer than 80 characters. So let's just axe the words "the module". :)

  3. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
    @@ -113,6 +113,13 @@ protected function getMigrations($database_state_key, $drupal_version) {
    +        $tags = $migration->getMigrationTags() ? $migration->getMigrationTags() : [];
    

    This should be $tags = $migration->getMigrationTags() ?: [].

  4. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
    @@ -113,6 +113,13 @@ protected function getMigrations($database_state_key, $drupal_version) {
    +        if (in_array('Multilingual', $tags) && (!\Drupal::service('module_handler')->moduleExists('migrate_drupal_multilingual'))) {
    

    Let's pass TRUE as the third parameter to in_array().

  5. +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.info.yml
    @@ -0,0 +1,8 @@
    +description: 'Provides a requirement for entity and i18n migrations.'
    

    Can we remove "entity"? That's a bit vague.

  6. +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.info.yml
    @@ -0,0 +1,8 @@
    +dependencies:
    +  - migrate
    +  - migrate_drupal
    

    migrate_drupal depends on migrate already, so we can remove that.

  7. +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.module
    @@ -0,0 +1,21 @@
    +      $output .= '<p>' . t('The Migrate Drupal i18n module is a requirement for migrating entity and i18n translations. It does not provide a user interface. For more information, see the <a href=":migrate_drupal_multilingual">online documentation for the Migrate Drupal Multilingual module</a>.', [':migrate_drupal_multilingual' => 'https://www.drupal.org/docs/8/core/modules/experimental-migrate-drupal-i18n']) . '</p>';
    

    I think we can remove "entity and i18n".

  8. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6I18nTest.php
    @@ -0,0 +1,357 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    

    This should be {@inheritdoc}.

  9. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6I18nTest.php
    @@ -0,0 +1,357 @@
    +  public function testMigrateUpgradeExecute() {
    +
    +    $connection_options = $this->sourceDatabase->getConnectionOptions();
    

    Nit: Empty line.

  10. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
    @@ -83,7 +85,7 @@ protected function getEntityCounts() {
    -      'tour' => 4,
    +      'tour' => 5,
    

    Why did this entity count change?

  11. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7I18nTest.php
    @@ -0,0 +1,360 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    

    This should be {@inheritdoc}.

  12. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7I18nTest.php
    @@ -0,0 +1,360 @@
    +  public function testMigrateUpgradeExecute() {
    +
    +    $connection_options = $this->sourceDatabase->getConnectionOptions();
    

    Nit: Empty line.

quietone’s picture

Status: Needs work » Needs review
FileSize
59.08 KB
5.08 KB

1-9 and 11-12 fixed
10. I presume because config_translation is installed and it wasn't before.

Status: Needs review » Needs work

The last submitted patch, 118: 2953360-117.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
59.4 KB
1.35 KB

Fix a copy paste error with the admin password for the new tests.

Status: Needs review » Needs work

The last submitted patch, 120: 2953360-119.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
59.41 KB
1.34 KB

This should fix the tests.

This introduces two new full migration tests, MigrateUpgrade6I18nTest and MigrateUpgrade7I18nTest, which are based on MigrateUpgrade6Test and MigrateUpgrade7Test respectively and thus a lot of boilerplate code. I suggest that any comments regarding reducing/improving the structure of those test be done in the existing issue to re-factor the base test class, #2974445: [Meta] Refactor Migrate Drupal UI Functional tests.

The new tests do the full test, migrate, then incremental, then follow-up migrations. The difference between these tests and the MigrateUpgrade6Test and MigrateUpgrade7Test is that they do not enable the translation modules. Yes, the name has I18n name but it does not install the translation module. I did not change that because there is already MigrateUpgrade6I18nReviewTest which is the same, it doesn't install translation modules. I reckon it is better to be consistent in the naming to avoid confusion. Perhaps a follow up to rename all the tests and base classes is needed?

phenaproxima’s picture

  1. +++ b/core/lib/Drupal.php
    @@ -82,7 +82,7 @@ class Drupal {
    -  const VERSION = '8.6.0-dev';
    +  const VERSION = '8.6.0-alpha1';
    

    This snippet is probably a git snafu and should be reverted :)

  2. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
    @@ -113,6 +113,13 @@ protected function getMigrations($database_state_key, $drupal_version) {
    +        // Multilingual migrations require migrate_drupal_multilingual.
    +        $tags = $migration->getMigrationTags() ?: [];
    +        if (in_array('Multilingual', $tags, TRUE) && (!\Drupal::service('module_handler')->moduleExists('migrate_drupal_multilingual'))) {
    +          throw new RequirementsException("The module migrate_drupal_multilingual is not enabled on the new site.");
    +        }
    

    Should this be *outside* the try block? I've never seen an exception thrown directly inside a try block before.

  3. +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.module
    @@ -0,0 +1,21 @@
    + * A requirement for migrating i18n translations.
    

    Let's rephrase: "Provides a requirement for multilingual content and configuration migrations."

  4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6I18nTest.php
    @@ -0,0 +1,354 @@
    +/**
    + * Tests Drupal 6 upgrade via the UI without translations.
    + *
    + * The test method is provided by the MigrateUpgradeTestBase class.
    + *
    + * @group migrate_drupal_ui
    + */
    +class MigrateUpgrade6I18nTest extends MigrateUpgradeExecuteTestBase {
    

    The fact that this class has the word "i18n" in it, yet does not involve translations, is *incredibly* confusing. In the interest of time, and since this is a test (and therefore eligible to be changed at any time), I don't mind opening a follow-up to correct this. But we should definitely fix it as soon as possible.

  5. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6I18nTest.php
    @@ -0,0 +1,354 @@
    +    $this->drupalPostForm(NULL, [], t('I acknowledge I may lose data. Continue anyway.'));
    

    drupalPostForm() is harder to grok than just $assert->buttonExists('I acknowledge I may lose data. Continue anyway.')->press().

  6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7I18nTest.php
    @@ -0,0 +1,358 @@
    +/**
    + * Tests Drupal 7 upgrade via the UI without translations.
    + *
    + * The test method is provided by the MigrateUpgradeTestBase class.
    + *
    + * @group migrate_drupal_ui
    + */
    +class MigrateUpgrade7I18nTest extends MigrateUpgradeExecuteTestBase {
    

    Here too, I don't think we should name a class "*i18n*" if it doesn't involve translations.

quietone’s picture

Thanks phenaproxima.

1,2,3 - Fixed,
4,6 - Follow up to rename tests classes #2987418: Rename MigrateUpgrade tests.
5. These are really doing a post not asserting that a button exists on the display. Are you suggesting adding buttonExists?

phenaproxima’s picture

These are really doing a post not asserting that a button exists on the display. Are you suggesting adding buttonExists?

$this->assertSession()->buttonExists('Text of button') both asserts that the button exists, and returns the Mink element for you to manipulate further (in this case, by calling its press() method), whereas drupalPostForm is more obscure, especially when you're already on the page with the form. :)

Status: Needs review » Needs work

The last submitted patch, 124: 2953360-124.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
12.19 KB
60.83 KB

Yes, thanks. I should've done more research before commenting. This patch fixes #123.5, so that is now fixed.

This is more difficult since the scope expanded from i18n only to i18n and entity and now i18n, entity and node translations. There are translation migrations in several modules, config_translation, content_translation, node, statistics and taxonomy (maybe more). So when someone is migrating with the UI and they have enabled node then the node translation migrations are also found and will run, which of course is what we are trying to prevent. This patch takes a simple approach and moves all the translation migrations to either config_translation or content_translation so those migration are only found then the relevant module is installed. But the followup migrations, d6_entity_reference_translation.yml and d7_entity_reference_translation.yml do not have either a Content of Configuration tag, and I don't know the details of the FollowUp process to be sure where to put them. Here, I put them in content_translation.

This patch also adds some context the error message by adding the migration id. I found that useful when I kept missing the migration that had Multilingual added even though it shouldn't have.

The migration tests do not pass locally. I am finished for the day and hoping that after some sleep I'll see why the test(s) are failing.

quietone’s picture

The taxonomy test needs config_translation enabled now.

The last submitted patch, 127: 2953360-127.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 128: 2953360-128.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Almost starting over here. On reflecting I want to step back and just test that the migrations does not run when the translation modules are installed and migrate_drupal_multilingual is not. And that the tests do run when the translation modules are installed and migrate_drupal_multilingual is installed. To do this the 2 poorly named tests have been deleted and 2 new ones added, MigrateUpgrade6NoMultilingualTest and similar for D7. If this passes tests then that should be covered.

Just a note about the deleted tests. They were running the migration without content_translation and config_translation installed which is a completely new test, when the translation migrations started to be added we never made a test without the necessary translation modules. It should work, I did a fresh install and enabled modules as per the test and it ran successfully but in the test environment only about 10 content migrations tried to run and none of the configuration ones. Very odd. Getting that test working isn't necessary here, I think.

quietone’s picture

FileSize
36.96 KB

Here is the interdiff.

phenaproxima’s picture

Patch makes sense, and I'd feel fine about RTBCing it. I just have a few questions...

  1. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d7/MigrateEntityTranslationSettingsTest.php
    @@ -20,6 +20,7 @@ class MigrateEntityTranslationSettingsTest extends MigrateDrupal7TestBase {
    diff --git a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    
    diff --git a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    index 36e4f30dc0..2c920bbdce 100644
    
    index 36e4f30dc0..2c920bbdce 100644
    --- a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    
    --- a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    
    +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTest.php
    @@ -20,6 +20,7 @@ class MigrateMenuLinkTest extends MigrateNodeTestBase {
    
    @@ -20,6 +20,7 @@ class MigrateMenuLinkTest extends MigrateNodeTestBase {
         'content_translation',
         'language',
         'menu_link_content',
    +    'migrate_drupal_multilingual',
         'menu_ui',
       ];
    

    We should probably add an inline comment explaining why this is here (because node_translation_menu_links and d6_node_translation are executed as part of the test).

  2. +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d6/MigrateMenuLinkTranslationTest.php
    @@ -20,6 +20,7 @@ class MigrateMenuLinkTranslationTest extends MigrateDrupal6TestBase {
    diff --git a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
    
    diff --git a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
    index ab21f2c611..219122fb3f 100644
    
    index ab21f2c611..219122fb3f 100644
    --- a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
    
    --- a/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
    +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
    
    +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
    +++ b/core/modules/menu_link_content/tests/src/Kernel/Migrate/d7/MigrateMenuLinkTest.php
    @@ -24,6 +24,7 @@ class MigrateMenuLinkTest extends MigrateDrupal7TestBase {
    
    @@ -24,6 +24,7 @@ class MigrateMenuLinkTest extends MigrateDrupal7TestBase {
         'link',
         'menu_ui',
         'menu_link_content',
    +    'migrate_drupal_multilingual',
         'node',
         'text',
       ];
    

    Same here.

  3. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
    @@ -110,6 +110,12 @@ protected function getMigrations($database_state_key, $drupal_version) {
    diff --git a/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    
    diff --git a/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    index e02254a5e5..46fc15f351 100644
    
    index e02254a5e5..46fc15f351 100644
    --- a/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    
    --- a/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    
    +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    @@ -19,6 +19,7 @@ class FollowUpMigrationsTest extends MigrateNodeTestBase {
    
    @@ -19,6 +19,7 @@ class FollowUpMigrationsTest extends MigrateNodeTestBase {
         'content_translation',
         'language',
         'menu_ui',
    +    'migrate_drupal_multilingual',
       ];
    

    And here.

  4. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FollowUpMigrationsTest.php
    @@ -19,6 +19,7 @@ class FollowUpMigrationsTest extends MigrateNodeTestBase {
    diff --git a/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php b/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    
    diff --git a/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php b/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    index b9b38c2f73..a36db17510 100644
    
    index b9b38c2f73..a36db17510 100644
    --- a/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    
    --- a/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    
    +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    +++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FollowUpMigrationsTest.php
    @@ -26,6 +26,7 @@ class FollowUpMigrationsTest extends MigrateDrupal7TestBase {
    
    @@ -26,6 +26,7 @@ class FollowUpMigrationsTest extends MigrateDrupal7TestBase {
         'language',
         'link',
         'menu_ui',
    +    'migrate_drupal_multilingual',
         'node',
         'taxonomy',
         'telephone',
    

    And here too :)

  5. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
    @@ -81,7 +81,6 @@ class ReviewForm extends MigrateUpgradeFormBase {
    -      'i18n',
           'i18nstrings',
    

    Why is i18n removed, but not i18nstrings?

  6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6I18nReviewPageTest.php
    @@ -59,7 +59,6 @@ protected function getAvailablePaths() {
    @@ -103,7 +102,6 @@ protected function getAvailablePaths() {
    
    @@ -103,7 +102,6 @@ protected function getAvailablePaths() {
           'fieldgroup',
           'filefield_meta',
           'help',
    -      'i18n',
           'i18nstrings',
    

    Same here.

  7. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6NoMultilingualTest.php
    @@ -132,10 +126,6 @@ protected function getAvailablePaths() {
    @@ -161,7 +151,6 @@ protected function getAvailablePaths() {
    
    @@ -161,7 +151,6 @@ protected function getAvailablePaths() {
           'date_api',
           'date_timezone',
           'event',
    -      'i18n',
           'i18nstrings',
    

    And here.

  8. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7NoMultilingualTest.php
    @@ -5,20 +5,19 @@
    +use Drupal\Tests\WebAssert;
    

    I'm not sure this is actually used anywhere?

quietone’s picture

1, 2, 3, 4. Fixed. It uses a very simple comment and I couldn't just comment on a few so I added it all the tests.
5, 6, 7. Yea, probably out of scope (and I though I commented on this but see that I didn't). The short answer is that i18n doesn't belong in the noUpgradePaths list. I have removed that but haven't run all the full migration tests so this may not pass. I'll make a follow for this.
8. Fixed

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Interdiff looks good; RTBC once green on all backends. In my opinion, the benefits of adding this module (i.e., being able to explicitly say that i18n migrations are not stable, which is totally true) outweigh the problems. Hopefully the committers agree.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Only found one minor, but commit-blocking, thing.

  1. +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.module
    @@ -0,0 +1,21 @@
    +      $output .= '<p>' . t('The Migrate Drupal i18n module is a requirement for migrating translations. It does not provide a user interface. For more information, see the <a href=":migrate_drupal_multilingual">online documentation for the Migrate Drupal Multilingual module</a>.', [':migrate_drupal_multilingual' => 'https://www.drupal.org/docs/8/core/modules/experimental-migrate-drupal-i18n']) . '</p>';
    

    This says "Migrate Drupal i18n", but it should be "Migrate Drupal Multilingual".

  2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7NoMultilingualTest.php
    @@ -183,36 +177,37 @@ protected function getMissingPaths() {
    +    // Use the driver connection form to get the correct options out of the
    +    // database settings. This supports all of the databases we test against.
    +    $drivers = drupal_get_database_types();
    +    $form = $drivers[$driver]->getFormOptions($connection_options);
    +    $connection_options = array_intersect_key($connection_options, $form + $form['advanced_options']);
    +    $version = $this->getLegacyDrupalVersion($this->sourceDatabase);
    +    $edit = [
    +      $driver => $connection_options,
    +      'version' => $version,
    +    ];
    +    if (count($drivers) !== 1) {
    +      $edit['driver'] = $driver;
    +    }
    +    $edits = $this->translatePostValues($edit);
    +    $this->drupalPostForm(NULL, $edits, t('Review upgrade'));
    

    It's sad that we have to copy all this stuff across tests, but fixing that now is a bit out of scope. Good thing we have a follow-up filed to improve this.

quietone’s picture

1. Well spotted and fixed.
2. Yes, I look forward to refactoring the upgrade tests.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Restoring RTBC once green on all backends.

quietone’s picture

The followup for #134 5,6,7 has been made, #2988235: Remove i18n from d6 $noUpgradePaths

heddn’s picture

Status: Reviewed & tested by the community » Needs work

Back to NW to update the URL for the module page in hook_help.

quietone’s picture

The URL for the help page changed so fixing that and using the nid, since this is a short term module and we don't want to have to change it again.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC.

heddn’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Changes look fine. Most test changes that look like a bunch of stuff removed is actually more tests added because the file is copied.

Since the postgres fix is preexisting, we should not hold this issue accountable to fix it.

This confused me, should be a quick fix:

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6NoMultilingualTest.php
@@ -2,34 +2,30 @@
- * Tests Drupal 6 upgrade using the migrate UI.
+ * Tests Drupal 6 upgrade with translation requires migrate_drupal_multilingual.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7NoMultilingualTest.php
@@ -2,23 +2,19 @@
- * Tests Drupal 7 upgrade using the migrate UI.
+ * Tests Drupal 7 upgrade with translation requires migrate_drupal_multilingual.

I was trying to figure out what is going on here. So what you did is you have a copy of both D7 and D6 upgrade tests without and with multilingual. Ok.

The description is quite unclear, because it says "with translation" but for the test without translation :)

Also if you read the whole comment as a sentence it is not clear English. So I would do something like "Tests Drupal X upgrade without translations."

heddn’s picture

Status: Needs work » Needs review
FileSize
47.79 KB
1.45 KB
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Much better. Back to the ol' RTBC shed.

  • Gábor Hojtsy committed 20f44c0 on 8.7.x
    Issue #2953360 by quietone, heddn, yogeshmpawar, phenaproxima, Gábor...

  • Gábor Hojtsy committed 33511d3 on 8.6.x
    Issue #2953360 by quietone, heddn, yogeshmpawar, phenaproxima, Gábor...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Yay thanks all!

jibran’s picture

+++ b/core/modules/migrate_drupal/migrate_drupal.install
@@ -24,3 +24,10 @@ function migrate_drupal_update_8502() {
+
+/**
+ * Install migrate_drupal_multilingual since migrate_drupal is installed.
+ */
+function migrate_drupal_update_8601() {
+  \Drupal::service('module_installer')->install(['migrate_drupal_multilingual']);
+}

Why are we enabling this module by default if my site is not multilingual?

Gábor Hojtsy’s picture

@jibran: that your current site is not yet multilingual does not mean your old one was not. We are enabling it because before this patch all the migrations that are hidden by this patch were there, and after this patch they are not. If we don't enable the module then we introduced a regression.

Gábor Hojtsy’s picture

Published change record at https://www.drupal.org/node/2960040

jibran’s picture

Thanks, for explaining that @Gábor Hojtsy. I have one more question. If my source and destination sites are not multilingual then can I uninstall this module?

Gábor Hojtsy’s picture

Yes, sure.

xjm’s picture

Issue tags: +8.6.0 release notes

In addition to the CR, I think we should also mention this in the release notes since people need to know to enable the multilingual migrate module (either as a dependency or for in-progress migration work).

jcnventura’s picture

Status: Fixed » Needs work

This patch introduced a major (critical?) bug on using the migration system with Drupal 8.6, when having content_translation enabled, but migrate_drupal disabled.

This is because d6_entity_reference_translation.yml and d7_entity_reference_translation.yml were moved to content_translation from migrate_drupal, but they use the Drupal\migrate_drupal\Plugin\migrate\EntityReferenceTranslationDeriver class that is only available if migrate_drupal is enabled.

In this probably very common setup (i.e. any new D8 site using content_translation), attempting to use the migration system will throw the following error:

In DerivativeDiscoveryDecorator.php line 218:

  Plugin (d6_entity_reference_translation) deriver "Drupal\migrate_drupal\Plugin\migrate\EntityReferenceTranslationDeriver" does not exist.

Some possible solutions:

1. Move d6_entity_reference_translation.yml and d7_entity_reference_translation.yml back to migrate_drupal - reverts part of the cleanup done in this patch, but is simple to do.
2. Move EntityReferenceTranslationDeriver to content_translation - cleaner solution, but breaks the API, as it would no longer be Drupal\migrate_drupal\Plugin\migrate\EntityReferenceTranslationDeriver and instead Drupal\content_translation\Plugin\migrate\EntityReferenceTranslationDeriver. These migrations are the only references in core to this deriver, and migrate_drupal was an experimental module until now, so maybe we're OK to break the API?

jcnventura’s picture

Status: Needs work » Needs review
FileSize
738 bytes

Attaching patch for solution 1 in #156. No interdiff because duh.

Status: Needs review » Needs work

The last submitted patch, 157: 2953360-157.patch, failed testing. View results

heddn’s picture

Some thoughts here. But let's start by we open a new issue?

I'll post my further comments in that new issue.

jcnventura’s picture

Status: Needs work » Fixed

Created the child issue #2991710: Migrate system broken if content_translation enabled, as requested by @heddn. Setting this back to fixed as per #155.

Status: Fixed » Closed (fixed)

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

quietone’s picture

Publish change record.