I have this on an entity

function baseFieldDefinitions() {
    $fields['channels'] = BaseFieldDefinition::create('dynamic_entity_reference')
       ...
      ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED)
      ->setReadOnly(TRUE);
}

function foo() {
  $this->channels->appendItem($entity);
}

Using foo() will cause this fatal:

( ! ) Fatal error: Cannot use object of type Drupal\courier\Entity\Email as array in [...]/modules/dynamic_entity_reference/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php on line 340

DynamicEntityReferenceItem::setValue() expects an array, instead of allowing an entity. This is different behavior to EntityReferenceItem::setValue() (core ER), which does not have the code block that I have remove in the patch.

CommentFileSizeAuthor
#7 deritem_expects_an-2473931-7.patch2.81 KBjibran
#7 deritem_expects_an-2473931-7-test-only.patch1.54 KBjibran
der-set-entity-obj.patch855 bytesdpi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, der-set-entity-obj.patch, failed testing.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Nice catch. We just can't just remove that hunk it's there for a reason. :)
We do have tests for this DynamicEntityReferenceItemTest::testContentEntityReferenceItem()

    // Test value assignment via the computed 'entity' property.
    $entity->field_der->entity = $term;
    $this->assertEqual($entity->field_der->target_id, $term->id());
    $this->assertEqual($entity->field_der->entity->getName(), $term->getName());

    $entity->field_der = ['entity' => $term2];
    $this->assertEqual($entity->field_der->target_id, $term2->id());
    $this->assertEqual($entity->field_der->entity->getName(), $term2->getName());

I'd suggest to add tests for your case in this same method and then we can move forward from there.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi commented

We just can't just remove that hunk it's there for a reason. :)

If you're passing an entity, we dont need to check if it has entitytypeid. The patch was meant as a demonstration, the code block could be moved into the first else block.

I would love to update tests but a lot of them are failing locally, unlike testbot. Is there something I need to do?

  • jibran committed 7c6c05d on 8.x-1.x 
    Issue #2473931 by dpi, jibran: DERItem expects an array, should allow...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Category: Bug report » Support request
Status: Needs work » Fixed

I think you are calling it with wrong parameters.
Try 

$this->channels->appendItem(['entity' => $entity]);

DynamicEntityReferenceItem::setValue() always take an array either with 'entity' key or with 'target_id' and 'target_type' keys.
I have added a test for above case please see 7c6c05d.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Category: Support request » Bug report
Status: Fixed » Needs review
FileSize
deritem_expects_an-2473931-7-test-only.patch1.54 KB
deritem_expects_an-2473931-7.patch2.81 KB

After reading more code I think it's a valid bug.

The last submitted patch, 7: deritem_expects_an-2473931-7-test-only.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

looks good to me - I think the original reason was DER had less features than ER cause simplicity.
But that ship has sailed - we are near feature parity.

  • jibran committed 480dc6d on 8.x-1.x 
    Issue #2473931 by jibran, larowlan : Follow up: DERItem expects an array...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Reviewed & tested by the community » Fixed

Thanks for the review.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.