Maintain composite relationship

Overriding PostSave function to assign id and parent type on creation.

Tests fixed, it was a trivial fix, after commit this we can continue on paragraphs module.

No, a save is needed. But it's the $entity that has to be set and saved, not $this.

Changes made. Should be Ok now.

As discussed, lets add some test coverage to make sure that this actually works.

Tests added. A lot of changes. Should work now.

1. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
   @@ -270,6 +270,23 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
   +  public function postSave($update) {
   +    parent::postSave($update);
   +    if ($entity = $this->entity) {
   +      if ($entity->getEntityType()->hasKey('parent_type') && $entity->getEntityType()->hasKey('parent_id')) {
   +        $parent_entity = $this->getEntity();
   +        if ($entity->get($entity->getEntityType()->getKey('parent_type'))->value != $parent_entity->getEntityTypeId() || $entity->get($entity->getEntityType()->getKey('parent_id'))->value != $parent_entity->id()) {
   +          $entity->set($entity->getEntityType()->getKey('parent_type'), $parent_entity->getEntityTypeId());
   +          $entity->set($entity->getEntityType()->getKey('parent_id'), $parent_entity->id());
   +          $entity->save();
   +        }
   +      }
   +    }
   

   This needs comments to explain what we are doing here. Explain the concept and the different checks and why they are necessary.

   Might also be easier to read if you have a $parent_type_key variable, then those lines will get a lot shorter.

   Given that this is about revisionable entities, we might want to ensure that no new revision is saved when updating this.

2. +++ b/src/Tests/EntityReferenceRevisionsAdminTest.php
   @@ -82,7 +82,7 @@ class EntityReferenceRevisionsAdminTest extends WebTestBase {
        $edit = array(
          'title[0][value]' => 'Entity reference revision content',
   -      'field_entity_reference_revisions[0][target_id]' => $node->label() . '(' . $node->id() . ')',
   +      'field_entity_reference_revisions[0][target_id]' => $node->label() . ' (' . $node->id() . ')',
        );
   

   this seems unrelated, same for the one below?

3. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,102 @@
   + *
   + * entity_reference_revisions configuration test functions.
   + *
   

   remove this

4. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,102 @@
   +
   +  use FieldUiTestTrait;
   

   we no longer need this.

5. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,102 @@
   +    // Place the breadcrumb, tested in fieldUIAddNewField().
   +    $this->drupalPlaceBlock('system_breadcrumb_block');
   

   same.

6. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,102 @@
   +  /**
   +   * Test for maintaining composite relationship.
   +   *
   +   * Tests that the referenced entity saves the entity_type and entity_id of the
   +   * parent when saving it.
   +   */
   

   first part repeats the class documentation. maybe we can make the more specific second line a bit shorter so it fits on a single line and can be the only documentation here?

7. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,102 @@
   +      'field_name' => 'entity_test_composite',
   

   lets pick a separate name for he field as the entity_type, this will be easier to understand then. composite_reference, maybe?

8. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,102 @@
   +    // Verify the value of parent type and id before create a node. Should be
   +    // set as the default value of the entity fields.
   +    $composite_before = EntityTestCompositeRelationship::load($composite->id())->toArray();
   +    $this->assertEqual($composite_before['parent_type'][0]['value'], 'unknown', 'parent_type is unknown');
   +    $this->assertEqual($composite_before['parent_id'][0]['value'], 'unknown', 'parent_id is unknown');
   

   I don't think we need to test this. I still want to find a way to not make them required and NULL by default.

9. +++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
   @@ -0,0 +1,102 @@
   +    $composite_after = EntityTestCompositeRelationship::load($composite->id())->toArray();
   +    $this->assertEqual($composite_after['parent_type'][0]['value'], $node->getEntityTypeId(), 'parent_type is set as node');
   +    $this->assertEqual($composite_after['parent_id'][0]['value'], $node->id(), 'parent_id is set as 1');
   

   the description doesn't seem very useful. You don't need to repeat the value. I'd just leave it out.

Changes made based on comment #11 about the second point I made those changes to make the test pass. It seems something has changed because the previous test fails where caused because of that space change. (See https://www.drupal.org/pift-ci-job/164228)

+++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
@@ -45,21 +40,18 @@ class EntityReferenceRevisionsCompositeTest extends WebTestBase {
-      'field_name' => 'entity_test_composite',
+      'field_name' => 'composite_reference',

I have checked and this seems to cause the test fail. Should we keep it like entity_test_composite?

So sorry, I should have updated the reference everywhere. Should work now.

Discussed with @Berdir, the field parent_name has been added. Also fixed the variable names a bit and the comments.

1. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
   @@ -274,16 +274,19 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
   +      $entity_type = $entity->getEntityType();
   +      if ($entity_type->hasKey('parent_type') && $entity_type->hasKey('parent_id') && $entity_type->hasKey('parent_name')) {
   +        // If $entity has those keys get their value and get its parent entity.
   

   I think the key should be more specific, parent_field_name. Just name doesn't explain what it is.

   Also, parent_field_name should be optional. It should be possible for an entity type to only have parent_type and parent_id. so check parent_name additional, within.

2. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
   @@ -274,16 +274,19 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
   +        if ($entity->get($parent_type_key)->value != $parent_entity->getEntityTypeId() || $entity->get($parent_id_key)->value != $parent_entity->id() || $entity->get($parent_name_key)->value != $parent_entity->label()) {
   

   that will make this a bit complicated, since we do not want to trigger this if there was no change and there is no parent_field_name.

   You need a combined || $parent_name_key && .. the check) condition, which will make this very long. Maybe split out the different checks all into variables and then combine it ($parent_type_changed || $parent_id_changed || ...

Changes made, now it seems easier to understand.

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -272,21 +272,35 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+        if ($parent_type_changed || $parent_id_changed || $parent_field_name_changed) {
+          // Check if any of the keys has changed, do not create a new revision.
           $entity->setNewRevision(FALSE);
           $entity->save();

Oh. That's not exactly what I was thinking about but actually, I like this approach.

except, when you do it like this, a single variable is enough.. I'd use $needs_save = FALSE;

You also shouldn't set it to the return value of $entity->set, you should explicitly/separately set it to TRUE in the if.

That is totally right, it should be better in that way. Interdiff added.

1. You could do a combined if ($this->entity && parent_type && parent_id) and return early, that would save two nested if's, yes.
2. getEntity() is not a load. It is the entity the item is attached to. And this happens during saving, which absolutely requires an entity. So yes, I'd say you can rely on this.

1. Wouldnt make that the if too long?, actually I think it should stay like that especially because we get the entity_type after get the entity.

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -270,6 +270,48 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+    if ($entity = $this->entity) {
+      // Check if $this has an entity.
+      $entity_type = $entity->getEntityType();
+      if ($entity_type->hasKey('parent_type') && $entity_type->hasKey('parent_id')) {
+        // If $entity has those keys get its parent entity.

Otherwise it would be something like

+    if ($this->entity && $this->entity->getEntityType()->hasKey('parent_type') && $this->entity->getEntityType()->hasKey('parent_id')) {

I think we can live with that. The idea is that you would make it a negative check.

if (!$this->entity || !$this->entity->getEntityType()->hasKey('parent_type') || !$this->entity->getEntityType()->hasKey('parent_id'))  {
  return;
}
// Do stuff here

That should make it easier to read the code as you have two nested if's less.

Also, I like using single empty lines between different parts of the code, that helps to visually group them.

Added two different patches, the first one is based on comment #25 and the second is not using entity_keys anymore (no-entity-keys.patch), both work in the same way. With these patches we can decide if we will use entity keys and set a default value to them or not using entity keys at all.

How the comments in the patch refer to the field names now could possibly be slightly improved now. I'd suggest to use "parent type" and refer to it as a concept and not a key or variable name (instead of parent_type, or parent_type_key and similar things that are used right now. For example "If parent_field_name_key has changed then set it.", we don't *set* the key, the key just tells us the field name for which we want to set the value. If you write "parent field name" instead, then that makes perfect sense.

Miro can probably do this on commit.

Otherwise this looks good to me.

Patch added based on comments above, continuing with the non entity keys branch. Should work now.

Changes made. :)