Move content_translation I18nQueryTrait to migrate module

I think this will be fixed by 📌 Move migration code from content_translation to migrate_drupal Postponed

Added a way to move this to migrate drupal.

This problem is about the source plugins, not migrations so I think we can just move the affected source plugins. I have not done the needed deprecations. I wanted to make sure there were no test failures.

Nice no test failures. Moving to NW for deprecations mentioned in #17

godotislate → changed the visibility of the branch 3258581-move-contenttranslation-i18nquerytrait to hidden.

This is a blocker for 📌 Convert MigrateSource plugin discovery to attributes Active , because instantiating reflection on classes that use I18nQueryTrait when content_translation is uninstalled cause a fatal error.

@godotislate, I see you are working on this issue. It is currently assigned to me and I have been working on it locally for a while. :-(

@quietone I completely missed the assigneed. Many apologies.

I've updated the title and unassigned.

Thank you.

I have added deprecations to the moved classes and traits. Do deprecations still need tests? Or is phpstan covering that now?

And again, I apologize to @quietone for missing that you already were working on this and I didn't notice.

@godotislate, no worries. It is just unfortunate that we both were doing the same thing. I think you got there first because I took a break to eat.

Updated per MR feedback.

Maybe it will be less disruptive to move file and provide alias via composer autoloader?

I'm not certain that we can do anything here, but we definitely can't do this. All of these sources being moved are scheduled to be deprecated and removed completely by d12 along with the migrate_drupal module, so we can't set a deprecation message telling people to use the migrate_drupal versions before d12 because those aren't going to exist anymore. I think at best we could introduce a copy of the trait to migrate_drupal and switch the sources over to it without doing any deprecation notices about moving things, and I'm not sure we should bother doing that at this point either.

Problem/Motivation

Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait is also used in multiple migrate source plugins of other modules (e.g. Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation). This may make sense as translations usually require to have content_translation enabled. However it prevents e.g. the type checking of such plugins without instantiating them, as done in https://www.drupal.org/project/entity_import → , Drupal\entity_import\EntityImportSourceManager::getDefinitions().

Steps to reproduce

• Install Drupal standard installation profile in English, make sure content_translation is not enabled
• Install migrate_drupal module
• Install https://www.drupal.org/project/entity_import →
• Login in and go to /admin/config/system/entity-importer/add (Content > Importers > entity importer creation page)

Instead of displaying a form to add an entity importer, the following fatal error occurs:

Trait 'Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait' not found in /var/www/html/web/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Proposed resolution

I18nQueryTrait is used for source plugins that require migrate_drupal. So, move it to migrate_drupal. This is on MR 10426

User interface changes

None.

API changes

None.

Data model changes

None.

godotislate → changed the visibility of the branch 3258581-to-migrate-drupal to hidden.

I think you missed core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php, so NW for that.

Can we really make the existing trait a wrapper for the new trait in the migrate_drupal module and not add any deprecation notice? Maybe we can, but it feels too easy. I guess the idea is that when we deprecate the new trait, that will automatically deprecate the existing one (the wrapper), and we have to do that before the next major version (D12).

The existing steps to reproduce (STR) involve a contrib module. That has two problems:

1. It is not clear from the STR that this is a bug in Drupal, rather than the contrib module.
2. Manual testing is complicated, since the fix is on a branch based on 11.x and the contrib module does not have a release compatible with Drupal 11.

I know, I can add the issue fork for the D11 compatibility issue as a package repository and install the contrib module that way, or I could use the lenient composer plugin. I said it is complicated, not impossible. ;)

We can solve both problems if we give different STR. Using drush php,

> \Drupal::service('plugin.manager.migration')->createInstance('d7_custom_block_translation')->getSourcePlugin();
PHP Fatal error:  Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /var/www/html/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Fatal error: Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /var/www/html/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Before running that code, I installed Drupal with the standard profile, enabled migrate_drupal and a custom module with a simplified version of d7_custom_block_translation:

id: d7_custom_block_translation
label: Content block translations
source:
  plugin: d7_block_custom_translation
process: {}
destination:
  plugin: null

The original version of d7_custom_block_translation is in the content_translation module.

Can we turn these STR into an automated test? I think so. I am adding the tag for tests.

We should update the issue summary with the improved STR. I can do that, but not now. I need some sleep. For now, another tag.

When I check out the feature branch from the MR, it fixes my manual test:

> \Drupal::service('plugin.manager.migration')->createInstance('d7_custom_block_translation')->getSourcePlugin();
= Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation {#7934}

It would be nice to get some manual testing of the original report: does the current MR fix the problem with the contrib entity_import module? Or does that require moving a lot more into the migrate_drupal module?

I think you missed core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php, so NW for that.

Thanks for catching that. Rebased the MR and added the change.

Can we really make the existing trait a wrapper for the new trait in the migrate_drupal module and not add any deprecation notice?

Not sure what the appropriate deprecation message, given #29 🐛 Move content_translation I18nQueryTrait to migrate module Active and whatever is going to happen to migrate_drupal, but I think I agree with the idea that the deprecation message should be set when migrate drupal moves out of core.

Can we turn these STR into an automated test? I think so. I am adding the tag for tests.

Unfortunately, this is more difficult than you might think, because all classes are autoloaded when tests run. So the trait would never be missing, unless it just doesn't exist at all.

I did run the steps to reproduce with the Entity Import module. I had to

• Use the lenient composer plugin
• Edit entity_import.info.yml
• Add an inherited method return type.

I was able to install entity_import against 11.x and this MR's branch. Confirmed that the fatal error was reproduced on 11.x and gone after the trait was moved.

Moving back to Needs Review. Leaving at needs tests in case there is another idea of how to test this proposed.

I added a Functional test that shows the error locally. But when I run the test-only changes it doesn't fail as expected. Gotta run.

As it needs test, and IS needs updating and the added test doesn't fail on test only right now this is still needs work.

To figure this out I would like to see the browser output from the test. However, the artifacts for the test-only changes test run does not include the browser output. Is this by design? Is there something that can be changed in the template so the browser output is saved?

To get the browser output I made an MR with just the test changes. And looking at all the pages of output hasn't given me an idea as to why the fail test passes in gitlab but fails locally.

@quitone I check test case you create at your MR, and left my feedback there.
Short summary between difference running I18nQueryTraitTest On Gitlab CI and localhost:

• on 8 step Fatal error:is not presented (link)
• there is no 9 step presented at artifact

Found solution for checking fatal error at Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest.
Update quietone's test only MR 5316, and rebased and cherry picked changes to MR 10426.

Test is ready. Set status to NR.
Tests only MR 5316.
MR to review MR 10426

The test migration can be simpler, and the assertion messages (when the test fails) can give more information: see my comments on the MR. Back to NW for that.

The issue summary still needs an update. I offered to do that in Comment #34. I will try to find time tomorrow.

Applied changes suggested by @benjifisher.
Create new draft MR with tests only, to display error message.
MR is ready for review, but IS still need to be update, keep status to NW.
Can someone help with updating IS?

benjifisher → changed the visibility of the branch 3258581-tests-only to hidden.

@nikolay shapovalov:

Thanks for those updates. +1 for adding the helper method instead of suppressing the warning with @file(...).

GitLab CI has a test-only job. (It is not run automatically. You have to trigger it when you want it.) We do not need a test-only branch, so I am hiding the one you added.

I ran the test-only job: https://git.drupalcode.org/issue/drupal-3258581/-/jobs/3880198

There was 1 failure:
1) Drupal\Tests\block_content\Functional\migrate\d7\I18nQueryTraitTest::testUpgradeStart
Fatal error during migrate.
Failed asserting that '[06-Jan-2025 16:00:29 Australia/Sydney] PHP Fatal error: Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /builds/issue/drupal-3258581/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22\n
' is false.
/builds/issue/drupal-3258581/core/modules/block_content/tests/src/Functional/migrate/d7/I18nQueryTraitTest.php:79
FAILURES!
Tests: 1, Assertions: 6, Failures: 1.

I was not sure whether the optional message in an assertion method is supposed to be the expected result or the error condition, but that output looks right, so I think you made the right choice: "Fatal error during migrate."

I am satisfied with the code review, and the tests are green. If I can get the issue summary updated, then I will call this issue RTBC.

In Comment #34, I added the "Needs issue summary update" tag. I asked for STR that do not involve a contrib module. I have rewritten that part of the issue summary, so I am removing the tag.

Looking again at my previous comment, I think we do not need the custom message " Fatal error during migrate" at all. The detail I requested is in the default message:

 Failed asserting that '[06-Jan-2025 16:00:29 Australia/Sydney] PHP Fatal error: Trait "Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait" not found in /builds/issue/drupal-3258581/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22\n
' is false.

So I will add two more suggestions to the MR (one for each assertion) but I will still mark this issue RTBC. (The only reason it was NW instead of NR was for the issue summary update.)

@benjifisher thanks a lot for great review and your changes to IS. I applied suggested change to second assert and update fork to latest 11.x.

GitLab CI has a test-only job. (It is not run automatically. You have to trigger it when you want it.) We do not need a test-only branch, so I am hiding the one you added.

Thanks for sharing information about test-only job, didn't know.

Addressed latest MR comments.

I reviewed the latest changes, and I think all the requested changes have been made.

Really minor point (not enough to hold up this issue, in my opinion): I would have changed (shortened) the namespace of the test instead of moving it (and creating two nested directories with no other contents).

I do not care much either way, but perhaps we should leave the trait in the content_translation module intact (but still deprecate it) instead of making it a wrapper for the new version of the trait. The only reason for doing that is so that we can add type declarations to the new code. This is the current approach in [$#3498915].

There is one remaining question, from @quietone:

Can the error be generated without using the Migrate UI?

It was hard to get a suitable test for this issue: I am still not sure why my STR do not generate an error in a test. @quietone created the first version of the current test, as a Functional test using migrate_drupal_ui. (See Comments #36, #38.)

Ideally, we should be able to write the test without using migrate_drupal_ui, but it does not look easy. There is a lot of logic in CredentialForm::validateForm() and in other methods in that form class, such as setupMigrations().

The test shows us that the man code is not well structured, but fixing that is out of scope for this issue and probably not worth the effort since migrate_drupal is going away "soon". This issue is holding up 📌 Convert MigrateSource plugin discovery to attributes Active and related issues, so I vote for leaving the test as is.

I re-reviewed the changes in the MR: back to RTBC.

I spend some time trying to generate error without Migrate UI, but have no luck. I agree with @benjifisher to keep test as is.