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
This is a follow up to #2208401: [META] Remaining multilingual migration paths to tidy up the migration state files modules/*/migration/state.
Title says it all. There are @todo
s in the relevant *.migrate_drupal.yml
files pointing to
- #2208401: [META] Remaining multilingual migration paths
- #3073050: Migrate D7 i18n taxonomy term field translations
that indicate those are the last issues to be able to mark those as finished. They've been fixed, but the metadata has not been updated accordingly.
Steps to reproduce
Proposed resolution
Set the i18n migrations to finished, removing @todos from the state files.
Update tests.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#37 | 3175729-37.patch | 11.77 KB | quietone |
#32 | 3175729-30.patch | 11.84 KB | quietone |
#30 | interdiff-28-30.txt | 892 bytes | quietone |
#28 | 3175729-28.patch | 11.72 KB | quietone |
#28 | interdiff-27-28.txt | 1.48 KB | quietone |
Comments
Comment #2
Wim LeersComment #3
quietone CreditAttribution: quietone as a volunteer commented@Wim Leers, thanks for opening this issue. Unfortunately, the Meta was closed before all the child issues were committed and I didn't remember that there were todos referring to it in core. This needs to wait on at least, #3008028: Migrate D7 i18n menu links.
Comment #4
quietone CreditAttribution: quietone as a volunteer commentedSorry, meant that to be just 'postponed'
Comment #5
Wim Leers👍
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedLet's expand this to update the D6 states as well. And simplified by the title.
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedIgnore that patch.
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedAdjusting the tests for the updated states.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedComment #12
quietone CreditAttribution: quietone as a volunteer commentedI want to review this again.
Comment #13
idebr CreditAttribution: idebr at iO commentedNot sure how this fits the definition of 'finished' for i18n, but there is #3175477: Migration destination for configuration entity translations is opinionated towards i18n and can only migrate a single property at a time
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedThis is ready for review now.
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedComment #17
benjifisher@idebr: "Complete" does not mean "completely bug free".
Comment #18
benjifisherWhy remove
i18nstrings
?Does
locale: language
belong in this file?Comment #19
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Creating a patch as suggested in #18 comment.
Please review the patch.
Thanks.
Comment #20
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #21
daffie CreditAttribution: daffie commentedPatch failed to apply.
Comment #22
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #23
benjifisherIn #18, I asked questions. I do not know the answers to those questions. We should not update the patch until the questions are answered.
Comment #24
quietone CreditAttribution: quietone as a volunteer commentedFrom 18
1. Very good question. I think it was a simple mistake but it got me looking further. There is a migrate_drupal state file which had i18n entries for d6. Those are now moved to config_translation or content_translation and i18nstrings is restored. That cleans up the migrate_drupal state file to contain a variety of legacy modules that are now in core. It is a nice cleanup.
2. Those lines make no sense. It is supposed to be 'legacy module: destination module' and there is no language module. Those can be deleted.
Point 2, raises the question about what to do when the destination module is not enabled on the destination site. Typically, the destination module provides the migrations but that is not always the case. Or, it could just be a typo in the state file. In any case, the UI checks the state files and plugins etc but there is no check that the destination module is actually enabled. So, the UI could show that it will be upgraded when it will not be. Sigh. Probably needs a follow up for to consider that, adding tag.
Comment #25
benjifisherBut there is a
language
module, andlanguage.migrate_drupal.yml
includesI would say to move the lines there from
config_translation.migrate_drupal.yml
, but they are already in the right place so we just have to remove them from the wrong place.Or maybe the right thing to do is change the lines so that
config_translation.migrate_drupal.yml
haslocale: config_translation
.Since the previous patch, you have moved four lines
i18n...: content_translation
from themigrate_drupal
module to thecontent_translation
module. That makes sense. But you also addedi18n_string: config_translation
there. That looks like a mistake. That line is already in the file for theconfig_translation
module (D7 only).Comment #26
quietone CreditAttribution: quietone as a volunteer commentedGrr! I need another holiday!
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedI do not know what planet I was visiting when I last worked on this. :-(
First, this needs a reroll.
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedOk, now restore the lines in config_translation.migrate_drupal.yml.
locale: language
. For background that was added in #2936365: Migrate UI - allow modules to declare the state of their migrations. And fix the copy/paste error benjifisher found and pointed out in the last paragraph of #25.This should now be back on track.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedNot sure how I lost this one line change.
Comment #31
benjifisher@quietone:
Have a cup of tea and upload the patch along with the interdiff for #30. ;)
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedLol, you are soo right!
Here is the patch for #30.
Comment #33
quietone CreditAttribution: quietone as a volunteer commentedI am no longer convinced that a follow up is needed as I was thinking in #24. MigationState does build the migration so all the checkRequirements are done so all should be well. Also, did a test with a destination module with a typo,
Results in help being listed in the will not be upgraded with a destination module of block2. That is sufficient to alert someone that block2 does not exist.
Removing needs follow up tag.
Comment #34
benjifisherThe patch in #32 (labeled for #30) looks good to me. Quick, commit this before it needs another reroll!
Comment #36
catchCommitted/pushed to 9.2.x, thanks!
This doesn't cherry-pick cleanly to 9.1.x - I'm not sure if the intention here was to backport it to 9.1.x or not (or whether we should), so marking fixed - but if it should go back then please re-open with a re-roll.
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedBack in #3, this was postponed on #3008028: Migrate D7 i18n menu links. That was committed to 9.1.x so, yes, this can be committed to 9.1.x as well. The question remains, should we do that? What are the reasons not to backport? I can't think of any.
Here is a 9.1 patch in case there is agreement to backport.
Comment #38
catchAlso can't see a reason not to backport. Changing status and kicking off a test run.
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedI think this gets backport added to the title
Comment #41
catchCommitted ee8c012 and pushed to 9.1.x. Thanks!
Comment #42
quietone CreditAttribution: quietone as a volunteer commented@catch, thank you.
Removing [backport] from the title.