Consider Dynamic Entity Reference for storage needs

Now that entityreference is in core the future statement on module page is mostly invalidated and it is good for Relation's future to play nicely with other modules. We definitely should replace endpoints with that.

I just deleted relation_endpoint module and added dynamic_entity_reference to replace that on branch 8.x-1.x-der. There is probably going to be challenges with directionality. So far r_index = 0 has meant that entity is source for all others (1:n) but with delta ordering this could probably be solved with 2 instances for directional relations (n:m). What drawbacks would there be for such solution? We wouldn't need our own config for source & target entity bundles. Querying related entities would be harder if you want to ignore directionality.

I was thinking about the widget and entity bundle config that comes with dynamic entity reference. There you can reorder items and all items share the same entity limitations. Both of these are problematic for directional relations as we want to support different bundle settings for source and target entities.

hmm. one option could be to create a new edit widget for directional relations. Built in widget works just fine for non-directional relations with added extra validation for min arity and uniqueness of endpoints.

Yeah it's probably better to break the first out. Reordering is a cool feature.

I've added a new add relation UI to make widget testing a bit easier

Thanks @chx for creating the issue. @mikran please have a look at DER issue queue and let me and @larowlan now if we can be any help. Feel free to create feature requests there. Can you please post some roadmap or updates of your work so that I can keep up with it? Thank you for using DER.

Okay so delta is used to replace r_index in storage, but, delta can not be used as condition in queries, right?

From now on the DER branch is built on top of the latest patch from #2384459: Add entity query condition for delta in EFQ as it's clear that this project can't continue without that.

Patches that are not directly related to DER should still be rolled against 8.x-1.x branch first and I'll then merge them to 8.x-1.x-der so both branches are somewhat usable. (and once it's 100% sure that DER is the way to go that branch will be merged to 8.x-1.x).

#2407587: Allow multiple target entity types in the entity reference field

That most likely means that we don't need Dynamic Entity Reference as dependency and that definitely means that this is the way forward and I'll soon merge this branch to main 8.x-1.x branch for easier development.

Looks uncertain for now, since #2407505: [meta] Finalize the menu links (and other user-entered paths) system is going in a different direction.

Do we have a problem with depending on dynamic_entity_reference?

Do we have a problem with depending on dynamic_entity_reference?

No we don't.

DER version works better already so maybe it's waste of time to keep 2 branches active?

And a patch.

(Forgive me if my post is poorly informed. I just started looking at the relation module.)

Why not use regular non-dynamic entity reference?
As I see it, there are two relevant cases:
1. Possibly symmetric, possibly transitive, relation, within the same entity type.
2. Directional relation, which can have distinct source and target entity type.

For 1., a multiple-value entityreference field can be used.
For 2., two single-value entityreference fields can be used.

Maybe there is an exotic 3rd case, where a symmetric and/or transitive relation exists across entity types. But this seems rather rare.
Maybe also a 4th case, directional relation with more than 2 end points.
In these special cases we could use dynamic entity reference, or just additional entity reference fields.

If the different cases are treated separately, simple entityreference will be sufficient most of the time.
See also #2684867: Equivalent to custom entity type + 2x entity reference?

In fact, for the most simple case of directional relation from one entity type to another, it would be sufficient to just have 2 integer columns in the main relation table.
Of course this would mean that this table cannot be reused for other non-symmetric relation types. And thus, there would have to be different relation entity types, for different types of relations. Maybe not desirable.

entity reference hardwires the target entity type as a setting. relation doesn't. End.

But in most real-world cases, hard-wiring the source and target type is the intended behavior.
This would allow to use entityreference for these cases, and only use dynamic entity reference for cases that need to have a dynamic source and target type.

Obviously, if the same approach should be used across all relations, then it has to be dynamic entity reference.

Both #2384459: Add entity query condition for delta in EFQ and code in this module changed a lot. This patch definitely needs a reroll.

That patch is quite a bit behind the 8.x-1.x-der branch so here is the latest.

Here's a re-rolled patch based on all the latest code :-)

Building on the last patch (comment 26), this patch also fixes the following:

* Show the 'add relation' action on the relation list page correctly
* Query target columns of the DER field table rather than those that were for relation_endpoint
* Fix links & redirects for the add relation page
* Fixed error on creation of relations from add relation page.
* Configured DER instance appropriately for a relation, given the relation's allowed bundles settings (this makes use of #2405467-22: Add EntityType and Bundle constraint on entity property of DynamicEntityReferenceItem, which ensures validation actually happens and gives the user feedback)

Unfortunately the item in my last point in my previous comment does not work correctly, and I've run out of time to work on this. I hadn't realised that the allowed entity types are *only* set on the FieldStorage, whereas only the allowed *bundles* are set on the field instance - which then also meant relation+DER weren't appropriate for my use case either so I've had to move on.

Here's the 'fix' for the error that would otherwise occur (with interdiff), and TODOs in code for the outstanding problems that I had intended to work on, so someone else should at least be able to pick this up now. I would suggest at least updating the 8.x-1.x-der branch with this perhaps, so that it is still more usable for others to make progress with?

patches 26, 27 and 28 committed to der branch, thanks!

Ok let's change this to 'Needs review', is there something that we absolutely have to do before merging this to main branch?

DER provides so much more features than current state of relation endpoints that I think all efforts should be focused on just one branch now.

As I said in my last comment, the allowed bundles of relations go through validation, via the work done under #2405467-22: Add EntityType and Bundle constraint on entity property of DynamicEntityReferenceItem, but the validation done is not correct. Please see the '@TODO' comments in the code. The allowed bundle settings are not where the current code expected them to be, so the code needs changing to adapt, though that will become trickier as that means the validation cannot simply be done at the level of the DER field items.

#2405467: Add EntityType and Bundle constraint on entity property of DynamicEntityReferenceItem is fixed. Can you please try a new dev release and report any bugs regarding that issue? Thanks!

-der branch is quite out of date. We would need to merge 8.x-1.x and resolve conflicts. We also need to look at tests, which are currently failing in -der branch.

@Denchev will look at bundle validation and provide feedback.

Let's make this ready for merging and push it over the finish line.

@Denchev rerolled -der branch and I pushed the update. Attaching 8.x-1.x...8.x-1.x-der patch to see what testbot thinks about it.

Here are some test fixes. There should be a few remaining related to directional relationship. Working on that part.

Forgot about that. We need the test dependency.

Done.

patch from #39

While working on the test fails I had problems with the condition:

  if (isset($delta)) {
    $group->condition('endpoints.%delta', $delta, '=');
  }

in relation.module relation_query_add_related() it just looks like its ignored.

This is the query i get in the end:

SELECT base_table.revision_id AS revision_id, base_table.relation_id AS relation_id
FROM 
test31978804relation base_table
INNER JOIN test31978804relation__endpoints relation__endpoints ON relation__endpoints.entity_id = base_table.relation_id
INNER JOIN test31978804relation__endpoints relation__endpoints_2 ON relation__endpoints_2.entity_id = base_table.relation_id
WHERE (relation__endpoints.endpoints_target_type = :db_condition_placeholder_0) and (relation__endpoints.endpoints_target_id = :db_condition_placeholder_1) and (relation__endpoints_2.delta = :db_condition_placeholder_2)

it works when i delete the second inner join and changed the relation__endpoints_2 to relation__endpoints. Maybe someone has an idea, looks like a bug?

1. +++ b/composer.json
   @@ -14,5 +14,7 @@
   +    "drupal/dynamic_entity_reference": "^8.1@RC"
   

   1.0 is out :D

2. +++ b/relation.info.yml
   @@ -4,7 +4,5 @@ description: 'Describes relationships between entities.'
   -  - relation_endpoint
   

   Shouldn't we add DER here as well?

+++ b/relation.module
@@ -160,17 +160,17 @@ function relation_query($entity_type = NULL, $entity_id = NULL, $r_index = NULL)
   if (isset($entity_id)) {
     $operator = is_array($entity_id) ? 'IN' : '=';
-    $group->condition('endpoints.entity_id', $entity_id, $operator);
+    $group->condition('endpoints.target_id', $entity_id, $operator);
   }
 
-  if (isset($r_index)) {
-    $group->condition('endpoints.r_index', $r_index, '=');
+  if (isset($delta)) {
+    $group->condition('endpoints.%delta', $delta, '=');
   }

This is actually documented, yay me and @slashrsm ;)

+   *   will find only entities which have at least six tags. Finally, the
+   *   condition on the delta itself accompanied with a condition on the value
+   *   will require the value to appear in the specific delta range. For
+   *   example,
+   *   @code
+   *   ->condition('tags.%delta', 0, '>'))
+   *   ->condition('tags.%delta.value', 'news'))
+   *   @endcode

If you want another condition to be considered in combination with a delta, you need to include %delta in the path.

That's because the query builder takes the prefix of the condition as an identifier for the joins it builds. Same prefix = same join.

It might work if you always include it, not sure, otherwise do it as an if/else with two separate conditions.

#47.1 Nice! changed
#47.2 Yes definitely. Removed by mistake.

#48 Thanks for the help. This helped. I think there are more fails. looking into it...

yea, not getting the delta like this..

Great progress. Few comments:

1. +++ b/relation.module
   @@ -241,17 +241,17 @@ function relation_get_related_entity($entity_type, $entity_id, $relation_type =
   -      $storage_handler = \Drupal::entityTypeManager()->getStorage($entities[1]->entity_type);
   -      return $storage_handler->load($entities[1]->entity_id);
   +      $storage_handler = \Drupal::entityTypeManager()->getStorage($entities[1]->target_type);
   +      return $storage_handler->load($entities[1]->target_id);
        }
   -    $storage_handler = \Drupal::entityTypeManager()->getStorage($entities[0]->entity_type);
   -    return $storage_handler->load($entities[0]->entity_id);
   +    $storage_handler = \Drupal::entityTypeManager()->getStorage($entities[0]->target_type);
   +    return $storage_handler->load($entities[0]->target_id);
   

   Let's convert those two to one-liners. No need for variable assignments IMO.

2. +++ b/relation.module
   @@ -315,19 +315,51 @@ function relation_add_endpoint_field(RelationTypeInterface $relation_type) {
   +        // @TODO DER puts these two here, NOT on the instance. So instead we
   +        // need to allow all possible entity types on the field storage entity
   +        // as instances are added, and ensure the bundles validation on the
   +        // instances is sufficient.
   

   Comment is nice, but probably shouldn't be TODO. Is there anything actionable here?

3. +++ b/relation.module
   @@ -315,19 +315,51 @@ function relation_add_endpoint_field(RelationTypeInterface $relation_type) {
   +      // @TODO Either patch DER to allow '*' in some way (e.g. allow no bundles
   +      // to be selected, to mean all bundle), or subscribe to bundle creation &
   +      // bundle rename events in order to add them then if this setting is *.
   +      // Also, default field settings, which will include all existing content
   +      // entity types, will be merged with our $settings array, so this will not
   +      // sufficiently restrict the allowed entity types. A rethink is therefore
   +      // needed.
   

   This isn't DER actually. It is core's default selection plugin.

   Strange thing about it is the fact that it actually allows wildcard for bundles, but it doesn't allow this to be selected in UI. Go figure... We should probably open an issue about that if it is not already.

   See: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/Plugi...

4. +++ b/src/Controller/RelationController.php
   @@ -28,6 +30,60 @@ class RelationController extends ControllerBase {
   +  public function addPage() {
   ...
   +  public function add(RelationTypeInterface $relation_type) {
   

   Core now provides generic logic for that. We should use that.

   See #2666792: Provide a route provider for add-page of entities for more info and #2716135: Use add page implementation from core for example implementation.

5. +++ b/src/Entity/Relation.php
   @@ -117,7 +117,8 @@ class Relation extends ContentEntityBase implements RelationInterface {
   +      $value = $endpoint->getValue();
   +      $entities[$value['target_type']][] = $value['target_id'];
   

   Do we really need to assign new variable?

6. +++ b/src/RelationListBuilder.php
   @@ -52,21 +52,21 @@ class RelationListBuilder extends EntityListBuilder {
        // Sort entities by their type.
        foreach ($entity->endpoints as $endpoint) {
   -      $entities[$endpoint->entity_type][] = $endpoint->entity_id;
   +      $entities[$endpoint->target_type][] = $endpoint->target_id;
        }
   

   Var that we're building here seems to be unused with the new code.

1. done.
2. Left the comment for now. Not sure but can't we add the selected entity type ids from Relation type. That what i did here.
3. Also left the comment for reference, will remove if were ok with the changes. #2831751: In default selection plugin allow wildcard
4. Should be done.
5. Looks more readable like this but changed it like proposed.
6. We are not using the RelationListBuilder?

Just one more note on this.
Currently when we edit an already created relation type the endpoints field, that already exists is not updated.
I can create a followup issue for this...

6. We are not using the RelationListBuilder?

We are. But not the $entities array built in that loop.

5. Looks more readable like this but changed it like proposed.

No, it does not. Specially if done right. ($endpoint->target_type and $endpoint->target_id)

Currently when we edit an already created relation type the endpoints field, that already exists is not updated.
I can create a followup issue for this...

Yes, please do. Is this the only follow-up that we need or are there more? Is validation complete?

+++ /dev/null
@@ -1,15 +0,0 @@
-{#
-/**
- * @file
- * Default theme implementation to present all relation data.
- *
- * @see template_preprocess_relation()
- *
- * @ingroup themeable
- */
-#}
-<article{{ attributes }}>
-    {% if content %}
-        {{- content -}}
-    {% endif %}
-</article>

I think that we want to keep this template. Why did you decide to remove it?

Sorry missed that...
1. Removed the foreach
2. Fixed endpoints part
3. Yes, there might be one more. Have to test it to confirm but... For all the entities that are not selected when we create the relation type i set the target bundle to an empty array. That way we don't allow any bundle from that entity. If a new entity is added, i think, it will pass validation. I will leave a comment when i know for sure.
4. Reverted that change, I thought we didn't need it.

Forgot to uncomment the locked. Was for testing.

Looks good to me. There are follow-ups, but I think that we can commit this and tackle those afterwards.

Followups:
#2833635: Update endpoints field when relation type is updated
#2833645: After adding new entity fatal when creating relation

Nice!

This will break all existing d8 installations, and as we don't have releases yet we don't have any good way to communicate this major change. I can think of 2 options. We can add a notice text to relation module page or we can open a new 8.x-2.x branch for this new relation architecture.

We've clearly communicated that there will be absolutely no upgrade path so that isn't a problem. But I know I will have to update a installation or two so there will be at least some experimental update scripts coming in future. To make that a bit easier I vote for 8.x-2.x branch.

Works for me. I think that we should clearly state that there is no further development planned on 1.x branch.

New 8.x-2.x branch has now been opened for this.