Move I18nQueryTrait from content_translation to migrate_drupal

Patch applies against 9.3.3

Sorry, I didn't intend to change the version number.

I tested this on 9.4.x, standard install and was not able to reproduce this error. I followed the steps given in the Issue Summary. Is there something else that needs to be done? Changing status to get more information.

The migrate maintainers work from the 'migration system' component, changing component.

This isn't a feature request and although I can't reproduce the problem, changing to a bug report.

Just ran into the exact same issue while trying to install entity_import module.
The 'Add entity importer' just gives a WSOD.

@sanduhrs, thanks for reporting the problem.

I am still not able to reproduce the problem using the steps in the Issue Summary.

Doing more research I see that In the related issue the explanation for the problem is given and so is the workaround. I suggest using that method.

The i18nQuery trait is only needed for migrating a drupal database and thus will not be moving to the migrate module. What could be done in core is add checkRequirements to the source plugins using the trait to ensure that content_translation is enabled. Assigning this to myself to consider that later.

I think this will be fixed by #3332998: Move migration code from content_translation to migrate_drupal

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

This is a blocker for #3421014: Convert MigrateSource plugin discovery to attributes, because instantiating reflection on classes that use I18nQueryTrait when content_translation is uninstalled causes 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 assignee. 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.

Note that moving the MenuLinkTranslation classes to migrate_drupal means they now extend MenuLink from menu_link_content, but this is an improvement in that extending an unknown class throws an exception and does not fatal.

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.

After conversation with @benjifisher on Slack, I investigated the feasibility of moving just the Trait, without the plugin classes that depend on it, in context of #3421014: Convert MigrateSource plugin discovery to attributes, without running into a fatal error.

It turns out that since the plugins extend DrupalSqlBase, an exception is thrown, which avoid the fatal.

Created MR MR 10426 that copies the trait to migrate_drupal, with the original trait in content_translation using it.

Per #29, left off the deprecation messages.

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 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 message is not presented at Gitlab CI (link)
• there is no 9 step presented at Gitlab CI artifact

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

Test is ready. Set status to NR.
Tests only MR 10779.
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?

@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 one more suggestion to the MR 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 class 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: Move content_entity source plugin to migrate module.

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 #3421014: Convert MigrateSource plugin discovery to attributes 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.

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.

I think this would be a good idea. We're still finalising how to add type hints to existing code in-place, so it would save at least two further issues if we were to do this now, based on the current plan for doing that (one to indicate we're going to add the type hints and another to actually add the type hints). Don't want to hold this issue up, but if someone has time to do it, it would save more time later I think.

#3498915: Move content_entity source plugin to migrate module is also RTBC but I haven't reviewed it yet, so I will try to look over there.

Made the changes per #55. Does it need to go back to NR?

Yes, the issue should go back to NR. I will review it soon, unless someone else does it first.

I think we made the same decision on #3498915: Move content_entity source plugin to migrate module: keep the existing class intact and add type declarations to the new one. That issue seems to have merge conflicts, and I think I have time to handle that now.

I reviewed the changes. There is just one problem: we should add parameter type declarations as well as a return-type declaration. Back to NW for that.

On the plus side, I see that the earlier version of the MR only did part of the recommended deprecation. (It added @trigger_error() below the namespace line, but it did not add @deprecated and @see link-to-cr to the doc block). The current version fixes that: thanks.

Reference: https://www.drupal.org/about/core/policies/core-change-policies/how-to-d...

I also updated the change record (CR). It still targeted 10.3.x, and I updated that to 11.2.x. It said that several classes that use the trait were also being moved to migrate_drupal, which was the plan before Comment #31. I removed that section of the CR.

Can't believe I missed the parameter typehints, but everything should be addressed now.

The last few commits address the suggestion in Comment #55. Back to RTBC.

I took inspiration from #3502913: Add a fallback classloader that can handle missing traits for attribute discovery and took a look at seeing whether it's possible to set the class loader not to load namespaces for uninstalled modules in a test method. I did this in MR 11081 and was able to create a failing Kernel test. Setting back to NR, since it'll be much faster to use a Kernel test instead of a Functional test.

@godotislate:

Good find! Yes, a kernel test is much better.

On the principle that no good deed goes unpunished, I made several suggestions for changes on the MR.

We do not need both MRs. Either cherry-pick the last two commits from MR 10246 to MR 11081 or else cherry-pick the last commit in the other direction. (Or make one commit on MR 10246, incorporating my feedback.)

Maybe use a data provider so that the test-only job fails on all 5 source plugins instead of getting a fatal error on just the first one.

Finally, I think we may want to reuse the first part of the test (the part that breaks discovery for uninstalled modules). Eventually, we should move it to a trait, but I think it is premature to do that now, so let's start by making it a separate method in the test class. It should have at least one argument: a list (array) of module names to update.

I am not sure: for this test, is it better to break class discovery for all uninstalled modules or just for content_moderation? I am leaning toward the second option. That would also save a few lines of code.

If you prefer the first option, then the new method should default to acting on all uninstalled modules. And it should have a required first argument: a list of module names that can be asserted to be on the other list: something like disablePsr4(array $expected, ?array $disabled = NULL): void. Maybe use longer, more descriptive, parameter names.

I created new MR 11081 in case the old test was preferred, but I forgot that the last two commits on MR 10426 weren't on my local.

Anyway, I've closed MR 10426 and decided to go ahead with 11081 so that the last commit diff specifically addresses the MR feedback. I've also rebased to include the two missing commits.

Back to NR.

We now have a version that addresses the request in #55 and also replaces the Functional test with a Kernel test. Back to RTBC.

Just re-read @mikelutz comment and sort of agree but also think where we ended up is fine. For modules integrating with migrate_drupal now, we should tell them the class is moved, this can help with dependency issues in contrib etc. Then when we deprecate migrate_drupal as a whole, they'll get different deprecation message again, but sometimes this is what happens.

I published the change record, and I am about to un-postpone #3421014: Convert MigrateSource plugin discovery to attributes.