Move content_entity source plugin to migrate module

I added a draft change record so that the deprecation messages can refer to it: https://www.drupal.org/node/3498916 →

There is a kernel test. We can just move that, right? Or do we need to lave behind a wrapper class and deprecate it?

I moved four classes: the source plugin, its deriver, and two test classes. Maybe that is enough for this issue.

I updated the phpstan baseline file. Maybe, as part of this issue, I should fix those errors and remove the entries from the baseline, instead of updating those entries. I might also add return-type declarations and use constructor promotion ... or maybe that is scope creep.

Let's see what the testbot thinks.

I reviewed this pretty closely, since we use the old classes as a wrapper we can't add return types since it's extending right?
Is there a way to do this so we can add those?

Also the CR is not public or attached to this issue so I could not review that, why is it restricted?

The CR was unpublished because I got confused by the base field and the regular boolean field, both of which (if you have access to them) are labeled "Published". I think you should be able to see it, now.

I suppose we do not have to make the old class a wrapper for the new one. We could just add the deprecation notice (in the class docblock and in the constructor) and remove the plugin annotations. Then we could add return-type declarations in the new class and it would have no effect on any code that extends the old class.

Yeah when creating a cr you shouldn't have to change any publish settings, maybe because you are on the security team you have more permissions.

I'll tweak the cr in a bit.

I think the change so we can add return types is worth it because we can remove those lines from phpstan when it's removed otherwise we have to do another compatibility dance when we add them.

Setting to needs work just for that change.

@nicxvan:

I did the following:

1. Restore the classes in the migrate_drupal module (but keep the deprecation notices).
2. Revert the changes to the phpstan baseline file.
3. In the new classes (in the migrate module), fix the errors from the phpstan baseline file.

Was I right to restore the test class in the migrate_drupal module? Maybe I should delete it again, along with the corresponding lines in the phpstan baseline file. I did not restore the test that refers to the source plugin by its plugin ID instead of by its class: that no longer tests the deprecated class in the migrate_drupal module.

Since we are giving up on keeping the new classes as close as possible to the old ones, I made some further modernization:

• Add parameter and return-type declarations.
• Use constructor promotion.

I had some trouble with phpstan. It is tricky when tests implement methods without declaring return types, for example.

If the tests pass, then I will move this issue back to NR.

This looks great, for baseline I run the following command to auto generate:
./vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/.phpstan-baseline.php

To be honest I'm not sure why you didn't have to add expect deprecation to the test marked with @legacy, I think class deprecations are different so I don't think you need to change anything.

I did compare the deprecation process docs for classes and confirmed you matched the documentation.

The only other thing I'm not sure of is the new unused parameter errors. I think since these are new classes now the preference is to ignore a single line. I'll ask for clarification on this in #core-development. Other than that looks great to me.

benjifisher → credited mondrake → .

I resolved the new "unused parameter" error after discussing on Slack with @nicxvan and @mstrlan.

@mondrake, thanks for the comments on the MR. I replied to all three, even though I only made the changes suggested in one of them. I am adding credit to you for the review.

I updated the Proposed resolution in the issue summary: we decided not to make the old classes wrappers for the new ones (because we are adding type declarations that could break BC).

I still wonder whether I should remove core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityConstructorTest.php, which tests the now-deprecated class. The MR copies that test class to core/modules/migrate/tests/src/Kernel/Plugin/source/ContentEntityConstructorTest.php.

Failing tests:

1. Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest
2. Drupal\Tests\file\Functional\DownloadTest
3. Drupal\Tests\package_manager\Build\PackageInstallTest
4. Drupal\Tests\package_manager\Build\PackageInstallSubmoduleTest
5. Drupal\Tests\package_manager\Build\PackageUpdateTest
6. tabbingManagerTest.js (I think: certainly one of the Nightwatch tests)

I am re-running the tests, and I will add a comment to 🌱 [meta] Known intermittent, random, and environment-specific test failures Active .

I think everything has been addressed. It's been moved instead of wrapping so we can add return types.

Look great!

my dream always was to stop using hook_update_N for content and write in place migrations exactly using this plugin

a followup, of course but still

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

There were merge conflicts in the PHPStan baseline file.

I rebased on the 11.x branch, skipping the commits that affected the baseline file. I also used the opportunity to simplify the git history. Then I manually removed the lines from the baseline for the removed test class and added an ignored error for the new version of that file with

phpstan analyze -c core/phpstan.neon.dist -b core/.phpstan-baseline.php

Back to NR to make sure I did not mess it up with my rebase. It looks as though PHPStan has already approved of the changes.

Made and applied one suggestion on the MR to add an additional type hint on a property.

Committed/pushed to 11.x, thanks!

catch → closed merge request !10869

Automatically closed - issue fixed for 2 weeks with no activity.