Problem/Motivation

On my way to DrupalCamp Karachi I was having a look at EntityReferenceFieldItemList for to find a fix for #2389605: DER's "autocreate" feature is mostly broken (and untested). I realized EntityReferenceFieldItemList::referencedEntities(), EntityReferenceFieldItemList::processDefaultValue() and EntityReferenceFieldItemList::defaultValuesFormSubmit() are specific to ER. We need DynamicEntityReferenceFieldItemList for DER. It will also fix #2365297: Can't set field default value which is a wrong fix anyway.

Proposed resolution

Create DynamicEntityReferenceFieldItemList for DER.

Remaining tasks

Complete the patch.
Add tests.
Review.

User interface changes

None

API changes

None

CommentFileSizeAuthor
#21 der-list.2.patch1.7 KBlarowlan
#18 der-list.patch1.69 KBlarowlan
#12 2405009-DER-FieldItemList-12.patch13.24 KBjibran
#12 interdiff.txt5.56 KBjibran
#11 2405009-DER-FieldItemList-11.patch7.68 KBjibran
#11 interdiff.txt822 bytesjibran
#10 2405009-DER-FieldItemList-10.patch7.6 KBjibran
#10 interdiff.txt3.05 KBjibran
#8 2405009-DER-FieldItemList-8.patch7.42 KBjibran
#8 interdiff.txt684 bytesjibran
#6 2405009-DER-FieldItemList-6.patch6.75 KBjibran
#6 interdiff.txt1.25 KBjibran
#4 2405009-DER-FieldItemList-3.patch5.5 KBjibran
#3 interdiff.txt791 bytesjibran
der-FieldItemList-wip.patch4.61 KBjibran

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/src/DynamicEntityReferenceFieldItemList.php
@@ -0,0 +1,130 @@
+      $target_type = $item->target_type;
+      $entities = \Drupal::entityManager()->getStorage($target_type)->loadMultiple($ids);

aaaah

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Don't you think we need tests for that? Should we create an interface for DERFieldItemList? How does Drupal know that now it has to user DERFieldItemList instead of ERFieldItemList?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new interdiff.txt791 bytes

How does Drupal know that now it has to user DERFieldItemList instead of ERFieldItemList?

+ * list_class = "\Drupal\dynamic_entity_reference\DynamicEntityReferenceFieldItemList",

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new 2405009-DER-FieldItemList-3.patch5.5 KB

Now with patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2405009-DER-FieldItemList-3.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.25 KB
new 2405009-DER-FieldItemList-6.patch6.75 KB

I think we can remove the hunk introduced in #2365297: Can't set field default value.

Status: Needs review » Needs work

The last submitted patch, 6: 2405009-DER-FieldItemList-6.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt684 bytes
new 2405009-DER-FieldItemList-8.patch7.42 KB

This will fix the test.

Status: Needs review » Needs work

The last submitted patch, 8: 2405009-DER-FieldItemList-8.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.05 KB
new 2405009-DER-FieldItemList-10.patch7.6 KB

With a proper fix.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new interdiff.txt822 bytes
new 2405009-DER-FieldItemList-11.patch7.68 KB
  1. We need tests for DynamicEntityReferenceFieldItemList::referencedEntities() see #2073661: Add a EntityReferenceField::referencedEntities() method for tests.
  2. We are using ValidReferenceConstraint in DynamicEntityReferenceItem which uses ValidReferenceConstraintValidator and ValidReferenceConstraintValidator::validate is specific to ER field.
  3. Do we depend on ReferenceAccessConstraintValidator if yes then it is also specific to ER.
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: -Need tests
StatusFileSize
new interdiff.txt5.56 KB
new 2405009-DER-FieldItemList-12.patch13.24 KB

Added tests for point 1. I think we can fix this issue. Going to move 2 and 3 to new issue I have a failing test for 2.

larowlan’s picture

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

Thanks @jibran great job

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Created #2405607: Fix DER constraint validation for #11.2 and #11.3

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Looks good to me as well.

jibran’s picture

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

Thanks.

  • jibran committed c0eea4a on 8.x-1.x 
    Issue #2405009 by jibran: DER needs its own FieldItemList class
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Fixed » Needs review
StatusFileSize
new der-list.patch1.69 KB 
+++ b/src/DynamicEntityReferenceFieldItemList.php
@@ -0,0 +1,132 @@
+    if ($ids) {
+      $target_type = $item->target_type;

$item is out of scope here

Status: Needs review » Needs work

The last submitted patch, 18: der-list.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
+++ b/src/DynamicEntityReferenceFieldItemList.php
@@ -29,23 +29,25 @@ class DynamicEntityReferenceFieldItemList extends EntityReferenceFieldItemList {
+      foreach ($ids as $target_type => $entity_type_ids) {
...
+        foreach ($ids as $delta => $target_id) {

This wrong. We can't use id's again.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
StatusFileSize
new der-list.2.patch1.7 KB

whoops

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Cool

larowlan’s picture

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

  • larowlan committed a14a4c1 on 8.x-1.x 
    Issue #2405009 by jibran, larowlan: DER needs its own FieldItemList...

Status: Fixed » Closed (fixed)

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