Just like entity field in ER

    $properties['field_collection_item'] = DataReferenceDefinition::create('entity')
      ->setLabel(t('Field collection item'))
      ->setDescription(t('The field collection item'))
      // The field collection object is computed out of the value.
      ->setComputed(TRUE)
      ->setReadOnly(FALSE)
      ->setTargetDefinition(EntityDataDefinition::create('field_collection_item'))
      ->addConstraint('EntityType', 'field_collection');

After that we can remove the methods like getFieldCollectionItem and calculate the value in setValue ofr getValue methods.

In D7 it make sense not to make a ER field but in D8 ER is in core so I think we can leverage that.

CommentFileSizeAuthor
#43 interdiff-41-43.txt6.08 KBjmuzz
#43 field_collection-extend_entity_reference-2509254-43.patch215.74 KBjmuzz
#41 field_collection-extend_entity_reference-2509254-41.patch219.21 KBjmuzz
#40 field_collection-extend_entity_reference-2509254-40.patch218.97 KBjmuzz
#37 interdiff-35-37.txt948 bytesjmuzz
#37 field_collection-extend_entity_reference-2509254-37.patch219.36 KBjmuzz
#35 interdiff-33-35.patch328 bytesjmuzz
#35 field_collection-extend_entity_reference-2509254-35.patch218.78 KBjmuzz
#33 interdiff-32-33.txt189.23 KBjmuzz
#33 field_collection-extend_entity_reference-2509254-33.patch218.83 KBjmuzz
#32 field_collection-extend_entity_reference-2509254-32.patch32.81 KBjmuzz
#29 interdiff-25-29.txt6.53 KBjmuzz
#29 field_collection-extend_entity_reference-2509254-29.patch32.66 KBjmuzz
#25 interdiff-22-25.txt3.29 KBjmuzz
#25 field_collection-extend_entity_reference-2509254-25.patch32.31 KBjmuzz
#22 interdiff-20-22.txt768 bytesjmuzz
#22 field_collection-extend_entity_reference-2509254-22.patch28.72 KBjmuzz
#20 interdiff-15-20.txt3.88 KBjmuzz
#20 field_collection-extend_entity_reference-2509254-20.patch27.88 KBjmuzz
#17 field_collection-extend_entity_reference-2509254-15.patch27.15 KBjmuzz
#16 interdiff-13-15.txt1.82 KBjmuzz
#13 field_collection-extend_entity_reference-2509254-13.patch26.33 KBjmuzz
#11 field_collection-extend_entity_reference-2509254-11.patch23.52 KBjmuzz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Title: Convert field collection field type value property to entity reference » Add field collection entity reference property to field collection field type
Category: Feature request » Task
Issue summary: View changes

I think we should keep the original property as it is and add a new computed field.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

So instead of getFieldCollectionItem() we'd get an entity reference and then use getValue() on that. I'm not sure I see the advantage of that. Field collection items are connected to their host via a reference to their ID, but that seems like an implementation detail to me since field collection items behave like data in their host instead of an independent entity being referenced.

Could you explain a bit more about what entity reference provides that field collection would benefit from?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Project: Field collection d8 port » Field collection
Version: » 8.x-1.x-dev

Moving project.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Status: Active » Postponed (maintainer needs more info)
jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Version: 8.x-1.x-dev » 8.x-2.x-dev

This is the approach that the people involved in 8.x-2.x are attempting to implement.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

It might be worth doing to gain access to the entity reference formatters. That would solve #2761505: Allow to choose a display mode in the "fc items" formatter settings

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

We'd be able to make use of entity reference REST functionality too. #2614292: Add normalizers for Field Collection Items (to enable REST/JSON API support)

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

Another feature of entity reference that would benefit people using field collection: #1131872-15: Fieldcollection items that have a taxonomy field assigned and a term given are not shown on the corresponding taxonomy page

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Version: 8.x-2.x-dev » 8.x-1.x-dev
Status: Postponed (maintainer needs more info) » Active

I think it would be worth doing sooner rather than later if possible.

Making field collection items an extension of entity references makes more sense to me than having field collection items hold an entity reference.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Title: Add field collection entity reference property to field collection field type » Field collection fields should extend entity reference fields
jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-11.patch23.52 KB

I've gotten a start on this. I've done some minimal manual testing and it seems the basic features are working. It doesn't currently enjoy any of the theoretical benefits of using this approach. The tests are dying and I'm running into some difficulties getting information from simpletest. I'm going to make another issue for that.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

Created an issue about difficulties getting a meaningful error message: https://www.drupal.org/node/2764783

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-13.patch26.33 KB

Tests are passing now. I don't want to commit it until it is making use of at least one of the benefits of this change.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

There are some unnecessary options on the field settings form from entity_reference that will need to be removed or hidden.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

It's repeating an entity reference behaviour where it adds different options for entity types to reference to the field creation form, so for example "Taxonomy term" can be selected under "General" field types and it will be created as a field collection field that references taxonomy terms instead of field collection items.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
interdiff-13-15.txt1.82 KB

The field collection items formatter now extends the one from entity reference. It allows the user to choose a display mode for the field collection items, which is one thing field collection 8.x-1.x couldn't do before.

This patch should include an update since the name of one of the field properties has changed from 'value' to 'target_id'.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-15.patch27.15 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Active » Needs review

Patch looks grreat. This is a big change and will break existing sites.

+++ b/src/Tests/FieldCollectionRESTTest.php
@@ -57,6 +58,7 @@ class FieldCollectionRESTTest extends RESTTestBase {
+    _d($field_collection_data);

debug code?

Status: Needs review » Needs work

The last submitted patch, 17: field_collection-extend_entity_reference-2509254-15.patch, failed testing.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Status: Needs work » Needs review
FileSize
field_collection-extend_entity_reference-2509254-20.patch27.88 KB
interdiff-15-20.txt3.88 KB

Yes it was.

I fixed the interface problems I mentioned.

The update still needs to be written.

Status: Needs review » Needs work

The last submitted patch, 20: field_collection-extend_entity_reference-2509254-20.patch, failed testing.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Status: Needs work » Needs review
FileSize
field_collection-extend_entity_reference-2509254-22.patch28.72 KB
interdiff-20-22.txt768 bytes

Forgot a file.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

I thought such an update would only have to change some column names or something but it turns out to be a huge can of worms. There is a longstanding issue about supporting such changes: https://www.drupal.org/node/937442

There is some field settings that could be updated, and berdir has linked from here to a solution which involves manually updating field schema. https://www.drupal.org/node/2554097

I'm thinking it won't be necessary to write an update for this because it is just an alpha version still and tedbow has said that there aren't many installs of it yet. https://www.drupal.org/node/2628388#comment-11204273

jibran’s picture

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

a huge can of worms

Tell me about it. In #2555027: Support non-numeric entity ID's the only change was 

-    $properties['target_id'] = DataReferenceTargetDefinition::create('integer')
+    $properties['target_id'] = DataReferenceTargetDefinition::create('string')

and have look at dynamic_entity_reference_update_8001 how massive it is.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-25.patch32.31 KB
interdiff-22-25.txt3.29 KB

I did have a look at it and it's a good example. With this and the one @berdir posted I was able to put together an update. I've only tested it with one fairly simple dataset.

jibran’s picture

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

Update function looks good. I'll request chx to have a quick look at it.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Needs work

Here are some review points:

  1. +++ b/field_collection.install
@@ -0,0 +1,83 @@
+  $column_schema = array('type' => 'int',
+                         'unsigned' => TRUE,
+                         'description' => 'The ID of the field collection item.',
+                         'not null' => TRUE,);

    Can we please use short array syntax in this patch? Also the indentation is wrong. 

  2. +++ b/field_collection.install
@@ -0,0 +1,83 @@
+  foreach ($config->listAll('field.field.') as $field_id) {

    This is wrong you are not changing any config instead of this you should use $entity_field_manager->getFieldMapByFieldType() see #2555027: Support non-numeric entity ID's for reference. 

  3. +++ b/field_collection.install
@@ -0,0 +1,83 @@
+      $column_name = $field->get('field_name') . '_target_id';
...
+      $tables = array(
+        $field->get('entity_type') . '__' . $field->get('field_name'),
+        $field->get('entity_type') . '_revision__' . $field->get('field_name'));

    Use the table mapping to get the table name and column name please have a look at #2555027: Support non-numeric entity ID's for reference. 

  4. +++ b/field_collection.install
@@ -0,0 +1,83 @@
+          $field->get('field_name') . '_value',
...
+            $field->get('field_name') . '_revision_id' => array(

    Use a local variable ;-) 

  5. +++ b/field_collection.install
@@ -0,0 +1,83 @@
+        $key = $field->get('entity_type') . '.field_schema_data.' . $field->get('field_name');

    The getter calls can be avoided by using field mapping. 

  6. +++ b/field_collection.install
@@ -0,0 +1,83 @@
+        $spec = array(
+          'fields' => array(
+            $column_name => array(

    short array syntax please. 

  7. +++ b/src/Entity/FieldCollectionItem.php
@@ -329,7 +330,7 @@ class FieldCollectionItem extends ContentEntityBase implements FieldCollectionIt
+          $entity->{$this->bundle()}[] = array('entity' => $this);

    Can we please change it to $bundle = $this->bundle(); $entity->$bundle->appendItem(['entity' => $this]);? 

  8. +++ b/src/FieldCollectionItemList.php
@@ -0,0 +1,23 @@
+    return array();

    Short syntax please. :-)

  9. This needs an upgrade path test have a look at #2555027-49: Support non-numeric entity ID's for taking a quick DB dump.
jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

Thanks for the feedback.

It lead to me taking a look at the policy discussion about array syntax and creating #2769205: Switch to short array syntax.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-29.patch32.66 KB
interdiff-25-29.txt6.53 KB

This addresses 1 to 8.

I'm leaving it needs work because there is no upgrade path test yet.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Needs review

Interdiff looks great. NR for test bot.

+++ b/field_collection.install
@@ -0,0 +1,91 @@
+        str_replace('__', '_revision__', $table_mapping->getFieldTableName($field_name)),
...
+          "{$field_name}_value",
...
+            "{$field_name}_revision_id" => [

You can't make sure the name of the table or column like this. We do shorten these strings if these are bigger then the the allowed limit. Unfortunately, we have to use the api here.

Status: Needs review » Needs work

The last submitted patch, 29: field_collection-extend_entity_reference-2509254-29.patch, failed testing.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Status: Needs work » Needs review
FileSize
field_collection-extend_entity_reference-2509254-32.patch32.81 KB

Minor reroll.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-33.patch218.83 KB
interdiff-32-33.txt189.23 KB

I addressed the review points and added a test with some additions to get it passing.

I still want to add some asserts to the test.

Status: Needs review » Needs work

The last submitted patch, 33: field_collection-extend_entity_reference-2509254-33.patch, failed testing.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Status: Needs work » Needs review
FileSize
field_collection-extend_entity_reference-2509254-35.patch218.78 KB
interdiff-33-35.patch328 bytes

Status: Needs review » Needs work

The last submitted patch, 35: interdiff-33-35.patch, failed testing.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Status: Needs work » Needs review
FileSize
field_collection-extend_entity_reference-2509254-37.patch219.36 KB
interdiff-35-37.txt948 bytes

The update test class verifies all the schema changes so I guess there isn't much left to test for. I added some checks for the field values.

jibran’s picture

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

Great to see the green patch with update test. Very nice work @jmuzz.

+++ b/field_collection.install
@@ -0,0 +1,159 @@
+function field_collection_update_8001() {

I think this one update hook is doing way too much work. We should split it into 3 update hooks.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

It is a large update, but it all has to be done for it to reach the next working state. I don't think I can support or make tests for if only part of it is done.

When the API supports changes like this properly then updates like this won't need to do so much work.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-40.patch218.97 KB

Reroll for short array syntax.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-41.patch219.21 KB

Reroll that preserves the support for displaying subfields in twig from #2628648: Allow access to subfields from twig.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented 
+++ b/src/Entity/FieldCollectionItem.php
@@ -112,7 +112,8 @@ class FieldCollectionItem extends ContentEntityBase implements FieldCollectionIt
-      ->setReadOnly(TRUE);
+      ->setReadOnly(TRUE)
+      ->setSetting('unsigned', TRUE);

Thank you I do understand your point but all I'm saying that we should move this change to follow up and add another update hook for this.
This is an isolated change from target_id.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
FileSize
field_collection-extend_entity_reference-2509254-43.patch215.74 KB
interdiff-41-43.txt6.08 KB

I suppose that is an unrelated change.

I removed that and the new description for revision_id in the schema so the revision related columns won't need to be updated.

jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented

I created #2772379: Revision ID should be unsigned and have a description for the change to revision IDs.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Perfect! Thank you for moving it to new issue. We have update path we have update path tests so this is ready. We could test it on other databases drivers but I don't think that should block this issue.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented

I agree the change makes sense - not only does it give more compatibility with things like widgets or formatters, it also improves DX as the field collection field does not use special column names any more.
For widget/formatter compatibility it's a bit sad, that core's compatibility handling does not cover extended plugins, but we could easily solve that with some drupal_alter() in a follow-up.

Code looks pretty a decent and even has upgrade path test coverage! Awesome!

  • jmuzz committed dba4052 on 8.x-3.x 
    Issue #2509254 by jmuzz, jibran: Field collection fields should extend...
jmuzz’s picture

jmuzz CreditAttribution: jmuzz commented
Status: Reviewed & tested by the community » Fixed

Thanks again for all the valuable feedback!

Status: Fixed » Closed (fixed)

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