Create trait for getDefinitionFromEntity

I will work on. What name to use for the trait? Thanks

Here is the trait. Please review. thanks

@heddn, Thanks for reviewing patch. Removed un-used variable as mentioned in #6, Please review it :)

Didn't review the code, just saw that it needs work for the comment in #6 about using the new trait in ContentEntity and EntityContentBase.

Aww pants! It's only as I'm about to upload the patch that I notice this was tagged 'Novice'. Sorry.

I've taken a bit of a punt on this one: ContentEntity uses EntityFieldManager while EntityContentBase uses (deprecated) EntityManager so I have made changes to EntityContentBase (even though there are a number of other classes that extend it) rather than returning EntityFieldManager to (deprecated) EntityManager in ContentEntity.

After all that, perhaps this isn't as 'Novice' as it first seems (or I've missed a simple way to do it ... perhaps a conditional). Anyway I'll put this out there for review.

Needed a reroll and added some logic to the trait to get the entity_type_id from with the static function for destination plugins or the entity type for source plugins.

On a closer look maybe this is better, add a getEntityTypeId() method to ContentEntity, the only source plugin that uses the trait.

Sounds good. The only reservation I decided not to use PluginBase::DERIVATIVE_SEPARATOR because this isn't operating on derivatives. In one case it is the destination, like 'entity:file' and the other is the d8 source 'content_entity:node'.

Sigh, I was only thinking about the d7 node derivers at the time.My mistake.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -370,34 +372,6 @@ public function rollback(array $destination_identifier) {
-    $entity_type_id = static::getEntityTypeId($this->getPluginId());

ContentEntityBase is still defining ::getEntityTypeId() after this patch - should that also be removed here?

Right, fixed the item in #22.

Here are the results of searching for 'function getEntityTypeId' with the patch in #23.

grep -r 'function getEntityTypeId' core | grep destination
core/modules/book/src/Plugin/migrate/destination/Book.php:  protected static function getEntityTypeId($plugin_id) {
core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityRevisionTest.php:  public static function getEntityTypeId($plugin_id) {
core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityContentBaseTest.php:  public static function getEntityTypeId($plugin_id) {
core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php:  protected static function getEntityTypeId($plugin_id) {

The methods in Book.php and EntityFieldStorageConfig.php return specific strings, 'node' and 'field_storage_config'.

Nice clean-up here!

This is also a soft blocker to #2746541: Migrate D6 and D7 node revision translations to D8.

The trait seems like a great idea based on the diff, but I see a lot of red on protected methods, including on a base class. There's been no discussion on issue of whether these are the BC breaks they appear, or whether there's a way to deprecate instead?

This patch adds a trait with two methods. Then it removes pre-existing implementations of those two methods from a number of classes. So we need to check for BC implications. Let's do that:

1. +++ b/core/modules/migrate/src/EntityFieldDefinitionTrait.php
   @@ -0,0 +1,56 @@
   +  protected function getDefinitionFromEntity($key) {
   
   +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -370,34 +370,6 @@ public function rollback(array $destination_identifier) {
   -  protected function getDefinitionFromEntity($key) {
   
   +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -270,25 +272,4 @@ public function getIds() {
   -  protected function getDefinitionFromEntity($key) {
   

   Method signature remains identical ⇒ no backwards compatibility break! 🥳

2. +++ b/core/modules/migrate/src/EntityFieldDefinitionTrait.php
   @@ -0,0 +1,56 @@
   +  protected static function getEntityTypeId($plugin_id) {
   
   +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -110,20 +112,6 @@ public static function create(ContainerInterface $container, array $configuratio
   -  protected static function getEntityTypeId($plugin_id) {
   
   +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php
   @@ -121,14 +121,6 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   -  protected static function getEntityTypeId($plugin_id) {
   

   Method signature remains identical ⇒ no backwards compatibility break! 🥳

These positive BC observations are confirmed by the fact that zero function calls are being modified.

Hence confirming RTBC.

Just to make 100% certain that #28 is correct and that even subclasses are not affected:

<?php

// Without patch (HEAD)
class A {
    public static function hello() {
        return 'hello';
    }

    public static function say() {
        return static::hello() . ' world';
    }
}
class ASub extends A {
    public static function say() {
        return parent::say() . '!';
    }
}

// With patch
trait TestTrait {
    public static function hello() {
        return 'hello';
    }
}
class B {
    use TestTrait;
    public static function say() {
        return static::hello() . ' world';
    }
}
class BSub extends B {
    public static function say() {
        return parent::say() . '!';
    }
}

var_dump(A::say());
var_dump(ASub::say());

var_dump(B::say());
var_dump(BSub::say());

outputs

string(11) "hello world"
string(12) "hello world!"
string(11) "hello world"
string(12) "hello world!"

See https://3v4l.org/6gLqn

Excellent, thanks @Wim Leers!

I was concerned more about behavior changes.

I carefully read over each removed implementation and confirmed that the trait method should have the same result based on the new combined API. I especially like that we're replacing magic numbers with actual use of existing APIs. Nice work!

I thought about whether this needs a CR about the new trait, but since most folks will be using the base class and just get it that way, I think there's no need.

Updating the issue credit. Not sure if @heddn or @quietone had deliberately made some changes to the credit, but I'm crediting @yo30 for the initial patch, and @heddn, @Wim Leers, and @catch as reviewers.

For @mohit1604, nice work on providing a patch that removed the unused use statement! Next time though be sure to read carefully address both the reviewers comments, even if for the second one it's just asking for clarification.

Sorry for the extra commit noise; the policy has been that BC Migrate fixes are backported to the production branch, but then I noticed the issue is filed against 8.9.x. I confirmed with @catch that he also has been backporting Migrate fixes. So backported to 8.8.x (again). Thanks!

@Wim Leers, thanks for the excellent explanations so this could be committed.

You're most welcome, @quietone 😊