Implement Entity::fields() for migration destinations

This does the trick for the base fields.
We should look how we could get the more granular fields for each bundle. Not only for the drush command, but also for the UI. #2470916: Implement the migration detail page

Perhaps we should split this up, add the basefields here, and discuss the architectural changes needed to get the bundle specific fields in a seperate issue.

Some of the boilerplate for this could probably be grabbed from the patch at #2642612: Create Entity Lookup & Generate process Base plugins.

The hard part is knowing the destination bundle. Because it isn't always required (bundle-less entities) nor is it in the same place or named the same thing across different types of entities (taxonomy terms use vocabulary, nodes use type, other entities use something different).

Since #2700581: Destination bundle set in destination plugin, not in process is getting close, i started rerolling and expanding this one.
I added the base fields and expanded with all the fields.

I can probably fix the test fails here, but i'm not sure if injecting the EntityFieldManager and thus changing the constructor is even allowed from a BC perspective.
If anyone could confirm this is the good or wrong direction, i'm happy to work further on this, and bring this home.

Just a small test to see if i can figure out how to fix the test fails. This should fix 1 of the failing tests.

Next try, should work ;-)

Better create the correct patches, so ignore the previous one

And some more test fixes. Next up writing tests for the new code.

Woohoo test pass. However not sure how to add a test for the new method. So need to leave this open. Anyhow blocked on #2700581: Destination bundle set in destination plugin, not in process before this is functional and can be tested. So gonna leave it open for now.

And #2700581: Destination bundle set in destination plugin, not in process is in!

The patch was no longer working for me in 8.x-2.x-dev:

patch -p1 < 2630732-fields-migration-destionation-17.patch
patching file core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
patching file core/modules/file/src/Plugin/migrate/destination/EntityFile.php
Hunk #1 FAILED at 4.
Hunk #2 FAILED at 40.
Hunk #3 FAILED at 48.
Hunk #4 FAILED at 66.
4 out of 4 hunks FAILED -- saving rejects to file core/modules/file/src/Plugin/migrate/destination/EntityFile.php.rej
patching file core/modules/migrate/src/Plugin/migrate/destination/Entity.php
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
Hunk #1 succeeded at 5 with fuzz 1.
Hunk #2 succeeded at 50 (offset 1 line).
Hunk #3 succeeded at 75 (offset 1 line).
patching file core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityContentBaseTest.php
Hunk #1 succeeded at 42 (offset 1 line).
Hunk #2 succeeded at 55 (offset 1 line).
Hunk #3 succeeded at 69 (offset 1 line).
Hunk #4 succeeded at 99 (offset 1 line).
patching file core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php
patching file core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
patching file core/modules/user/src/Plugin/migrate/destination/EntityUser.php
Hunk #2 FAILED at 44.
Hunk #3 succeeded at 53 (offset -2 lines).
Hunk #4 succeeded at 70 (offset -2 lines).
1 out of 4 hunks FAILED -- saving rejects to file core/modules/user/src/Plugin/migrate/destination/EntityUser.php.rej

The file EntityFile.php no longer has its constructors and merely extends from EntityContentBase.php so I have just removed that file. Finally I have corrected the minor issue with a comment in EntityUser.php

Thanks, i rerolled your work against 8.3.x, and fixed the new test fails. This is probably 8.3.x material at this time.

Mostly nits & questions. The last one, if I understand what is going on correctly though, should be addressed.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -107,7 +119,22 @@ public function getBundle(Row $row) {
   +    $entity_type_id = self::getEntityTypeId($this->getPluginId());
   

   Should this be $this?
   Does this handle revisions too? I'd like to see a test using both a revision and a non-revision.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -107,7 +119,22 @@ public function getBundle(Row $row) {
   +    $fields = array();
   

   New code, is it possible to use the new array syntax? Check around, are we already using it in this class?

3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -107,7 +119,22 @@ public function getBundle(Row $row) {
   +    foreach ($base_definitions as $base_field) {
   ...
   +    if (isset($this->configuration['default_bundle'])) {
   

   I think we should do an if/else here. If we have the default bundle, no need to add extra fields into the mix.

   i.e. if default bundle... gather them. else gather all from base field definitions.

Thanks for the review.

1. Since protected static function getEntityTypeId() is a static function and Entity is an abstract class self is more appropriate if i recall correctly.
2. No other arrays in this class, but changed to new syntax. Nice catch.
3. It's actually the other way around, when a default_bundle is around we can add the default specific fields. This value is set in the migration destiny. it was added in #2700581: Destination bundle set in destination plugin, not in process. So i think this way is correct. Add the default entity fields, and when we got the bundle specified in 'default_bundle' add the bundle specific fields as well.

I don't think revisions are on order here. Migration destinations are not revisionable AFAICT.
This indeed needs some tests, but i have not really an idea how to start with those tests.

Thanks for the feedback. This causes some inconsistency in the constructors, but that's probably not a problem. Let's remove also the test changes, to see if this is acutally BC.

With help from @chx, i rewrote the fields method, and finally found a way to add a test.

Instead of showing only basefields for a destination we return all fields from all bundles of the entity. This is actually also more logical, and does what the interface says, show all posibble destination fields.

I also added a test, and did some minor code changes. Interdiff is added to be complete, but almost size of the patch.

Just to note: According to me the entity destination class is actually untested.

Thanks all for the work on this!

Just tried the patch, and found a problem: This fails with a LogicException when the entity type is not fieldable (see EntityFieldManager::buildBaseFieldDefinitions). This is the case, for example, when migrating node type configuration entities and probably quite common.

Removing the bc breaking tag since we're providing the additional construction argument.

Just attaching an updated patch to apply to both 8.3.x + 8.4.x branch (minor adjustments to EntityComment.php. Pushing to needs review just to trigger testbot.

Thanks for the reroll. We still need to address the exception thrown on non-fieldable entities (e.g. config entities or custom content entities), which is probably easier once we have a test for that :)

$this->entityFieldManager->getFieldDefinitions and $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id); will both throw a LogicException in \Drupal\Core\Entity\EntityFieldManager::buildBaseFieldDefinitions via

  if (!$entity_type->isSubclassOf(FieldableEntityInterface::class)) {
    throw new \LogicException("Getting the base fields is not supported for entity type {$entity_type->getLabel()}.");
  }

Basically, we might add that check ourselves and just bail out if the interface is not implemented by our destination entity type.

Do an is_subclass_of makes sense. I would expect that.

We tried this patch with a book migration wich has as destination "plugin: book".
Because the function getEntityTypeId just cuts the first 7 letters to remove "entity:" that fails.
As quick fix we used this code:

public function fields(MigrationInterface $migration = NULL) {
  $entity_type_id = self::getEntityTypeId($this->getPluginId());
  if (!$entity_type_id && $this->storage) {
    $entity_type_id = $this->storage->getEntityTypeId();
  }

Patch doesn't apply on 8.5.x unfortunately.

Reroll.

Still need to address #36.

I've just added a check for #36 over in #2935951-59: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal. Thanks for the pointer.

And I've added the same check here. And moved the fieldable logic down into EntityContentBase. I'm assuming that config entities are normally not fieldable. There's also tests now, so I've removed that tag.

And cleaned up the DI and imports for the base Entity class here.

Came across this waiting for a long test to run.

Fixed the failing test by declaring the entityFieldManger in EntityContentBase. Also did some renaming of the test migrations and node type to something more descriptive that just appending a '2' to signify the difference. The intentions was to make the code easier to understand.. There are also several fixes for coding standards.

Retest with 8.6.x

Need to figure out a solution for the problem with not knowing the destination bundle. Since we don't know it, we cannot really pull up destination fields.

Perhaps we "standardize" on a method to designate the destination bundle that is optional. But if it is used, then trigger the Entity::fields logic for a destination.

My proposed standard is entity:node:page or entity:node:blog, etc.

Bumping back to NW for this piece.

@heddn i'm not really sure i can follow the comment, as i saw it initially (and seems the patch still follows that). We can only show the base fields when we don't have a bundle specified. When we don't specify the bundle, the migration can migrate to different bundles in one migration, so we can't set a default one here?
This would mean, that fields() can only return the fields it's certain about, not all possible fields, but i think that's good. Better showing only correct fields, even if that means those are not all available fields, then showing possible 'wrong' fields.

If we want to do this in 2 phases. Show base fields and bundle fields in a follow-up, that's fine. Let's open the follow-up though and link it in as an @TODO in this patch.

Reroll of #47 for 8.7.x.

+++ b/core/modules/migrate/tests/modules/migrate_destination_test/migrations/node_with_fields.yml
@@ -0,0 +1,12 @@
+  default_bundle: test_node_type_with_fields

I'm assuming the 'default_bundle' property is something this patch invents -- so it needs to be documented in the class docs for the destination plugin.

I don't really follow #50 either... is 'default_bundle' not sufficient here?

'default_bundle' was introduced in #2700581: Destination bundle set in destination plugin, not in process

Since an entity migration can migrate to different bundles in one migration we cannot specifiy the 'non' base fields of the target entity. When the migration specifices the 'default_bundle' property, we know it migrates to that specific bundle of the entity, and can request the non base fields of that bundle to add them in the destination.

I think #50 also missed the introduction of that in the 'preceeding' issue where it was introduced.

Hello,

Two small issues related this patch (#55) maybe worth saying :

When testing a 'drush mfd' command (see: https://www.drupal.org/project/migrate_tools/issues/2630718) for a paragraphs destination migration. I have found that 'self::getEntityTypeId($this->getPluginId());' should be replaced by 'static::getEntityTypeId($this->getPluginId());'
inside the EntityContentBase:fields() function.

Afterwards, I also had to modify the _construct() function of EntityReferenceRevisions and EntityRevision classes to add the last parameter $entity_field_manager.

With these two small modification, I was able to execute the drush command that shows all migration destination fields for my paragraph type.

Hope this helps.

rerolled for 8.8.x

NW for reroll, #53 and #57

This should fix the php error.

This should fix the failed test case.

Added fix for failed test

This is still NW for #53 and #57

Looks like all changes that was offered in #57already implemented in 8.9 branch:

I have found that 'self::getEntityTypeId($this->getPluginId());' should be replaced by 'static::getEntityTypeId($this->getPluginId());'
inside the EntityContentBase:fields() function.

 /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration = NULL) {
    $entity_type = static::getEntityTypeId($plugin_id);

I also had to modify the _construct() function of EntityReferenceRevisions and EntityRevision classes to add the last parameter $entity_field_manager.

class EntityRevision extends EntityContentBase {

  /**
   * {@inheritdoc}
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityFieldManagerInterface $entity_field_manager, FieldTypePluginManagerInterface $field_type_manager) {
    $plugin_definition += [
      'label' => new TranslatableMarkup('@entity_type revisions', ['@entity_type' => $storage->getEntityType()->getSingularLabel()]),
    ];
    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $entity_field_manager, $field_type_manager);

Patch needs reroll for 9.1.x
Patch applies cleanly to 9.0.x

I have re-roll the patch for 9.1.x

The test module here needs to be updated to work with Drupal 9.

Trying to fix the test cases

Fixing failed test!

Started to review this and the changes to the User entity caught my eye, Why are those needed? They are not, so they are gone. And I've added a test for a user destination for verification. That required a changed to EntityContentBase::fields to get the fields for an entity that does not have bundles and therefor would be not setting the 'default_bundle' in the destination plugin configuration.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -375,4 +375,26 @@ public function getHighestId() {
+    $bundle = empty($this->configuration['default_bundle']) ? $entity_type->id() : $this->configuration['default_bundle'];
+    $field_definitions += $this->entityFieldManager->getFieldDefinitions($entity_type->id(), $bundle);

I think we can also pass the bundle in other ways. We should also return early or do some logic around this for when we don't know the bundle but the entity type is a bundle-able entity. I'm OK with not returning all the bundle fields, as returning something is better then nothing. But we should at least be a little more defensive.

@heddn I don't quite follow your points in #78.

> I think we can also pass the bundle in other ways

Could you say what other ways? Do you mean we should check for other ways in which the migration's destination bundle is defined in the migration?

> I'm OK with not returning all the bundle fields, as returning something is better then nothing.

Is the 'not' a typo in that sentence? It seems to contradict itself!

    $bundle = empty($this->configuration['default_bundle']) ? $entity_type->id() : $this->configuration['default_bundle'];

I agree this isn't quite right. For one thing, if a developer forgets to put in 'default_bundle' and it IS an entity type with bundles (maybe they are mapping the bundle from source data?), then this will take the entity type ID as the bundle, and pass it to entityFieldManager->getFieldDefinitions()... which will possibly throw an exception for an invalid bundle?

Reroll for no-fuzz on 9.3.x.

The patch has been in use for over a year with the WXT distribution. The patch applies on D9.1.x

If someone has a specific suggestion with the patch or implementation please provide helpful feedback.

There's no interdiff for this, straight up reroll.

wow ya, code style restrictions, ftlog.

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -374,4 +374,26 @@ public function getHighestId() {
   +  public function fields(MigrationInterface $migration = NULL) {
   

   $migration isn't used, nor is it on the parent method. Do we need it?

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -374,4 +374,26 @@ public function getHighestId() {
   +    $bundle = empty($this->configuration['default_bundle']) ? $entity_type->id() : $this->configuration['default_bundle'];
   

   A bundle can be provided in 2 or 3 different ways. And if isn't set by default bundle, we would pass null value along to $this->entityFieldManager->getFieldDefinitions and get PHP errors.

   But let's take node as an example. There are base fields on it we could return; so return those, at least.

   We can try some of the other common ways to get the "bundle". Actually, that isn't really true. At the point that fields() is called, there be might be very little context provided. The only thing we really have is "default_bundle", because we don't have a Row object. But that means, we need to just return the base fields without any bundle ones. i.e. getBaseFieldDefinitions

Thanks for clarifying @heddn

> We can try some of the other common ways to get the "bundle".
> At the point that fields() is called, there be might be very little context provided.

Here's what I think we can try at that point, in the order to attempt them (and complexity)

1. default_bundle
2. if the destination entity type has no bundles: there are no bundle fields (e.g. user)
3. if the destination entity type has only 1 bundle, it must be that one (e.g. if there's only the 'article' node type defined)
4. if the bundle field on the destination entity is mapped from a constant value, it's that
5. if the bundle field on the destination entity is mapped from a source field, and that source field is in the source query as a fixed value condition, it's that

I'm not sure about whether 4 is worth the effort. 5 is definitely not worth the effort IMO. 2 and 3 are pretty simple though.

I think I agree about 1,2,3. But 4 and 5 require data outside the scope of the destination and might not be data available so let's keep it simple for now. maybe we can hit those in follow-up issues, if we even think that is worth it.

> I think I agree about 1,2,3. But 4 and 5 require data outside the scope of the destination and might not be data available so let's keep it simple for now. maybe we can hit those in follow-up issues, if we even think that is worth it.

Agreed. Maybe a follow-up for 4.

Came to review but the IS is not up to date. In #85 and #86 it was agreed that certain cases should be implemented. Has that happened? Tagging for IS update.

Done.

@joachim, thanks that helped!

Working on fixing the tests...

Fixed tests and moved stuff from the test module into the test class as in the review.

Unsure about a couple of points in @quietone's review -- see review threads.

The above merge request no longer applies cleanly to the head of 9.3.x

wget https://git.drupalcode.org/project/drupal/-/merge_requests/907.diff
 ╰❯ $ patch -p1 --dry-run < 907.diff 
checking file core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
Hunk #1 succeeded at 4 with fuzz 2.
Hunk #2 FAILED at 54.
Hunk #3 FAILED at 74.
2 out of 3 hunks FAILED
checking file core/modules/migrate/src/Plugin/migrate/destination/Entity.php
checking file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
Hunk #1 succeeded at 6 with fuzz 1.
Hunk #2 succeeded at 101 with fuzz 2 (offset -3 lines).
Hunk #3 FAILED at 130.
Hunk #4 FAILED at 150.
Hunk #5 succeeded at 422 (offset 40 lines).
2 out of 5 hunks FAILED
checking file core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
Hunk #1 succeeded at 5 with fuzz 2.
Hunk #2 FAILED at 115.
1 out of 2 hunks FAILED
checking file core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
Hunk #1 succeeded at 317 with fuzz 2 (offset 224 lines).
checking file core/modules/migrate/tests/src/Kernel/MigrateEntityDestinationTest.php
checking file core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityContentBaseTest.php
Hunk #1 succeeded at 8 with fuzz 2.
Hunk #2 FAILED at 89.
1 out of 2 hunks FAILED
checking file core/modules/user/src/Plugin/migrate/destination/EntityUser.php
Hunk #2 FAILED at 94.
Hunk #3 FAILED at 116.
2 out of 3 hunks FAILED

The source branch is 379 commits behind the target branch

Rebased and conflicts fixed.

First push was rebase to 9.3.x but then I realized I need to do it to 9.4.x and that is the second push. No conflicts with the second one.

Rebased to 9.4.x.

In Slack, joachim asked me to review this.

I don't seem to be able to get to the MR comments to mark them as resolved -- possibly because I've rebased?

Anyway, points from reviews have been addressed:

- documentation of new @throw
- moved new test into the existing class.

this patch is a snapshot of todays MR 902
https://git.drupalcode.org/project/drupal/-/merge_requests/907.diff

new patch for D10.0.x / D10.1.x

straight up reroll of #103

The issue description makes this seem straightforward: implement the method. But looking at the patch, it seems more complicated than that. We are adding parameters to constructors, for example. I see some discussion of backwards compatibility (BC) in Comments #10, #28, and others. But I would like to see that discussion (and why we need to change the constructors for this issue) in the issue summary, so I am adding the tag for that. It will be easier to review this issue if we do not have to read all the comments to get that explained.

The patch in #107 failed static analysis. Unfortunately, the testbot does not set the issue to NW when that happens, so I am changing the status now. Just looking at the PHPStan report, and not looking too closely at the patch, it seems that the static analysis is catching a pretty serious problem:

constructor invoked with 9 parameters, 10 required.

> We are adding parameters to constructors, for example. I see some discussion of backwards compatibility (BC) in Comments #10, #28, and others.

Adding to constructors is a fairly standard change. We need the bundle info service to get the bundles of the entity type in question.

We do need to add BC handling, even though technically it's internal, as lots of things will be extending this class.

> But I would like to see that discussion (and why we need to change the constructors for this issue) in the issue summary, so I am adding the tag for that. It will be easier to review this issue if we do not have to read all the comments to get that explained.

That's already in there, where it says "To detect the destination bundle, the following methods should be tried, in this order".

Resolved the custom command fails.

> 2. the destination entity type has no bundles, in which case there are no bundle fields (e.g. user)

It occurs to me that it would still be possible to define bundle fields on an entity type with no bundle. For example, you might have something that always defines fields as bundle fields, on different entity types.

I've not checked the code, but what it should do here is take the one single bundle that's defined by default by the entity system.

Just attaching a re-roll for 9.5.x so patch works against 9.5.10

Just attaching a re-roll for 9.5.x so patch works against 9.5.10

At first I wanted to rebase the original MR, but this was not practical.
So now a new MR.

Left some comments on the MR.

I see @quietone has been involved so the submaintainer tag may not be needed but added just in case.

The patch #113 no longer applying for me in 10.3

I post our patches for D10.2(.7) and 10.3.
The patches are the changes from patch 113 adapted to the current code.
We are not doing migrations at this time, so it is not tested for migrations in real life.

In Comment #109, I asked for an issue summary update.

In Comment #110, the tag was removed without updating the issue summary.

Comment #121 adds the tag for subsystem maintainer review. Please do what the subsystem maintainer requests before adding that tag back.

Since DrupalCI shut down, we no longer run automated tests on patches. If anyone cares to move this issue forward, then it would help to convert the latest patch to a MR (and to update the issue summary).

I am hiding the original MR (for 9.4.x). The newer MR (for 11.x) needs work.

> We are adding parameters to constructors, for example. I see some discussion of backwards compatibility (BC) in Comments #10, #28, and others. But I would like to see that discussion (and why we need to change the constructors for this issue) in the issue summary, so I am adding the tag for that. It will be easier to review this issue if we do not have to read all the comments to get that explained.

We need to change the constructors because the EntityContentBase class will need the 'entity_type.bundle.info' service injecting so it can know about bundles.

I do not think this sort of detail needs to be explained in the proposed resolution. Adding a service is like adding a variable.

Thanks for updating the issue description.

It would be nice to finish up this old issue.

I had a look at the MR. (I got most of the way through it, but it is getting close to bed time.) I may have more comments the next time I review it.

I left some suggestions on the MR. @smustgrave, in #121 you said that you also commented on the MR, but I do not see those comments.

I am afraid that the MR also needs to be updated to resolve conflicts.

I am also not sure why this issue ever had the tag for subsystem maintainer review. Not every issue in the migration system needs that. Like most Drupal issues, it needs someone to write the code and someone to review it. In general, changing the status of an issue from NR to NW is not a good way to get the attention of the migration maintainers.

> ?AccountSwitcherInterface $account_switcher = NULL

$account_switcher should not be optional here.

It was optional for BC, but that handling was removed in f58fccc2fc690b62e1beac6a6d1c2b3d8a61c31a -- #3261004: Remove deprecated code from the migration system . The parameter should have been changed in that issue!

Fixing that here, but happy to cherry-pick it to a separate, blocking issue if preferred.

I've addressed all the points in the review.

The MR is not letting me ticky things to say I've resolved them though :/

I replied to a few threads on the MR.

Since the MR adds a parameter to the constructor of EntityContentBase, this counts as an API change. This class is intended to be extended, so it cannot be final nor @internal.

The API change should be documented in the issue summary, under the "API changes" section. I am adding the tag for an issue summary update again.

We also need to deprecate calling the constructor without the new parameter. See the "Constructor parameter additions" section in Drupal deprecation policy.

Done.

I modified the test to use a mock migration and that lead to changing variable names and moving things around a bit. Also, some other minor changes and removed an item from the phpstan baseline. Curious that that was not discovered when running the commit checks locally.

Also ... removed an item from the phpstan baseline. Curious that that was not discovered when running the commit checks locally.

I think that difference is explained by this code block in commit-code-check.sh:

# Run PHPStan on all files on DrupalCI or when phpstan files are changed.
# APCu is disabled to ensure that the composer classmap is not corrupted.
if [[ $PHPSTAN_DIST_FILE_CHANGED == "1" ]] || [[ "$DRUPALCI" == "1" ]]; then
  printf "\nRunning PHPStan on *all* files.\n"
  php -d apc.enabled=0 -d apc.enable_cli=0 vendor/bin/phpstan analyze --no-progress --configuration="$TOP_LEVEL/core/phpstan.neon.dist"
else
  # Only run PHPStan on changed files locally.
  printf "\nRunning PHPStan on changed files.\n"
  php -d apc.enabled=0 -d apc.enable_cli=0 vendor/bin/phpstan analyze --no-progress --configuration="$TOP_LEVEL/core/phpstan-partial.neon" $ABS_FILES
fi

@quietone, thanks for working on this issue.

I made several suggestions on the MR. Most of them are minor, like adding the optional comma at the end of an argument list. If you feel strongly about using the plugin manager in the test class instead of the helper function, then I will not insist.

In #128, I wrote,

I had a look at the MR. (I got most of the way through it, but it is getting close to bed time.) I may have more comments the next time I review it.

I have now reviewed the entire MR.

We need a change record to document the API change, so I am adding the tag for that.

I am adding a tag for a follow-up issue because of the comment on core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php.

I think I've addressed all the comments apart from the ones about createDestination().

I tried adding a plugin ID to createDestination(), but then hit the problem that createDestination() uses the $this->storage property, and in the testFields() test we are using a different entity type.

I find createDestination() a bit problematic, as it's a mixture of taking parameters and reading properties, and updates a property rather than returning the destination object. For testFields() which sets up the a destination plugin multiple times, not explicitly seeing $destination being changed worsens readability.

> We need a change record to document the API change, so I am adding the tag for that.

I don't see why we need this. It's not an API change - the fields() method was there all along, just that EntityContentBase was failing to correctly implement it. If anything, this is a bugfix.

There are arguments on both sides of using createDestination() (previous two comments). As I said, I will not insist: we can agree to disagree. Still, from the MR comment:

createDestination() also hardcodes the entity type the destination is made with, because it uses $this->storage.

and

That will have the wrong entity storage.

I did run the test with the suggested changes. Probably it fails if you apply some but not all of them. But the basic pattern works:

    $this->storage = $entity_type_manager->getStorage('entity_test_with_bundle');
    $this->createDestination([]);
    $fields = $this->destination->fields();

I tried adding a plugin ID to createDestination(), ...

It turns out that is not needed. Just setting $this->storage is enough.

I don't see why we need this. It's not an API change - the fields() method was there all along, just that EntityContentBase was failing to correctly implement it. If anything, this is a bugfix.

It is both a bug-fix and an API change.

Custom or contrib modules may extend EntityContentBase, just as some of the core destination plugins do. If so, then those modules will need to add the new parameter, just as entity:comment and entity:user do in this MR.

Another way to put it: in order to fix the bug, this issue makes an API change.

Thanks for updating the MR. I do not have time now to re-review.

I did another round of review. Some of my earlier comments were not addressed, and I added a few new comments on the MR.

Let's make sure the PHPUnit tests pass.

CR done and I think that's the rest of things fixed.

Thanks for the updates. This is very close: just one more @var comment should be nullable. (See the MR.)

Thanks for the CR. I added a line, and I am removing the issue tag.

Follow-up issues

I am removing these lines from the issue summary:

Make a follow up for the following, as it's a rarer case and more complicated to handle:

4. the migration's process mapping has the bundle field on the destination entity mapped from a constant value

In my opinion, that is a bad idea. The migration plugin knows about the destination plugin, and should pass configuration to it. The destination plugin should not know about the migration that is using it. Implementing this suggestion would add a lot of complexity to the system in order to make a small DX improvement when writing migrations. (See also Comments #86, #88, ...)

Thanks for adding #3474640: Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern. I am adding it as a related issue.

I added #3477244: Add return-type declaration to Drupal\migrate\Plugin\MigrateDestinationInterface::fields(), but I am not sure how we implement a change like this. If we just add a return-type declaration to the interface, then we will break any class that implements the interface and does not already have that declaration.

I am removing the issue tag for a folow-up.

> In my opinion, that is a bad idea. The migration plugin knows about the destination plugin, and should pass configuration to it. The destination plugin should not know about the migration that is using it. Implementing this suggestion would add a lot of complexity to the system in order to make a small DX improvement when writing migrations. (See also Comments #86, #88, ...)

Yup, it would certainly add a lot of complexity. Fair enough that it's too much, and architecturally dubious, for a minor DX improvement.

I've applied your suggestion for the nullable type to the MR.

Thanks for all the work on this, @quietone and @joachim! I think this issue is ready to go.

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.

The MR no longer applied.

I rebased on the current 11.x, and the only conflict was in core/.phpstan-baseline.php. There is a single commit, removing one item from that file. The context (3 lines above and below the removed item) has changed, so that commit did not apply. During the rebase, I skipped that commit, then made the same change in a new commit.

Given the nature of the change, I feel I can do that and still keep my role as a reviewer of this issue.

Now that the merge conflict is resolved, PHPStan complains:

------ ------------------------------------------------------------------------
Line core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
------ ------------------------------------------------------------------------
370 Method
Drupal\Tests\migrate\Kernel\MigrateEntityContentBaseTest::testFields()
has no return type specified.
------ ------------------------------------------------------------------------
[ERROR] Found 1 error

That should be easy to fix, but it is a little more than I should do as a reviewer. I am leaving the status at NW, and adding the Novice tag to add a return type to that function.

Working on it.

Heyy @benjifisher,
Fixed the missing return type issue.
Kindly check :)

@lavanyatalwar:

Thanks for fixing that. It may feel like a small thing, but helping to finish an issue that is 99% done has as much impact as completing an unstarted issue all by yourself.

Some unrelated tests were failing. That seems to happen a lot recently. Maybe I should add a note on #2829040: [meta] Known intermittent, random, and environment-specific test failures, but I was lazy and just re-ran the failing tests. They all pass, now.

Back to RTBC, The CI job and I agree that @lavanyatalwar correctly added the return type reported by PHPStan. I am updating the "Remaining tasks" in the issue summary and removing the Novice tag.

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.

No good deed goes unpunished: our follow-up issue #3474640: Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern was recently Fixed, and it creates a conflict with this issue.

Resolving the conflict is a little more than what I am comfortable doing while acting as a reviewer on this issue. (I already did that once: see Comment #144. The changes now are a little more complicated.)

I rewrote the git history, combining all the commits that change core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php, and force-pushed. You can confirm that I only changed history by comparing with the tag for the previous HEAD of the feature branch:

$ git diff previous/2630732-11-x-migration-destinations-entity-fields/2024-11-05 && echo done
done

That should make it easier to rebase on the current 11.x. I think that is considered a Novice task, so I am adding that tag again.

When rebasing, pay attention to the improvements that #3474640 made to the other class variables:

• Add a type declaration.
• Call reveal() in the setUp() method, not the test/helper method(s).

Hi, I attempted to resolve the merge conflict, please review.

@baltowen:

Thanks for working on this issue. Unfortunately, core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php fails after the changes you made. Back to NW.

There are also some unrelated tests that fail. For now, do not worry about those.

This issue adds the $entityTypeBundle class variable to the test class. After your rebase, that variable is handled correctly: call reveal() in the setUp() method, not in the test methods. But your rebase also changes the way $storage is handled. That is not in scope for this issue, and Comment #4 on #3474640: Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern explains why that one variable should not follow that pattern.

If you undo the changes to $storage in the test class, then that test should pass. Then we can worry about any unrelated test failures.

Thanks @benjifisher for the review. I have reverted the change of $storage.

@baltowen:

Thanks for fixing that. There is still one line that needs to be fixed: see my comment on the MR. I wish I had caught that sooner.

There is also one unrelated test failure: Drupal\Tests\block\Functional\BlockCacheTest::testCachePerPage. Again, let's not worry about that until we fix the problem we know about.

Back to needs review.

@baltowen:

Thanks for fixing that.

Next time around, please add additional commits instead of making your changes as part of a rebase. I had to compare to the tag previous/2630732-11-x-migration-destinations-entity-fields/2024-11-05 in order to make sure that there were no accidental changes added during the rebase.

The merge conflicts were in core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php. I reviewed that carefully, and the changes there look good to me. Back to RTBC.

I forgot to review the failing tests:

1. Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStyleUrlAndPathPrivate
2. Drupal\Tests\responsive_image\FunctionalJavascript\ResponsiveImageFieldUiTest::testResponsiveImageFormatterUi

I added a note to #2829040: [meta] Known intermittent, random, and environment-specific test failures and re-ran the tests. They both passed.

I was using 6111.diff as a patch in composer for core 10.3.6. Commits pushed in the last 1-2 days broke this and now it no longer applies. Here is a patch that captures all the latest changes and still applies to 10.3.x.

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.

The bot is wrong; the patch is for Drupal 10.3.

The deprecation links in the MR need to point to a change record, other than that there seems to still be a lot of open threads.
Can we get those resolved please? If the only change is to the deprecation link URLs, fine to self RTBC

I believe this one has required a few re-rolls so feel free to ping me for another look - thanks everyone

I have updated the link to point to the change record.

I resolved several threads. I left open the ones where I made a suggestion and it has not been implemented. I still think these are improvements.

Unfortunately, one fix seems to have been lost, so back to NW. It is pretty small: just a missing |null in an @var comment.

I have rebased and fixed the missing comment mentioned in #166.

I reviewed all the changes, and I do not see anything else that got lost.

I still think it is a bad idea to cast to string in EntityContentBase::fields(), and some of my other suggestions would be improvements. But I will not insist on those changes, so RTBC.

Patch #161 not working with 10.4.x, which is supposed to be LTS for D10. Tested patch created from the merge request's latest commit ID 021da95f and works with 10.4.x, adding patch here.

The are conflicts in the phpstan baseline.

@smulvih2, Yes, Drupal 10 is an LTS, but not all changes will be backported. There is more detail on that at release overview page and the allowed changes in each version.

As in Comment #144, I think I can rebase and resolve the conflict in .phpatan-baseline.php even though I am the reviewer of this issue. Back to RTBC.

The conflict was caused by #3486713: Bump PHPStan to version 2.0.4, which changed the format of the baseline file.

Since Benji RTBCed this, I'm going to assume his suggestions/comments on the MR can be resolved at this point. That is now done. Otherwise, +1 on RTBC.

@heddn:

In Comment #166, I wrote,

I resolved several threads. I left open the ones where I made a suggestion and it has not been implemented. I still think these are improvements.

and in #168,

I still think it is a bad idea to cast to string in EntityContentBase::fields(), and some of my other suggestions would be improvements. But I will not insist on those changes, so RTBC.

I think you may have resolved some threads that I intentionally left open.

This all looks good. I had to update the deprecation message to reference 11.2.

Committed/pushed to 11.x, thanks!