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
Once the multilingual migration path is fully stable (#2208401: [META] Remaining multilingual migration paths), we have no need for the separate Migrate Drupal Multilingual module.
Proposed resolution
Hide the module from the UI, disable it and let migrate discover multilingual migrations normally.
Remaining tasks
Hide migrate_drupal_multilingualAdd hook to uninstall itAdd followup to remove module in Drupal 9.- Review change record
- Answer questions in #22 and #23
- Commit it once #2208401: [META] Remaining multilingual migration paths is done.
User interface changes
No more Migrate Drupal Multilingual module visible.
API changes
None. We keep the module for backwards compatibility.
Data model changes
Release note
The experimental Multingual Drupal Migrate module is no longer required to run multilingual migrations, which have the same stability level as other core migrations now.
Comment | File | Size | Author |
---|---|---|---|
#73 | diff-2966856.69-73.txt | 1.03 KB | mikelutz |
#73 | 2966856-73-8.9.drupal.patch | 42.71 KB | mikelutz |
#69 | 2966856-69-drupal9.patch | 42.79 KB | Gábor Hojtsy |
#69 | interdiff.txt | 385 bytes | Gábor Hojtsy |
Comments
Comment #4
Gábor HojtsyComment #5
Gábor HojtsyComment #6
quietone CreditAttribution: quietone at Acro Commerce commentedHow does one hide a module from the UI and disable it ?
Comment #7
quietone CreditAttribution: quietone at Acro Commerce commentedI know this is postponed but wanted to make a start. Still need to modify the functional tests
Comment #9
quietone CreditAttribution: quietone at Acro Commerce commentedThose failures are expected.
Comment #10
quietone CreditAttribution: quietone at Acro Commerce commentedThis is going to take more work that I originally thought. When the multilingual migrations are discovered there will be failures when the i18n_something module is not enabled and that needs to be dealt with.
Comment #11
quietone CreditAttribution: quietone at Acro Commerce commentedWhile this needs to be committed after #2208401: [META] Remaining multilingual migration paths work on this can happen now. There is more to do here that I thought last month so might as well start to make some progress.
Comment #12
quietone CreditAttribution: quietone at Acro Commerce commentedComment #13
quietone CreditAttribution: quietone at Acro Commerce commentedComment #14
quietone CreditAttribution: quietone at Acro Commerce commentedFix the failing functional tests.
Comment #15
quietone CreditAttribution: quietone at Acro Commerce commentedI think the next step here is to create d6 and d7 db versions with out the i18n and/or entity translation enabled and run the full migration tests on those. Those db versions should be made through the UI so that all the tables and columns that are added to core tables are removed.
Comment #16
quietone CreditAttribution: quietone at Acro Commerce commentedRegarding my comment above, that work should be done in a separate issue and can can be done now. There is no reason we can't be testing with non-multilingual databases. Another followup could be to discuss if the multilingual migration in config_translation and content_translation should or should not be moved to the module responsible for the entity being saved.
The latest patch here does in fact hide and disable migrate_drupal_multilingual and is ready to review.
Comment #17
voleger8602
->8802
There have to be reflected in the actual minor version when this update_N hook implementation was defined.
Comment #18
Gábor HojtsyAgree with @volger in #17. Otherwise the patch looks fine. The pageTextNotContains() assertions look a bit odd given we never add that error anymore, but given the history of this feature, it is fine to keep them like that IMHO. Other than @voleger in #17 this is postponed on #2208401: [META] Remaining multilingual migration paths for actual commit though.
Comment #19
Gábor HojtsyA followup to remove the module in Drupal 9 would actually be needed. Added it to tasks in the issue summary.
Comment #20
catchAs well as uninstalling the module and marking it hidden, we should probably also add a hook_requirements() to the module that prevents it being installed with message. This would prevent it getting reinstalled via a configuration import or drush or similar after the update has run.
Comment #21
catchArghh one other thing I just thought of.
In the follow-up to remove the module from Drupal 9, we also need to remove the update function added here from Drupal 9 - because it will fail on Drupal 9 sites without a module to remove in the codebase.
This may be handled by #2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later) but it may also need to be covered separately if it doesn't get done there.
Comment #22
xjmDo we actually need to force uninstallation in a minor?
Comment #23
xjmAlso, what if someone updates directly from 8.7 to 9.0? (re: the update hooks etc.)
Comment #24
heddnWasn't there some recent discussion that we would semi require updating from 8.8 or 8.9 to 9.x? That anything more direct wouldn't be a supported upgrade path. Was there an issue for that or was it slack chatter? I can't recall.
Comment #25
quietone CreditAttribution: quietone at Acro Commerce commentedJust doing these two now.
#17. Fixed
#19. Followup to remove migrate_drupal_multilingual #3080244: Remove Drupal Migrate Multilingual
Comment #26
volegerThanks, changes addressed for #17.
+1 for rtbc
Comment #27
quietone CreditAttribution: quietone at Acro Commerce commented@voleger, your welcome.
Oops, there is already an issue to Remove Migrate Drupal Multilingual in D9. #2966859: Remove Migrate Drupal Multilingual module in Drupal 10
Comment #28
quietone CreditAttribution: quietone at Acro Commerce commentedChange record added, please review. Thanks.
Comment #29
quietone CreditAttribution: quietone at Acro Commerce commented#20. Added a hook_requirements(). Note that this does not prevent the module from being installed with Drush because of https://github.com/drush-ops/drush/issues/3669. This is my first hook_requirements (I think) and I kept wondering what was wrong until I did some searching and gound that issue.
Still to do is answering the questions in #22 and #23.
Comment #31
Gábor Hojtsy@xjm, @heddn: re updating from 8.7 to 9.0, see #2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later) that catch linked above :)
Comment #32
quietone CreditAttribution: quietone at Acro Commerce commentedI don't know how to fix the test failures, will someone provide some guidance please.
Comment #33
Gábor HojtsyIf it is a legitimate fail, then it could be if hook_requirements() are checked before update functions are run and the fixture for testing update functions has the multilingual migrate module enabled?
Comment #34
Gábor HojtsyThat may indeed be the case based on
UpdatePathTestBase::runUpdates()
. It does seem to check for minimal requirements errors, but attempts to swing forward if there were other errors. I don't know why is it not failing sooner on the continue link not being clickable, etc. but that is where it appears to be broken.I don't have more time to track this down unfortunately :/
Comment #35
quietone CreditAttribution: quietone at Acro Commerce commented@Gábor Hojtsy. thanks very much. That says to me that I am not the best person to work on fixing those errors. I see if the project can have alexpott take a look.
Comment #36
alexpottI think this should only be checked at runtime and not during install - which might be automated - or update which we want to proceed for security reasons.
We need to test this.
I think this should be change to assert that the expected migration is there and that
$session->pageTextNotContains("Install migrate_drupal_multilingual")
. Looking at this some more I think actually we should remove these tests as they are meaningless no thatComment #37
alexpottHere's a patch that addresses #36. I think we might want to add a test for the requirements error in the migrate_drupal_multilingual module.
The thing that needs discussion I think is the removal of
I think this makes sense as with this patch the tests don't seem to be really testing anything.
Comment #39
alexpottComment #40
Gábor HojtsyHah, the pageTextNotContains() assertions I also found odd as per #18, looking at their tests, it does seem like that is not testing anything particular anymore indeed.
Also agreed to put the requirements check at runtime only. If it blocks (security) updates that is a problem as then the update would not even run to disable it if we force to not run updates if it is enabled :D
Comment #42
clemens.tolboomFixed
>/li>
typoComment #43
quietone CreditAttribution: quietone as a volunteer commentedPostponed on #2208401: [META] Remaining multilingual migration paths
Comment #44
Gábor HojtsyTagging.
Comment #45
xjmComment #46
catch#2746541: Migrate D6 and D7 node revision translations to D8 is as far as I know the last remaining core migration to land.
#3076447: Migrate D7 entity translation revision translations is for the entity_translation contributed module in 7.x.
Do we want to mark this stable when the core migration lands, then add the contrib migration to the stable module later, or wait for both?
Comment #47
heddnThe whole rational for forking off the separate multi lingual migrate module in core was because we knew it would take a lot longer to stabilize those upgrade paths for drupal. (I don't think anyone expected it to take quite so much time though). The hardest thing we've found is that there isn't quite enough folks with the knowledge and ability to test and provide feedback on these complicated issues.
With that short back story, I'd be more in favor of marking things stable in the multi lingual landscape sooner rather than later. Let's unleash migrations on more people. The only thing that gives me some pause is whether there are any potential BC concerns with doing the contrib stuff at a later date. And I'd say that the risk is pretty low at this point. We don't have to have every. single. migration. figured. out. Just the the main important ones and feel comfortable that we won't break things for folks in the future.
+1 on marking stable after #2746541: Migrate D6 and D7 node revision translations to D8
Comment #48
catchOK given #2746541: Migrate D6 and D7 node revision translations to D8 is RTBC, unpostponing this one.
Comment #49
Gábor HojtsyHere is 39 again for sake of a rerun. Keeping 39 as-is so its green run is not hidden behind whatever comes out now :)
Comment #50
heddnLooks like there are still outstanding questions about #22/#23.
Why don't we just leave it enabled, and deprecate it in 9.x for removal in 10.x? Less risky.
Comment #51
vuilRe-roll the patch of #49 (#39).
Comment #52
catchWe need to force uninstall in 9.0.x, so that the module can be removed from the file system in 10.x. Otherwise we'd be requiring people to manually install it prior to 10.x update, or they'll get 'missing module' errors.
The force-uninstallation doesn't need to go into the 8.9.x backport though, we can just hide it there.
Comment #53
heddnI _think_ that's what I was trying to say. But #52 says it better. +1.
Comment #54
heddnNW because doesn't apply on 9.x. And note that D9 requires 7.3 at this point, so no need to stage tests on earlier version of PHP.
Comment #55
catch#2746541: Migrate D6 and D7 node revision translations to D8 landed, so this is properly unblocked now.
Comment #56
Gábor HojtsyRerolled #51 for Drupal 9 specifically. The removed update function made it not apply.
Comment #58
Gábor Hojtsydrupal-8.filled.standard.php.gz is not present anymore, but drupal-8.8.0.filled.standard.php.gz is, let's see what does that get us.
Comment #59
catchExciting.
This can be a post-update (and should be, makes backports a lot simpler).
Apart from switching the hook_update_N() to a post-update, looks great to me - but needs work for that.
Comment #60
Gábor HojtsyMade it a post-update.
Comment #61
catchNeeds to move to migrate_drupal.post_update.php
@see reference should change.
@see reference should change.
Comment #62
Gábor HojtsyComment #63
Gábor HojtsyDone :) I also noticed a small grammar problem in the requirements message.
@catch: I picked updates-8.9.x in the @addtogroup following prior groups for post_update hooks (although there is none currently in Drupal 9, this would be committed to Drupal 9 too I assume?). Or should this be backported all the way back to 8.8 in which case that would be the group?
Comment #64
heddnCouple questions to add to the mix.
We backported the final stable blocker to 8.8. So I don't see why we couldn't also add this to 8.8. But then again, there's no harm in waiting until 8.9 either.
Is this the only way to mark something deprecated? Is there any way to also trigger error so it would get caught by phpstan?
Comment #66
Gábor Hojtsy@heddn: yeah I would refer to @catch on whether deprecating a whole module in Drupal 8.8 bugfix releases is acceptable. My hunch is not.
Re: the requirements message / how to deprecate, I don't think we can catch people depending on this module in their info file or anything like that. That said, @catch should also be able to verify that :)
Comment #68
catchWe may need the module to minimise disruption for the 8.8 backport of #2746541: Migrate D6 and D7 node revision translations to D8, should not aim to deprecate/uninstall in 8.8 for that reason, but yeah also in general because it's a patch release.
No strong opinion but I think this is fine, to be honest we probably need to revisit that in general but not something to look at here.
Comment #69
Gábor HojtsyHelps to include PHP tags in a PHP file :D
Comment #70
heddnAll my feedback addressed. Almost 2 years since opening, we get to mark this sucker closed. Nice feeling.
Comment #72
catchCommitted c964d76 and pushed to 9.0.x. Thanks!
The patch doesn't cherry-pick to 8.9.x so it'll need a re-roll.
@xjm asked whether we need to force uninstall in 8.9.x
#2966856-22: Hide and disable Drupal Migrate Multilingual
Since the module doesn't provide any functionality at all - it's effectively a configuration option, I think it is good to uninstall and hide it in this case in 8.9.x too. Different if it was an experimental module that provided anything else.
Added a release note.
Comment #73
mikelutzHere's a straight up re-roll, not sure if we will need to adjust more tests in 8.9 that were removed from 9.0 already.
Comment #74
Gábor HojtsyCopy edited the change record and made it much clearer. Did not publish yet until it gets merged to 8.9.
Comment #75
Gábor HojtsyReroll looks good the post update file is different but because 8.9 already has a post update.
Comment #76
alexpottCommitted d201e98 and pushed to 8.9.x. Thanks!
Comment #78
xjmThis should probably be briefly explained in the release notes.
Comment #80
quietone CreditAttribution: quietone as a volunteer commentedPublished change record.
Comment #81
Wim LeersRelease note is already present.