Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Follow-up for #3008025: Migrate D7 i18n block translated strings.
d7_block_translation
does not depend on language
. But d6_custom_block_translation
, d7_menu_links_translation
, d7_language_negotiation_settings
and so many others do.
So shouldn't d7_block_translation
too?
Steps to reproduce
Proposed resolution
Update the migrations and the Kernel tests.
Further tests will not be needed. In this case, regression is extremely unlikely, because removing a dependency from a migration will ring alarm bells.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#58 | 3189463-D89.patch | 24.4 KB | GyD |
#51 | 3189463-51.patch | 33.14 KB | quietone |
#51 | diff-44-51.txt | 2.67 KB | quietone |
#44 | 3189463-44.patch | 35.68 KB | quietone |
#44 | interdiff-41-44.txt | 479 bytes | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedArg, probably worth double checking all the translation migrations.
Comment #3
Wim LeersAight, that's the nudge I needed 🤓
AFAICT there's even more of these…
Let's see what happens if I make all
Drupal 7
-tagged migrations in thecontent_translation
,config_translation
andlanguage
modules depend onlanguage
…Comment #4
quietone CreditAttribution: quietone as a volunteer commented@Wim Leers, I see that I got lax on the dependencies on the translation migrations. Thanks for picking up the slack.
I count 43 migrations with 'translation' in the title and only 11 have 'language' as a dependency, leaving 32 to change. I didn't expect it to be that many.
Comment #5
Wim LeersRebased.
Comment #6
Wim Leers#5 updates 19 of them!
Comment #8
Wim LeersLet's fix the test failures.
Comment #9
Wim LeersI think #5 & #8 update all the D7 cases. I did not touch any of the D6 ones.
Comment #11
Wim LeersRandom fail in
Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest
. Retesting.Comment #12
quietone CreditAttribution: quietone as a volunteer commentedFinally got back to review this. And it needs a reroll! This reroll is suitable for a novice.
Comment #13
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #14
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedRerolled the patch as suggested. Please review.
Comment #16
quietone CreditAttribution: quietone as a volunteer commented@ayushmishra206, can you add the diff?
Comment #17
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAdding the diff.
Comment #18
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedoops, #17 saved with wrong extension.
Comment #19
quietone CreditAttribution: quietone as a volunteer commented@adityasingh, Thanks you for the diff. It does show the mistakes in the reroll, which I have identified below. They should be easy to fix. Obviously, I don't know how you are doing reroll but using the method in Rerolling patches should help prevent these errors. Cheers,
Looking at the diff
The changes to this file need to be restored.
The migration should not be changed here.
Comment #20
adityasingh CreditAttribution: adityasingh as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@quietone
I haven't re-rolled the patch.
I only shared the diff requested in #16, to help review the patch in #14. I appreciate your consistency, but we should be more vigilant while being suggestive / critical.
Comment #21
quietone CreditAttribution: quietone as a volunteer commented@adityasingh, my sincere apologies for that mistake. Thank you for pointing out my error.
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedI wanted to use this patch for some testing with #3191782: Fix dependency in d6 user profile translation migrations in order to tracking down the problem here, #3191782: Fix dependency in d6 user profile translation migrations. It needed a reroll, so I have done that.
Comment #24
Wim LeersI wonder why #22 is failing on
MigrateLanguageContentSettingsTest
, since it passed just fine in #8 🤔For migrations that are currently being worked on, here's a fix-only patch (without the test changes), which should be possible to apply to both 9.1.x and 9.2.x. That should help migration projects until this patch lands.
Comment #25
quietone CreditAttribution: quietone as a volunteer commented24. It is failing due to a mistake in the reroll.
19.1 Another mistake. d7_block_translation should have a dependency on language
19.2. Fixed
This also adds the d6 migrations.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedComment #27
Wim Leers#25: D'oh, you just beat me to it!
I just noticed the same!
Comment #28
Wim LeersFor migrations that are currently being worked on, here's a D7-fixes-only patch (without the test or the D6 migration changes), which applies to 9.1.3 & 9.1.4— for the upcoming 9.1.5 release, #3187415: Module settings translation migrations should depend on the default settings migration has been applied and has moved a migration to a different location, hence the need for this patch.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedAnd again, I forgot to update the Migration tests.
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedA few more migrations.
According to the script below that leaves the entity translation migrations.
< core/modules/content_translation/migrations/d7_entity_translation_settings.yml
30d28
< core/modules/content_translation/migrations/d7_entity_reference_translation.yml
41d38
< core/modules/content_translation/migrations/d6_entity_reference_translation.yml
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedAnd the entity reference translation migrations.
That just leaves core/modules/content_translation/migrations/d7_entity_translation_settings.yml which I don't think needs the language migration.
Comment #33
Wim Leers#31: woah, nice script-fu! 😄
Hm, why not that one, if we do add the dependency for
core/modules/language/migrations/d7_language_content_settings.yml
andcore/modules/content_translation/migrations/d7_language_content_comment_settings.yml
? 🤔Comment #34
quietone CreditAttribution: quietone as a volunteer commented@Wim Leers, thank you.
No clue. I must have been off with the fairies (day dreaming).
Fixed now.
Comment #35
huzookaComment #36
Wim LeersPort of #34 to 9.1.7.
Comment #37
Wim LeersAlso in manual testing this works great 👍😊
Thanks for pushing this across the finish line, @quietone! 🙏
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedThis needed a reroll because d6 and d7 block_translation moved to config_translation. I also added language as a dependency to a couple more migration; d6_language_content_comment_settings, d6_language_types and language.
Comment #41
Wim Leers#39:
Correct, that already was the case in your #34 patch for 9.2 :)
I should've made it more clear in #36 probably that that was literally the only change I made between #34 (for 9.2) and #36 (for 9.1). I just wanted to be able to apply this excellent patch to the current stable release!
Oh, nice catch! 😄 Unfortunately it looks like that broke the tests.
I bet this caused it … this is making
language
depend on itself!While this is nonsensical, it shouldn't crash the system.
Instead, we should make the migration definition parser throw an explicit
\LogicException
. That would've prevented these mind-bendingly annoyingly useless error messages:… although we should fix those too in #3197324: Exception trace cannot be serialized because of closure.
Comment #42
quietone CreditAttribution: quietone as a volunteer commentedChanged one to many migrations.
Comment #44
quietone CreditAttribution: quietone as a volunteer commentedNeed to add the language migration to a test.
@Wim Leers, "mind-bendingly annoyingly useless error messages" is a far more polite expression that the one I use when I get that error. I waste a lot of time because of #3197324: Exception trace cannot be serialized because of closure.
Comment #45
Wim Leers😅😂😬
I'll try to push that forward — if it saves you time it is 💯worth my time!
Back to RTBC — this looks great once again! 👍
Comment #46
huzookaIf you know where the test fails, then something like this helps:
(Copied from Media Migration)
Comment #47
larowlanPlaying devil's advocate, how can we be sure that we don't inadvertently add another migration that should depend on language but does not.
Is there a way we can add a test to prevent it sneaking back.
Or in other words, can we automate some of quietone's know-how - specifically
Comment #48
quietone CreditAttribution: quietone as a volunteer commentedFirst, this happened because I simply forgot to go back and set the dependencies on all the migrations I wrote. Somehow that step never got into my workflow. :-(
After an admittedly brief time thinking about how to test for this nothing popped up. Figuring out the dependencies is a matter of experience and what is learned when writing the migration kernel test. Plus, I am not highly motivated to think about it because as far as I know all the necessary migrations have been added to core. It is very unlikely that a new one will be added; effort is better spent on other issues in the migration system.
Does that help?
Comment #49
larowlanPerfect, that's an excellent point
Comment #50
larowlanI tried to commit this, but it no longer applies
Comment #51
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll for changes from #3191782: Fix dependency in d6 user profile translation migrations.
Comment #53
quietone CreditAttribution: quietone as a volunteer commentedKnown random failure, Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany
Comment #54
Wim LeersComment #56
larowlanCommitted 24494c2 and pushed to 9.3.x. Thanks!
Backported to 9.2.x
Thanks folks
Comment #58
GyD CreditAttribution: GyD commentedWe are using a distribution that is still using Drupal 8.9 so I back ported the patch for 8.9