Entity stubs doesn't follows fallback logic from entities and leads to a broken migration

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Thank you for a super clear issue summary.

The proposed solution lines up with the patch.
#2 shows the issue

Triggering for 10.1

No failures.

+++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
@@ -304,4 +308,65 @@ public function testStubRows() {
+    $decorated_entity_field_manager = new class($entity_field_manager) implements EntityFieldManagerInterface {
+
+      public function __construct(
+        protected EntityFieldManagerInterface $entityFieldManager,
+      ) {}
+
+      public function getFieldDefinitions($entity_type_id, $bundle) {
+        if (\is_null($bundle)) {
+          throw new \Exception("Bundle value shouldn't be NULL.");
+        }
+
+        return $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id, $bundle);
+      }
+
+      public function getBaseFieldDefinitions($entity_type_id) {
+        return $this->entityFieldManager->getBaseFieldDefinitions($entity_type_id);
+      }
+
+      public function getFieldStorageDefinitions($entity_type_id) {
+        return $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id);
+      }
+
+      public function getFieldMap() {
+        return $this->entityFieldManager->getFieldMap();
+      }
+
+      public function setFieldMap(array $field_map) {
+        return $this->entityFieldManager->setFieldMap($field_map);
+      }
+
+      public function getFieldMapByFieldType($field_type) {
+        return $this->entityFieldManager->getFieldMapByFieldType($field_type);
+      }
+
+      public function clearCachedFieldDefinitions() {
+        return $this->entityFieldManager->clearCachedFieldDefinitions();
+      }
+
+      public function useCaches($use_caches = FALSE) {
+        return $this->entityFieldManager->useCaches($use_caches);
+      }
+
+      public function getExtraFields($entity_type_id, $bundle) {
+        return $this->entityFieldManager->getExtraFields($entity_type_id, $bundle);
+      }
+
+    };
+
+    $this->container->set('entity_field.manager', $decorated_entity_field_manager);
+    $this->createEntityStub('migrate_string_id_entity_test');

could we simplify this if the anonymous class extended from the existing entity field manager?

Thank you, looks ready again

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php	(date 1680103443219)
@@ -330,7 +330,7 @@
+    $bundle_key = $this->getKey('bundle') ?: $this->storage->getEntityTypeId();

I don't think this is the correct fix, a 'bundle key' is the name of the row property that contains the bundle field.

I think instead we should change this line

 $fields = $this->entityFieldManager
      ->getFieldDefinitions($this->storage->getEntityTypeId(), $row->getDestinationProperty($bundle_key));

which is a few rows lower.

I think it should be

 $fields = $this->entityFieldManager
      ->getFieldDefinitions($this->storage->getEntityTypeId(), $row->getDestinationProperty($bundle_key) ?? $this->storage->getEntityTypeId());

Sorry for not picking that up in my earlier review.

Fine to self RTBC after making that change.

I was tempted to just make that change on commit but felt it was best to go through the peer review process.

Once again, sorry for not picking that up the first time.

Btw, should such tests be marked somehow? Because when an interface will start using typehints this test becomes pointless, it will be covered by PHP directly.

Ordinarily we'd add a followup for that, i.e a new issue 'remove test x once [something] is complete' and put an // @todo remove this class in [url to followup] that way when someone resolves the issue they can search by URL for todos.

I'd support doing that here if you're rerolling for the above, as I agree, a proper type hint would have prevented this.

Retested and #17 addressed

Pushed to 10.1.x and 10.0.x

Queued a 9.5.x test and will backport there too if it passes

The patch doesn't apply to 9.5.x, so changing to patch (to be ported)

Committed/pushed to 9.5.x, thanks!