Problem/Motivation
Migrate the revisions for d7 entity translation to d8. This is moved out of #2746541: Migrate D6 and D7 node revision translations to D8, which is now only about node translation, to reduce complexity.
This issue is intended to be completed after #2746541: Migrate D6 and D7 node revision translations to D8.
Proposed resolution
Add a new node type, et, to the drupal7 fixture that uses entity translation and has several revisions.
Use the node master plugins that are introduced in #2746541: Migrate D6 and D7 node revision translations to D8 and add a join to the entity_translation_revision table, if the table exists. Then the language must be selected accordingly. If the row uses node translation then the language is set to what is in the node table and if the row used entity_translation then the language is set to what is in the entity translation revision table.
Also, a tweak is done in the node source plugin to select the language.
Remaining tasks
The patch in #28 is merged with2746541-231.patch from the node translation revision issue so that it is available for testing.
For Testers
- Use the lastest patch patch in this issue..
- There are instructions in the Issue summary of the META issue #2208401: [META] Stabilise Migrate Drupal Multilingual module/li>
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#97 | 3076447-97.patch | 671 bytes | daffie |
#90 | 3076447-90-9.0.patch | 45.51 KB | quietone |
#90 | 3076447-90-8.9.patch | 45.51 KB | quietone |
#90 | diff-82-80-9.0.txt | 636 bytes | quietone |
#90 | diff-82-80-8.9.txt | 1.53 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone at Acro Commerce commentedAdded an english and icelandic revision.
Comment #3
quietone CreditAttribution: quietone at Acro Commerce commentedCorrect the tags
Comment #4
quietone CreditAttribution: quietone at Acro Commerce commentedIgnore previous patch. How about this one which adds a new content type which uses entity translation and has some revisions.
Comment #5
heddnIf we update the IS to discuss the specific plan here, I think we could land this as a fixture only patch and do follow-up(s) for the actual migration.
Comment #6
quietone CreditAttribution: quietone at Acro Commerce commentedGood idea but I've noticed something since then about this patch. When using the test fixture and adding entity translation revisions the db table entity_translation_revision is not populated. But if I do the same with a fresh d7 site, the entity_translation_revision table is populated. That is a bit of a worry to me.
Comment #7
quietone CreditAttribution: quietone at Acro Commerce commentedWell, here is yet another approach. This uses a new fixture just for entity translation as that is the only way I could get have an 'entity_translation_revision' table. Hopefully I've made the correct changed to drupaci.yml (as was done in #2746541: Migrate D6 and D7 node revision translations to D8 by mikelutz so this just runs the new tests.
I am having a problem with the dates though and the test will fail.
Comment #9
quietone CreditAttribution: quietone at Acro Commerce commentedThose are the errors I expected. And I see that there are coding standard errors, due to comment out code in D7NodeDeriver, that needs to be fixed.
The method used here is based on the node master approach used in #2746541: Migrate D6 and D7 node revision translations to D8, the only difference is that the source plugin uses the entity_translation_revision table. I did try locally once without the node master approach and it didn't work, having the same sort of problems encountered in the node translation revision issue.
Still to do is to sort out the revision tabs, migrated changed dates and then to get this approach working with the whole system.
Comment #10
quietone CreditAttribution: quietone at Acro Commerce commentedRemoved test code. Removed translation tag from the migration. Change the migration to use the dates from the entity_translation_revision table and added a process plugin in an attempt to get the revision translation affected flag correct. Need to review again.
Comment #11
quietone CreditAttribution: quietone at Acro Commerce commentedThis is a different approach. This uses the NodeMaster source plugin and destination that are used in #2746541: Migrate D6 and D7 node revision translations to D8. The changes here are to the source plugin, a test fixture that uses entity translation and node translation and of course the migration test itself. The test is failing because of some changed dated fields are being set to the current time and the field value for the field, field_tree, is wrong. The correct field data is migrated so I guess I've mucked up the test.
edit:
local testing with drush shows that the revision tabs look correct. Of course, that needs manual testing by someone else as well.
Comment #12
quietone CreditAttribution: quietone at Acro Commerce commentedWow that is a bad patch. Need to redo it.
Comment #13
quietone CreditAttribution: quietone at Acro Commerce commentedLet's try that again. And now with a review patch that excludes the test fixture.
Comment #15
quietone CreditAttribution: quietone at Acro Commerce commentedComment #16
quietone CreditAttribution: quietone at Acro Commerce commentedSome cleanup and adding comments.
the difference between this is and #2746541: Migrate D6 and D7 node revision translations to D8 is that there are no d6 files here, or course, the addition of querying the entity_translation_revision table in the node_master source plugin, a process plugin to help determine the revision_translation_affected property and the new source fixture.
Comment #18
quietone CreditAttribution: quietone at Acro Commerce commentedRebuilding this ontop of the latest patch in the node translation issue. Created MigrateNodeMasterETTest just to test with the drupal7 fixture that has entity translation revision. Also, changing it to run all the test so I expect there will be many.
Comment #20
quietone CreditAttribution: quietone at Acro Commerce commentedFix the ordering in the query. Still just a test patch.
Comment #22
quietone CreditAttribution: quietone at Acro Commerce commentedAdd a test in prepareRow to drop rows that are not the one changed in this revision. The test assumes that the row with the latest changed timestamp in the entity_translation_revision table is the one that has been edited. And it is only this row that is migrated.
I'd like to think that this can be done in the query() method but I wasn't successful in making a subquery in Drupal. Maybe someone else can have a look.
What is not working is the migration of the translated field on the entity.
Comment #24
quietone CreditAttribution: quietone at Acro Commerce commentedNeeds a reroll.
Comment #25
quietone CreditAttribution: quietone at Acro Commerce commentedNow merge in 2746541-231.patch from the node translation revision issue.
Comment #28
quietone CreditAttribution: quietone at Acro Commerce commentedCreated an entity_translation_revision table for the test fixture drupal7.php and that changed the migrated times. Also, made a todo for looking to the migration of the a field.
Comment #30
quietone CreditAttribution: quietone at Acro Commerce commentedComment #31
quietone CreditAttribution: quietone at Acro Commerce commentedComment #32
quietone CreditAttribution: quietone at Acro Commerce commentedComment #33
Wim LeersI'm assuming this follows the same general approach as #2746541: Migrate D6 and D7 node revision translations to D8. Meaning: if #2746541 is considered ready, then this patch is likely to be able to be updated to match the same approach as #2746541.
If that is not correct, it'd be really helpful if it could be pointed out in what ways this (>1 MB!) patch deviates significantly from #2746541. That'd allow for a more targeted review.
Comment #34
quietone CreditAttribution: quietone at Acro Commerce commented@Wim Leers, The first paragraph in #33 is correct. This will use the same approach as in #2746541: Migrate D6 and D7 node revision translations to D8.
Comment #35
Wim LeersGreat, then I don't see a need to dig deep into this issue right now :)
Comment #36
quietone CreditAttribution: quietone at Acro Commerce commentedI working on rerolling this now.
Comment #37
quietone CreditAttribution: quietone at Acro Commerce commentedThis is a partial reroll and best if you just ignore it. The drupal7 test fixture here should be the same as the one in #2746541: Migrate D6 and D7 node revision translations to D8 which means it is missing changes needed here.
Comment #38
quietone CreditAttribution: quietone at Acro Commerce commentedAnother increment on this reroll. Working towards a patch that is base on the node revision translation one but has the additions to the complete node source plugin. This still does not have the changes to the drupal7 fixture.
I'm not convinced an interdiff will be of much use. The patch in #28 is based on an older version of the node revision translation patch and there is a lot that is no longer relevant.
Comment #39
quietone CreditAttribution: quietone at Acro Commerce commentedTwice I forgot to change the version.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedFixing the failing test and including a diff against the patch in #28 if anyone is interested. The key piece that is missing is the addition of the entity translation revision table in the d7 fixture. I've done that locally and tests are failing so still work to do.
Comment #41
Gábor HojtsyAdding tag.
Comment #42
quietone CreditAttribution: quietone as a volunteer commentedJust keeping in sync with the changes in #2746541: Migrate D6 and D7 node revision translations to D8.
Comment #43
quietone CreditAttribution: quietone as a volunteer commentedLet me try that again.
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedAdjust the entity counts.
Comment #46
quietone CreditAttribution: quietone as a volunteer commentedReexamined the entity translation test fixture and corrected MigrateNodeCompleteETTest.php accordingly.
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedOops, somehow lost the changes to drupal7.php. This restores that and adjusts the times for node 1 in the test.
Comment #50
quietone CreditAttribution: quietone as a volunteer commentedMessed up the drupal7 fixture again. Try again.
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedThis patch removes the test fixture, drupal7_et.php, which was a temporary tool for testing. The node with entity translation and it s field, field_tree, is now incorporated into the drupal7.php fixture.
This includes the patch from #380 from the node revision patch, #2746541: Migrate D6 and D7 node revision translations to D8.
Comment #54
quietone CreditAttribution: quietone as a volunteer commentedAlter tests to take account of changes to the fixture.
Comment #55
quietone CreditAttribution: quietone as a volunteer commentedAborted that because I left a note to myself in the code.
Comment #57
quietone CreditAttribution: quietone as a volunteer commentedUpdate the IS.
Comment #58
quietone CreditAttribution: quietone as a volunteer commentedFixing more tests.
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedUpdate entity count.
Comment #62
quietone CreditAttribution: quietone as a volunteer commentedcorrect number of field_config entities.
Comment #64
quietone CreditAttribution: quietone as a volunteer commentedAnd some more entity counts/
Comment #65
quietone CreditAttribution: quietone as a volunteer commentedThis is ready for testing.
Update the node for testers in the IS.
Comment #66
xjmComment #67
quietone CreditAttribution: quietone as a volunteer commentedA reroll of #64 since #2746541: Migrate D6 and D7 node revision translations to D8 was committed. The diff is largely unhelpful because the patch from #64 included the work from the node revision issue and this reroll just removes all that.
Comment #68
heddnI see nothing in here that is of concern. No BC concerns. Tests seem to cover things. Leaving in NR for other reviews, but if they don't surface in a few days, lets just get this merged.
Comment #69
jungle2 coding standards messages, see https://www.drupal.org/pift-ci-job/1611884
Comment #70
jungleDeleted as they two lines are comments, or both exceed 80 characters.
Comment #71
alexpottWere these commented out because we should remove them or because they are failing an a sign of a problem.
Comment #72
quietone CreditAttribution: quietone as a volunteer commentedThe commented out lines were something that needed to be fixed. And here is the fix whichi is just to test the revision creation time. And also improve a comment
Comment #74
Wim LeersWhat's left to be done here? A thorough review?
Comment #75
quietone CreditAttribution: quietone as a volunteer commented@Wim Leers, yes. A thorough review.
Comment #76
quietone CreditAttribution: quietone as a volunteer commentedComment #77
shaktikPatch Failed to Apply on D9.1
Comment #78
shaktikComment #79
jungleHi, @shaktik, Actually, the screenshot in #77 is unnecessary. The result of CI run in #72 indicates that obviously.
Comment #80
shaktikRe-rolled patch for 9.1.x, Kindly review.
Comment #82
quietone CreditAttribution: quietone as a volunteer commentedA 9.1.x version of the patch, I made this from the patch in #72.
Comment #83
mikelutzTime to move this forward. I don't see any issues, this will improve on what we have. I'd like to see it committed to 9.1, which will give us time to find any hidden bugs.
Comment #84
jungleBTW. confirmed on my local that those serialized data won't make
drupal7.php
to be recognized as a binary file by git later on. see #3126906: MenuLinkContentTest.php is recognized as a binary file by git for more.Comment #85
catchI see this being done for the node_complete migration, but entity_translation in 7.x contrib is used for potentially any fieldable entity type such as taxonomy terms. So was more expecting this table to be added to the source query generically. Is there a way to do that or does the same change need to be made for every entity migration to support entity_translation?
Comment #86
quietone CreditAttribution: quietone as a volunteer commentedYou are right, this is specific to the entity translation revisions for node and node only.
This patch introduces the first use of the entity_translation_revision table. The existing entity translation migrations for comment, taxonomy and user all use the entity_translation table. Why not the revision table? Not being familiar with entity translation for those entities I just did some testing and it is sufficient since they are not revision-able. Unless, of course, my testing was inadequate.
Comment #88
catchThanks for pointing to those. I was expecting to see more shared logic between the different entity types, but this is consistent with what we've already done and fills a big gap in the multilingual migrations.
Committed #82 to 9.1.x
#72 no longer applies to 9.0.x (and the 9.1.x patch doesn't cherry-pick) so will need a re-roll. If possibly it would be great to get this in prior to the 9.0.x and 8.9.x release candidates (i.e. today).
Comment #89
quietone CreditAttribution: quietone as a volunteer commentedComment #90
quietone CreditAttribution: quietone as a volunteer commentedRerolled for 8.9 and 9.0. It is late and I am rather tired so I hope Upgrade7Test will pass, that was the only one with a conflict.
Oh, the diff for both is against the 9.1.x version from #82.
Comment #91
catchInterdiffs look good and DrupalCI is happy, moving back to RTBC.
Comment #92
quietone CreditAttribution: quietone as a volunteer commented@catch, thanks.
Comment #95
catchThanks for the re-rolls. Committed/pushed to 9.0.x and 8.9.x respectively, thanks!
I'm not opposed to backporting this to 8.8.x, except that we're gearing up to do the very last patch release there so there'll be no opportunity to release follow-up bugfixes should one surface. So it might be better to leave this in 8.9.x - doing that for now but re-open if you'd really like to see a backport.
Comment #96
catchThis breaks on PostgreSQL. https://www.drupal.org/pift-ci-job/1696290 Should be a quick fix to add the table alias to the order by.
Comment #97
daffie CreditAttribution: daffie commentedFix for comment #96.
Comment #98
catch#97 looks right.
Comment #101
xjmCommitted the hotfix to 9.1.x, 9.0.x, and 8.9.x. Thanks!
Comment #103
quietone CreditAttribution: quietone as a volunteer commentedThis serves as a good reminder to test the migration patches with PostgreSQL and SQLlite before RTBC.
@daffie, thanks for making the patch and to catch and xjm for getting this one in.
Comment #104
Wim LeersSorry for not doing a thorough review here like I did for
node_complete
. I was unable to find the time.That being said, I know that all of the hardenings/improvements that were made to
node_complete
were ported to this patch too. So I very strongly agree with @mikelutz in #83 that this is a big step forward regardless.Thanks so much for all your work on this issue and the node sibling issue, @quietone! 👏👏👏👏 Very impressive work!
Comment #106
Wim LeersThe necessary corresponding
field_config_instance
table row was not created. This means the currentdrupal7
fixture is wrong/broken. 😬EDIT: created follow-up issue with patch: #3153791-2: Add comment field for 'et' content type to d7 fixture.