Hide and disable Drupal Migrate Multilingual

How does one hide a module from the UI and disable it ?

I know this is postponed but wanted to make a start. Still need to modify the functional tests

Those failures are expected.

This 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.

While 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.

Fix the failing functional tests.

I 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.

Regarding 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.

+++ b/core/modules/migrate_drupal/migrate_drupal.install
@@ -31,3 +31,10 @@ function migrate_drupal_update_8502() {
+function migrate_drupal_update_8602() {

8602 -> 8802

There have to be reflected in the actual minor version when this update_N hook implementation was defined.

Agree 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.

A followup to remove the module in Drupal 9 would actually be needed. Added it to tasks in the issue summary.

As 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.

Arghh 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.

Do we actually need to force uninstallation in a minor?

Also, what if someone updates directly from 8.7 to 9.0? (re: the update hooks etc.)

Just doing these two now.
#17. Fixed
#19. Followup to remove migrate_drupal_multilingual #3080244: Remove Drupal Migrate Multilingual

Thanks, changes addressed for #17.
+1 for rtbc

@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

Change record added, please review. Thanks.

#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.

@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 :)

I don't know how to fix the test failures, will someone provide some guidance please.

If 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?

That 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 :/

@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.

1. +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.install
   @@ -0,0 +1,19 @@
   +  $requirements = [];
   +  $requirements['migrate_drupal_multilingual'] = [
   +    'title' => t('Migrate Drupal Multilingual'),
   +    'severity' => REQUIREMENT_ERROR,
   +    'description' => t('The Migrate Drupal Multilingual is deprecated and should not be installed.'),
   +  ];
   +  return $requirements;
   

   I 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.

2. +++ b/core/modules/migrate_drupal/migrate_drupal.install
   @@ -31,3 +31,10 @@ function migrate_drupal_update_8502() {
   +/**
   + * Uninstall migrate_drupal_multilingual since migrate_drupal is installed.
   + */
   +function migrate_drupal_update_8802() {
   +  \Drupal::service('module_installer')->uninstall(['migrate_drupal_multilingual']);
   +}
   

   We need to test this.

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualTest.php
   @@ -203,7 +203,7 @@ public function testMigrateUpgradeExecute() {
   -    $session->pageTextContains("Install migrate_drupal_multilingual to run migration 'd6_system_maintenance_translation'.");
   +    $session->pageTextNotContains("Install migrate_drupal_multilingual to run migration 'd6_system_maintenance_translation'.");
   
   +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/NoMultilingualTest.php
   @@ -214,7 +214,7 @@ public function testMigrateUpgradeExecute() {
   -    $session->pageTextContains("Install migrate_drupal_multilingual to run migration 'd7_system_maintenance_translation'.");
   +    $session->pageTextNotContains("Install migrate_drupal_multilingual to run migration 'd7_system_maintenance_translation'.");
   

   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 that

Here'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

 delete mode 100644 core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualTest.php
 delete mode 100644 core/modules/migrate_drupal_ui/tests/src/Functional/d7/NoMultilingualTest.php

I think this makes sense as with this patch the tests don't seem to be really testing anything.

Hah, 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

Fixed >/li> typo

Postponed on #2208401: [META] Remaining multilingual migration paths

Tagging.

#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?

OK given #2746541: Migrate D6 and D7 node revision translations to D8 is RTBC, unpostponing this one.

Here is 39 again for sake of a rerun. Keeping 39 as-is so its green run is not hidden behind whatever comes out now :)

Re-roll the patch of #49 (#39).

We 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.

#2746541: Migrate D6 and D7 node revision translations to D8 landed, so this is properly unblocked now.

Rerolled #51 for Drupal 9 specifically. The removed update function made it not apply.

drupal-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.

1. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php
   @@ -27,8 +27,6 @@ class MigrateBlockContentTranslationTest extends MigrateDrupal6TestBase {
        'statistics',
        'taxonomy',
   -    // Required for translation migrations.
   -    'migrate_drupal_multilingual',
      ];
   

   Exciting.

2. +++ b/core/modules/migrate_drupal/migrate_drupal.install
   @@ -14,6 +14,13 @@ function migrate_drupal_update_last_removed() {
    
   +/**
   + * Uninstall migrate_drupal_multilingual since migrate_drupal is installed.
   + */
   +function migrate_drupal_update_8802() {
   +  \Drupal::service('module_installer')->uninstall(['migrate_drupal_multilingual']);
   +}
   +
   

   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.

Made it a post-update.

1. +++ b/core/modules/migrate_drupal/migrate_drupal.install
   @@ -17,7 +17,7 @@ function migrate_drupal_update_last_removed() {
     */
   -function migrate_drupal_update_8802() {
   +function migrate_drupal_post_update_uninstall_multilingual() {
      \Drupal::service('module_installer')->uninstall(['migrate_drupal_multilingual']);
    }
   

   Needs to move to migrate_drupal.post_update.php

2. +++ b/core/modules/migrate_drupal/tests/src/Functional/MigrateDrupalUpdateTest.php
   @@ -0,0 +1,38 @@
   +   */
   

   @see reference should change.

3. +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.install
   @@ -0,0 +1,23 @@
   + *
   + * @see migrate_drupal_update_8802()
   + */
   

   @see reference should change.

Done :) 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?

@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 :)

We 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.

@catch: I picked updates-8.9.x in the @addtogroup following prior groups for post_update hooks

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.

Helps to include PHP tags in a PHP file :D

Committed 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.

Here'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.

Copy edited the change record and made it much clearer. Did not publish yet until it gets merged to 8.9.

Reroll looks good the post update file is different but because 8.9 already has a post update.

Committed d201e98 and pushed to 8.9.x. Thanks!

This should probably be briefly explained in the release notes.

Published change record.

Release note is already present.