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.
Comment | File | Size | Author |
---|---|---|---|
#157 | 2953360-157.patch | 738 bytes | jcnventura |
#145 | interdiff_141-145.txt | 1.45 KB | heddn |
#145 | 2953360-145.patch | 47.79 KB | heddn |
Comments
Comment #2
heddnThis 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.
Comment #3
heddnComment #4
heddnWe 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.
Comment #5
quietone CreditAttribution: quietone at Acro Commerce commentedI 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.
Comment #6
heddnThis issue would unblock stability of migrate_drupal. So bumping to critical as per agreement with committers anything blocking stability for migrate is critical.
Comment #7
catchI 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.
Comment #8
quietone CreditAttribution: quietone at Acro Commerce commentedA search for migrations with 'translation' in the name return these:
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.
Comment #9
quietone CreditAttribution: quietone at Acro Commerce commentedI'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'.
Comment #11
quietone CreditAttribution: quietone at Acro Commerce commentedEnable migrate_drupal_i18n in more tests.
Comment #13
quietone CreditAttribution: quietone at Acro Commerce commentedStill missed some tests. And add migrate_drupal_i18n to core/composer.json.
Comment #15
heddnSurely we don't require i18n if someone is migrating d6 nodes and doesn't have i18n content on the source?
Comment #16
quietone CreditAttribution: quietone at Acro Commerce commentedTook 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
Comment #19
quietone CreditAttribution: quietone at Acro Commerce commentedAdd hook help.
Comment #21
quietone CreditAttribution: quietone at Acro Commerce commentedSeems 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.
Comment #22
quietone CreditAttribution: quietone at Acro Commerce commentedNow there is a passing test and a failing test, #16. That leaves creating documentation and updating the module link.
Needs a link to new documentation
Comment #23
quietone CreditAttribution: quietone at Acro Commerce commentedtagging for documentation
Comment #24
heddninheritdoc here.
Comment #25
quietone CreditAttribution: quietone at Acro Commerce commentedFix for #24
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedWhere 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.
Comment #27
heddnFor the hook_help, use https://www.drupal.org/docs/8/core/modules/experimental-migrate-drupal-i18n
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedLooks like I wasn't working from HEAD when I made the last patch. So, starting over from the patch in #21.
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll.
Comment #33
quietone CreditAttribution: quietone as a volunteer commentedOops, try again.
Comment #35
quietone CreditAttribution: quietone as a volunteer commentedFixed a comment and hopefully the tests.
Comment #36
heddnLet's see what happens when this goes to RTBC.
Comment #37
alexpottI think given that suddenly working migrations will not be there I think we should have a change record.
Are we concerned about this suddenly breaking custom migrations?
Comment #38
heddnBack 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.
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedDraft change record started. It is pretty terse, what more needs to be added?
Comment #40
heddnLet'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.
Comment #41
masipila CreditAttribution: masipila as a volunteer commentedRegarding 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
Comment #42
quietone CreditAttribution: quietone as a volunteer commented@heddn, thanks. And ARG! I am not a writer but I have updated the CR with all that you suggest.
Comment #43
heddnRemoving 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.
Comment #44
alexpottI 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.
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedDon't know about hook_update. Will this go into migrate_drupal_i18n? Will it run when migrate_drupal_i18n is not enabled?
Comment #46
alexpott@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.
Comment #47
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedThe 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.
Comment #49
heddnSuper nit: Let's drop the if and say 'since migrate_drupal is installed'. And we probably need a test of the update hook, no?
Comment #50
heddnConsidering 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.
Comment #51
heddnAlso, 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.
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedReroll and fix together in one patch, since it was a one line fix.
Comment #54
quietone CreditAttribution: quietone as a volunteer commentedNow update the test.
Comment #56
quietone CreditAttribution: quietone as a volunteer commentedAdjust entity count.
Comment #58
yogeshmpawarComment #59
yogeshmpawarMaybe this patch will solve the test fails & also added an interdiff.
Comment #60
heddnShall we all say RTBC together? Great work everyone.
Comment #61
maxocub CreditAttribution: maxocub as a volunteer commentedTagging to reflect that this is a (the only remaining) blocker for Migrate Drupal stability: #2905736: Mark Migrate Drupal as stable.
Comment #62
maxocub CreditAttribution: maxocub as a volunteer commentedForgot a tag.
Comment #63
catchThis 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.
Comment #64
catchAlso 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?
Comment #65
heddn#2966856: Hide and disable Drupal Migrate Multilingual & #2966859: Remove Migrate Drupal Multilingual module in Drupal 10 opened.
Comment #66
catchCorrection 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.
Comment #68
quietone CreditAttribution: quietone as a volunteer commentedThis should also be tagged i18n-migrate.
Comment #69
quietone CreditAttribution: quietone as a volunteer commentedThis needed a reroll.
Comment #71
quietone CreditAttribution: quietone as a volunteer commentedAdjust the tests for recent changes.
Comment #72
heddnBack to RTBC.
Comment #73
Gábor HojtsyWe 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.
Comment #74
Gábor HojtsyBTW said committer meeting publicly documented at #2978215: [Core Committers] Meeting notes 2018/06/07
Comment #75
catchI'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.
Comment #76
Gábor HojtsyComment #77
maxocub CreditAttribution: maxocub as a volunteer commentedBoth follow-ups already exist:
Comment #78
Gábor HojtsyAll 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?
Comment #79
maxocub CreditAttribution: maxocub as a volunteer commentedThat's true, Multilingual core migrations are not limited to i18n. We need to find another way.
Comment #80
heddnDiscussed 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).
Comment #81
quietone CreditAttribution: quietone as a volunteer commentedAs 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?
Comment #83
quietone CreditAttribution: quietone as a volunteer commentedChange 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.
Comment #85
quietone CreditAttribution: quietone as a volunteer commentedAdjust language content settings entity count because d7_entity_translation isn't running.
Comment #86
Gábor HojtsyUpdated 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?
Comment #87
heddnBack to NW for #86. Let use the tag everywhere seems to be the concern. Instead of checking the source module is i18n.
Comment #88
quietone CreditAttribution: quietone as a volunteer commentedThis needs a reroll, starting with that first.
Comment #90
quietone CreditAttribution: quietone as a volunteer commentedEnable 'migrate_drupal_multilingual' for the two recently committed test.
Comment #91
quietone CreditAttribution: quietone as a volunteer commentedI'm assuming that will pass. Here are the changes for #87
Comment #93
quietone CreditAttribution: quietone as a volunteer commentedEnabled 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'
Not tagged 'Multilingual' because these haven't been discussed in the issue.
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.
Comment #94
webchickWe'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?
Comment #95
maxocub CreditAttribution: maxocub as a volunteer commentedSadly, we are not close to fixing all i18n issues. This issue should be our main priority.
Comment #96
Gábor HojtsyWhy do we exclude the node translations and entity translations? I would expect all translation stuff under here if this is the multilingual module.
Comment #97
maxocub CreditAttribution: maxocub as a volunteer commentedDiscussed 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.
Comment #98
quietone CreditAttribution: quietone as a volunteer commentedThis 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.
Comment #99
quietone CreditAttribution: quietone as a volunteer commentedHere is the interdiff
Comment #100
quietone CreditAttribution: quietone as a volunteer commentedJust 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.
Comment #102
quietone CreditAttribution: quietone as a volunteer commentedA few improvements.
Comment #104
quietone CreditAttribution: quietone as a volunteer commentedInstead of putting the validation in Migration.php I think it should be in MigrationConfigurationTrait::checkRequirements and it has a todo.
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.
Comment #105
quietone CreditAttribution: quietone as a volunteer commentedApologies, I've only got time to upload the patch now. I'll come back later to comment more fully.
Comment #108
quietone CreditAttribution: quietone as a volunteer commentedStill a work in progress. No need to review. I just want to see what the testbot finds.
Comment #110
quietone CreditAttribution: quietone as a volunteer commentedThis 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.
Comment #111
quietone CreditAttribution: quietone as a volunteer commentedNo patch this time but a summary of the changes so far.
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.
Comment #112
quietone CreditAttribution: quietone as a volunteer commentedAdd a test for Drupal 7 upgrade without translation modules enabled.
Comment #113
quietone CreditAttribution: quietone as a volunteer commentedRemove a duplicate block of text from the IS
Comment #115
quietone CreditAttribution: quietone as a volunteer commentedFix entity count in the MigrateUpgrade7I18nTest and reverse two introduced mistakes.
Comment #117
phenaproximaFound some nits, but I think I need to read the tests more thoroughly.
No need for this to be two lines. Let's just collapse it into
\Drupal::service('module_installer')->install(['migrate_drupal_multilingual']);
Nit: Longer than 80 characters. So let's just axe the words "the module". :)
This should be
$tags = $migration->getMigrationTags() ?: []
.Let's pass TRUE as the third parameter to in_array().
Can we remove "entity"? That's a bit vague.
migrate_drupal depends on migrate already, so we can remove that.
I think we can remove "entity and i18n".
This should be {@inheritdoc}.
Nit: Empty line.
Why did this entity count change?
This should be {@inheritdoc}.
Nit: Empty line.
Comment #118
quietone CreditAttribution: quietone as a volunteer commented1-9 and 11-12 fixed
10. I presume because config_translation is installed and it wasn't before.
Comment #120
quietone CreditAttribution: quietone as a volunteer commentedFix a copy paste error with the admin password for the new tests.
Comment #122
quietone CreditAttribution: quietone as a volunteer commentedThis 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?
Comment #123
phenaproximaThis snippet is probably a git snafu and should be reverted :)
Should this be *outside* the try block? I've never seen an exception thrown directly inside a try block before.
Let's rephrase: "Provides a requirement for multilingual content and configuration migrations."
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.
drupalPostForm() is harder to grok than just
$assert->buttonExists('I acknowledge I may lose data. Continue anyway.')->press()
.Here too, I don't think we should name a class "*i18n*" if it doesn't involve translations.
Comment #124
quietone CreditAttribution: quietone as a volunteer commentedThanks 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?
Comment #125
phenaproxima$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 itspress()
method), whereas drupalPostForm is more obscure, especially when you're already on the page with the form. :)Comment #127
quietone CreditAttribution: quietone as a volunteer commentedYes, 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.
Comment #128
quietone CreditAttribution: quietone as a volunteer commentedThe taxonomy test needs config_translation enabled now.
Comment #131
quietone CreditAttribution: quietone as a volunteer commentedAlmost 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.
Comment #132
quietone CreditAttribution: quietone as a volunteer commentedHere is the interdiff.
Comment #133
phenaproximaPatch makes sense, and I'd feel fine about RTBCing it. I just have a few questions...
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).
Same here.
And here.
And here too :)
Why is i18n removed, but not i18nstrings?
Same here.
And here.
I'm not sure this is actually used anywhere?
Comment #134
quietone CreditAttribution: quietone as a volunteer commented1, 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
Comment #135
phenaproximaInterdiff 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.
Comment #136
phenaproximaOnly found one minor, but commit-blocking, thing.
This says "Migrate Drupal i18n", but it should be "Migrate Drupal Multilingual".
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.
Comment #137
quietone CreditAttribution: quietone as a volunteer commented1. Well spotted and fixed.
2. Yes, I look forward to refactoring the upgrade tests.
Comment #138
phenaproximaRestoring RTBC once green on all backends.
Comment #139
quietone CreditAttribution: quietone as a volunteer commentedThe followup for #134 5,6,7 has been made, #2988235: Remove i18n from d6 $noUpgradePaths
Comment #140
heddnBack to NW to update the URL for the module page in hook_help.
Comment #141
quietone CreditAttribution: quietone as a volunteer commentedThe 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.
Comment #142
heddnAnd back to RTBC.
Comment #143
heddnThe postgres failure is from: #2982755: Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key
Comment #144
Gábor HojtsyChanges 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:
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."
Comment #145
heddnComment #146
phenaproximaMuch better. Back to the ol' RTBC shed.
Comment #149
Gábor HojtsyYay thanks all!
Comment #150
jibranWhy are we enabling this module by default if my site is not multilingual?
Comment #151
Gábor Hojtsy@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.
Comment #152
Gábor HojtsyPublished change record at https://www.drupal.org/node/2960040
Comment #153
jibranThanks, 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?
Comment #154
Gábor HojtsyYes, sure.
Comment #155
xjmIn 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).
Comment #156
jcnventura CreditAttribution: jcnventura at Wunder commentedThis 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:
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?
Comment #157
jcnventura CreditAttribution: jcnventura at Wunder commentedAttaching patch for solution 1 in #156. No interdiff because duh.
Comment #159
heddnSome thoughts here. But let's start by we open a new issue?
I'll post my further comments in that new issue.
Comment #160
jcnventura CreditAttribution: jcnventura at Wunder commentedCreated the child issue #2991710: Migrate system broken if content_translation enabled, as requested by @heddn. Setting this back to fixed as per #155.
Comment #162
quietone CreditAttribution: quietone at PreviousNext commentedPublish change record.