Implement Entity::fields() for migration destinations

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

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
We are not doing migrations at this time, so it is not tested for migrations in real life.

benjifisher → changed the visibility of the branch 2630732-migration-destinations-entity-fields to hidden.

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, some other minor changes and removed an item from the phpstan baseline. Curious that that was not discovered when running the commit checks locally.

I think that 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 📌 Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern Active . I am adding it as a related issue.

I added 📌 Add return-type declaration to Drupal\migrate\Plugin\MigrateDestinationInterface::fields() Active , 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 🌱 [meta] Known intermittent, random, and environment-specific test failures Active , 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 📌 Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern Active 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.

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 📌 Drupal\Tests\migrate\Unit\destination\EntityRevisionTest uses weird mocking pattern Active 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 🌱 [meta] Known intermittent, random, and environment-specific test failures Active and re-ran the tests. They both passed.

liam morland → made their first commit to this issue’s fork.

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.