Comments: entity_id base field not overridden correctly in comment bundle field definitions for comment bundles attached to different entity target_type

I can confirm that \Drupal\comment\Entity\Comment::bundleFieldDefinitions() sets the target_type property of the entity_id field incorrectly. The problem is the way that the clone keyword behaves in PHP. As http://php.net/clone explains:

When an object is cloned, PHP 5 will perform a shallow copy of all of the object's properties. Any properties that are references to other variables will remain references.

$base_field_definitions['entity_id'] has a property named itemDefinition, which is an instance of the class \Drupal\Core\Field\TypedData\FieldItemDataDefinition. This object property is actually a reference to another object, so even though $base_field_definitions['entity_id'] is cloned into a new object, the itemDefinition property of the new object remains a reference to the same object as the original instance of $base_field_definitions['entity_id'] references. Therefore, the following line of code actually overrides the field definition of the entity_id field on all comment bundles that have already been built during the current HTTP request:

$fields['entity_id']->setSetting('target_type', $comment_type->getTargetEntityTypeId());

The result is that the bundle-specific overrides for all of the comment bundle field definitions that were already built during a given HTTP request are overridden by the settings for the next comment bundle's field definitions. However, before the process of building a comment bundle's field definitions overwrites the already-built field definitions of other comment bundles, those definitions have been saved to cache by \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions(). On the next HTTP request, after the cache has been warmed, EntityFieldManager::getFieldDefinitions() loads the correct field definitions for a given bundle out of cache. Therefore, this bug affects the copy of the field definitions in PHP memory during a single HTTP response after a cache clear, or when the discovery cache bin is disabled/set to null.

I'm attaching a KernelTest that demonstrates this bug. I'll post a stopgap patch with a workaround in a separate comment.

Here is the patch.

Attaching both the patch and the version with the test only to trigger the testbot on the test only version, to demonstrate the failing condition.

Hm, why don't we just implement __clone() on the BaseFieldDefinition object and possibly other related definition objects and make sure that we actually do a proper deep clone? Then it would just work?

Thanks for the feedback @kristiaanvandeneynde and @Berdir ! Assigning to myself to work on an approach based on defining a __clone() method on the BaseFieldDefinition.

I've attached a new patch (and an interdiff) that addresses the bug by implementing a deep clone using BaseFieldDefinition::__clone().

I am seeing on my local environment that the deep clone is not cloning all of the way down the chain of object properties...assigning back to myself to fix...

Here's a slightly different approach to the deep clone.

Given that BaseFieldDefinition->itemDefinition->fieldDefinition is a recursive reference back to the BaseFieldDefinition object, we need a way to overwrite that reference in the FieldItemDataDefinition class without having to instantiate a new instance of that class, and potentially losing any property values (e.g., custom values set by alter hooks). This patch adds a FieldItemDataDefinition::setFieldDefinition() public method to overwrite the existing value of BaseFieldDefinition->itemDefinition->fieldDefinition.

+++ b/core/lib/Drupal/Core/Field/TypedData/FieldItemDataDefinition.php
@@ -83,4 +83,17 @@ public function getFieldDefinition() {
+   * Sets the field item's field definition.
+   *
+   * @param \Drupal\Core\Field\FieldDefinitionInterface
+   *
+   * @return static
+   *   The object itself for chaining.
+   */
+  public function setFieldDefinition($field_definition) {
+    $this->fieldDefinition = $field_definition;
+    return $this;

I see, this is tricky.

A bit weird to add this method without having it on an interface, but the get() above is the same (which btw is only used twice in search_api and nowhere else that I can find).

Can we add a note that this is only used internally by BaseFieldDefinition::__clone() and should not be used otherwise?

Also, misses description and variable name of the @param.

Here's an update to the patch with the changes to the comment. Let me know if this looks OK or not.

Not sure why this issue was moved back to 8.2.x, but that branch has no further development since Feb. 1. Since this includes API additions and internal breaks/disruptions, it should be targeted for 8.4.x. Thanks!

The patch on #20 does not apply to 8.4.x

+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -723,4 +723,19 @@ public function getConfig($bundle) {
+  /**
+   * Magic method: Implements a deep clone.
+   */
+  public function __clone() {
+    // Ensure the itemDefinition property is actually cloned
+    // by overwriting the original reference.
+    $this->itemDefinition = clone $this->itemDefinition;
+
+    // The itemDefinition (\Drupal\Core\Field\TypedData\FieldItemDataDefinition)
+    // has a property fieldDefinition, which is a recursive reference to the
+    // parent BaseFieldDefinition. Need to overwrite the reference to the old
+    // object with a reference to the cloned one.
+    $this->itemDefinition->setFieldDefinition($this);
+  }

Is there any reason this can't be

  /**
   * Magic method: Implements a deep clone.
   */
  public function __clone() {
    // Ensure the itemDefinition property is actually cloned
    // by overwriting the original reference.
    $this->itemDefinition = FieldItemDataDefinition::create($this);;
  }

That way the setter is not needed.

Reroll patch applying #23 suggestion.

Needs work, but no longer needs a reroll.

In answer to @alexpott's question in #23, if FieldItemDataDefinition::create($this) is used then $this->itemDefinition->definition['settings'] is not set (it is with clone $this->itemDefinition) so I have re-rolled #20.

I've just came across this bug. The problem is not only in the comment module but in the field API itself, so I am setting the component to the field system. I've added one additional test, which is independent of the comment module.

+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -755,4 +755,19 @@ public function setStorageRequired($required) {
+  public function __clone() {
...
+    $this->itemDefinition = clone $this->itemDefinition;

By cloning the item definition only in BaseFieldDefinition we are covering only the cloning of base field definitions objects, but we have to cover the cloning of the parent class as well. I've moved this piece of code to the parent class.

I've also created a new interface covering the two FieldItemDataDefinition methods - getFieldDefinition() and setFieldDefinition() and marked properly the setFieldDefinition() method as internal.

I am also uploading a test only patch.

By mistake I've uploaded a not entirely clean test only patch and left some part of the fix inside, therefore I am uploading the test only and the full patch again.

+++ b/core/modules/comment/tests/src/Kernel/CommentBundlesTest.php
@@ -0,0 +1,83 @@
+      // Verify that the target type remains correct
+      // in the deeply-nested object properties.

another nit: wraps too early here. (can be fixed on commit)

+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -838,4 +838,17 @@ public function setStorageRequired($required) {
+    $this->itemDefinition->setFieldDefinition($this);

Wondering if we should expliictly re-define the itemDefinition property in this class with the type it expectedly has here, or maybe even throw an exception if it doesn't match, to have a better error if someone did something weird?

This class also has a propertyDefinitions property. Not sure if we should also handle that here, but just unsetting that might make sense as it is basically just a static cache and the property definitions might also depend on the item definition.

Other than that this looks OK.

Perhaps a committer can fix the minor nitpick mentioned in #21?

I've just updated it in the new patch.

Wondering if we should expliictly re-define the itemDefinition property in this class with the type it expectedly has here, or maybe even throw an exception if it doesn't match, to have a better error if someone did something weird?

What do you mean by "something weird"? :)

This class also has a propertyDefinitions property. Not sure if we should also handle that here, but just unsetting that might make sense as it is basically just a static cache and the property definitions might also depend on the item definition.

Although I don't think that this is dangerous I think we should take care of this as well. As we are defining proper cloning of the class BaseFieldDefinition I think it is justified to cover it in this issue, therefore I've included it in the new patch.

What I meant is that someone could in theory have a subclass of BaseFieldDefinition that has an itemDefinition property that is *not* implementing the new interface. I think that's highly unlikely and protected properties are AFAIK also kind of excluded from BC, not sure.

PhpStorm currently also complains that setFieldDefinition() does not exist in DataDefinitionInterface when you look at the __clone() method.

That is why we coul do this:

  /**
   * The data definition of the base field
   *
   * @var \Drupal\Core\Field\TypedData\FieldItemDataDefinitionInterface
   */
  protected $itemDefinition;

To at least document that we expect the item definition to be an instance of that. Or be even more strict and throw an exception if it is not, instead of letting it fatal.

I think the exception is probably overkill for a protected property but adding the property would be kind of useful?

Hmm I am not really sure about this. I mean I like the idea with the typyhinting, but if we do this here then others might argue that we have to do it at each place where we extend from a class and expect different interfaces for properties or return values e.g. following this logic we have to redefine the method EntityStorageInterface::load() in the ContentEntityStorageInterface so that it returns a value of the type ContentEntityInterface.

Committed 4f24c91 and pushed to 8.5.x. Thanks!

Not at all sure about an 8.4.x backport given the new interface etc., if this is blocking a contrib module we might have to make the new interface @internal or something. Moving straight to fixed but re-open if you really think this is needed.

This also affects Commerce, I'd love a core fix instead of having to copy Group's workaround.