Test ERR default values and configuration dependencies

Please don't mix unrelated code cleanups with functional fixes and adding tests.
It's really hard to review code changes to then realise they have no impact.. and finally finding the relevant pieces in all the changes.

The functional changes look pretty fine.

+++ b/src/Tests/EntityReferenceRevisionsCompositeTest.php
@@ -44,38 +44,47 @@ class EntityReferenceRevisionsCompositeTest extends WebTestBase {
    */
   public function testEntityReferenceRevisionsCompositeRelationship() {
 
+    // Create the test composite entity.
+    $composite = EntityTestCompositeRelationship::create([
+      'uuid' => '2d04c2b4-9c3d-4fa6-869e-ecb6fa5c9410',
+      'name' => $this->randomMachineName(),
+    ]);
+    $composite->save();
+
     // Create the reference to the composite entity test.
-    $field_storage = FieldStorageConfig::create(array(

This is not related to composite. In fact, having a default value with composite doesn't really work.

Lets make a new test instead.

Yes, having the test separate is much better to review.

Not sure if we shouldn't put it in a separate test class, possibly also a kernel test since we don't need a UI.

1. +++ b/src/Tests/EntityReferenceRevisionsAdminTest.php
   @@ -109,4 +112,62 @@ class EntityReferenceRevisionsAdminTest extends WebTestBase {
   +    // Create a node with a reference to the test target node.
   +    $node2 = $this->createNode([
   +      'title' => $this->randomMachineName(),
   +      'type' => 'article',
   +      'target_node_reference' => $node1,
   +    ]);
   

   To test that the default value works, you want to create the node without explicitly specifying the node.

2. +++ b/src/Tests/EntityReferenceRevisionsAdminTest.php
   @@ -109,4 +112,62 @@ class EntityReferenceRevisionsAdminTest extends WebTestBase {
   +
   +    // Test for loadEntityByUuid() in EntityReferenceRevisionsItem::calculateDependencies().
   +    $node1_after = Node::load($node1->id())->toArray();
   +    $this->assertEqual($node1_after['uuid'][0]['value'], '2d04c2b4-9c3d-4fa6-869e-ecb6fa5c9410');
   +
   

   This is not checking anything, just your own code above. It doesn't call that function.

   Instead, you simply want to check that $node2->target_node_reference->entity has the same id and revision id as $node 1.

   Additionally, after setting the default value, save $node1 with a new revision_id, keep the old one. The default value should reference the first version, not the new one.

   Second, instead of $node1 and $node2, use $node_target and $node_host or something like that.

3. +++ b/src/Tests/EntityReferenceRevisionsAdminTest.php
   @@ -109,4 +112,62 @@ class EntityReferenceRevisionsAdminTest extends WebTestBase {
   +    // Check if the ERR default values are properly created.
   +    $default_reference = $field->getDefaultValue($node2);
   +    $this->assertEqual($default_reference[0]['target_id'], $node1->id());
   +    $this->assertEqual($default_reference[0]['revision_id'], $node1->getRevisionId());
   

   Then this isn't needed.

4. +++ b/src/Tests/EntityReferenceRevisionsAdminTest.php
   @@ -109,4 +112,62 @@ class EntityReferenceRevisionsAdminTest extends WebTestBase {
   +    // Check if the configuration dependencies are properly created.
   +    $dependencies = $field->calculateDependencies()->getDependencies();
   +    $this->assertEqual($dependencies['config'][0], 'field.storage.node.target_node_reference');
   +    $this->assertEqual($dependencies['config'][1], 'node.type.article');
   +    $this->assertEqual($dependencies['module'][0], 'entity_reference_revisions');
   

   This is the part that actually checks the default value dependencies.

   This is where you need to check for the UUID of the content entity, if it's missing, then it doesn't work yet.

Ok, took me quite a bit of debugging too but the problem was actually trivial. It's target_revision_id, not just revision_id.

As written above, I'd prefer to have this test method in \Drupal\Tests\entity_reference_revisions\Kernel\EntityReferenceRevisionsSaveTest, remove all the UI and login stuff.

1. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
   @@ -23,6 +23,8 @@ use Drupal\entity_reference_revisions\EntityNeedsSaveInterface;
     *   allowed bundles.
   + * - target_id: The entity ID to reference to, used as default value.
   + * - revision_id: The revision ID.
     *
     * @FieldType(
   

   Those are not settings, they are default value (and also wrong, now).

   => remove.

2. +++ b/tests/src/Kernel/EntityReferenceRevisionsSaveTest.php
   @@ -184,4 +184,75 @@ class EntityReferenceRevisionsSaveTest extends KernelTestBase {
   +    $field->save();
   +
   +    // Add reference values to field config that will be used as default value.
   +    $default_value = [
   +      [
   

   you shouldn't have to save the field twice, remove the first call.

3. +++ b/tests/src/Kernel/EntityReferenceRevisionsSaveTest.php
   @@ -184,4 +184,75 @@ class EntityReferenceRevisionsSaveTest extends KernelTestBase {
   +    // Set new revision_id to the test target node.
   

   We don't really set: // Resave the target node so that the default revision is not he one we want to use.

4. +++ b/tests/src/Kernel/EntityReferenceRevisionsSaveTest.php
   @@ -184,4 +184,75 @@ class EntityReferenceRevisionsSaveTest extends KernelTestBase {
   +    // Check if the ERR default values are properly created.
   +    $this->assertTrue($node_target_after->getRevisionId() != $revision_id);
   

   If you do want to assert this then move it up and do it after resaving the node.

5. +++ b/tests/src/Kernel/EntityReferenceRevisionsSaveTest.php
   @@ -184,4 +184,75 @@ class EntityReferenceRevisionsSaveTest extends KernelTestBase {
   +    $node_target_after = $node_target_after->toArray();
   +    $node_host_after = Node::load($node_host->id())->toArray();
   +    $this->assertEquals($node_target_after['uuid'][0]['value'], '2d04c2b4-9c3d-4fa6-869e-ecb6fa5c9410');
   +    $this->assertEquals($node_host_after['target_node_reference'][0]['target_id'], $node_target->id());
   +    $this->assertEquals($node_host_after['target_node_reference'][0]['target_revision_id'], $revision_id);
   

   It's way easier to access those values on the node object instead of using toArray(). Just write $node_host_after->target_node_reference->target_id.

   The uuid check is also unnecessary and unrelated.

6. +++ b/tests/src/Kernel/EntityReferenceRevisionsSaveTest.php
   @@ -184,4 +184,75 @@ class EntityReferenceRevisionsSaveTest extends KernelTestBase {
   +    // Check if the configuration dependencies are properly created.
   +    $dependencies = $field->calculateDependencies()->getDependencies();
   +    $this->assertEquals($dependencies['config'][0], 'field.storage.node.target_node_reference');
   +    $this->assertEquals($dependencies['config'][1], 'node.type.article');
   +    $this->assertEquals($dependencies['module'][0], 'entity_reference_revisions');
   

   This missing the actually relevant part here. The content dependency on the uuid of $node_target.

It has to be in $dependencies. If it is not there, then it's not working yet and you need to debug calculateDependencies().

This means we're not testing that the target_uuid is actually stored properly when configuring this in the UI. Not sure if that's worth doing.

I'm also not sure if we need our custom code for this. Is the calculateDependencies() method different from the parent class? Maybe we can just remove it.

Without a revision UUID, it's pretty bogus anyway.

I think you misunderstood me.

target_uuid isn't going to magically work. We do need it.

The only question I have is if it is actually set if you select a default in the UI. I think we use standard widgets and extend from core, so it *should* work. Not sure if it's worth to add a test for this. Looking at \Drupal\entity_reference_revisions\Tests\EntityReferenceRevisionsAdminTest::testEntityReferenceRevisions, it might just work if you create the node before the field and then put that node into the $field_edit argument of static::fieldUIAddNewField(). Then you can check the UI part there and also make sure that adding it actually adds the target_uuid to the field default value.

What I mean with calculateDependencies() is that as far as I see, it's pretty much a duplicate of the parent implementation. I would recommend you just remove, and if the test still passes, then I think we can just move forward with the core implementation.

Small misunderstanding, I didn't mean to remove the uuid from the kernel test, we still want to have that there. Just re-add the removed/commented code from before.

Now we're good to go I think.

Committed.