Support from Acquia helps fund testing for Drupal Acquia logo

Comments

danielbeeke’s picture

Here is a patch,

There are two ways to address this issue, fixing it via entity reference revisions or via the paragraphs module.

Option 1 (as JeroenB said on skype):
Create a key in the entity annotation, which tells the entity reference revisions module to clean up orphans.

Option 2 (in the patch)
Clean up on hook_entity_delete. I think the widget makes it so you don't want to use the orphans. With other widgets it could be that you want to references the orphan later.

danielbeeke’s picture

Ah.. encoding stuff.

danielbeeke’s picture

Status: Active » Needs review
jeroen.b’s picture

I rather go for option 1. I'd really like the Entity Reference Revisions module to clean up the orphaned entities when the annotation of the Entity tells it only creates embedded entities.

I think checking for the widget type is a "dirty" solution and only needed for edge cases, but since we control the Entity Reference Revisions module I'd like to fix it there.

rcross’s picture

It may be worth consider how this design decision would impact things like this issue around node_clone

miro_dietiker’s picture

Status: Needs review » Needs work

In Drupal terms, paragraphsh are fully qualified entities.
However, in our semantics, paragraphs are considered a composite and "owned" by the host entity.

This means, cloning the host semantically requires you to clone the paragraphs, too.
And it also means that every paragraph can be only place once in its host entity.

Since Drupal core does not introduce such a concept, we need to catch up with such a situation. As a result, a clone operation introduced by any (core, contrib) module then needs to be explicitly supported by the paragraphs module and establish the concept.
We should create a followup to support common D8 cloning.

Indeed, the proposed solution results in different responsibility:

For option A, entity reference revisions would know about the semantics of an inline entity and can clean up on its own when deletion happens.
Here's an alternative to define a new annotation: Every regular entity has a canonical link template pointing to its canonical URL. The lack of such a thing can be considered a qualified sign that the entity is considered a composite use entity. Entity reference revisions could propagate delete to target entities in case their annotation lacks a canonical definition.

For option B, entity references revisions couldn't do its job. Instead, the module introducing a widget (paragraphs) needs to catch up with the situation the way you did.
Note that just because one widget presents entities in an inline / composite style does not necessarily mean they are exclusively used that way.

Option A is cleaner, but also has clear design decisions / side effects.
- If we plan to #2414865: [META] Reuse paragraphs / Library ever, then semantics (if composite or reusable) would need to be done per entity instance. Since it is not a priority, we can exclude it in consideration.

I would ask much more the question, if the "composite" semantics is a matter of the relationship (field definition) or the target entity. We could add a setting to propagate actions such as "clone target" and "delete target" on the entity reference revision field storage definition.

miro_dietiker’s picture

Ah, forgot to mention: Core has no concept of inline entity or does not know about semantics of entity relationships at all.
But it introduced some related concept with Files, provided with the FileUsageInterface, designed for target (file) reuse with autorelease if the use is zero.

Berdir’s picture

I think doing anything based on the widget that doesn't happen in the context of a form mode/display in 8.x is problematic. It's actually possible to have two different form displays and use two different widgets, what are you going to do then?

On clone vs. delete, not 100% sure, would need to think more about it. As miro said, it depends a lot on #2414865: [META] Reuse paragraphs / Library. If we ever plan on doing/supporting that, then we would not need to duplicate them on clone. But until we actually do, it might be a good idea to do so, yes.

And yes, if we want to delete and support re-use, then we'd essentially need usage tracking similar to files. Which is pretty complicated.

miro_dietiker’s picture

I tend to recommend to put it as a storage setting into entity reference revision field.
With this approach: It is independent from paragraphs. And no matter what widget is used, it is a semantic relationship definition. "Target entities are composite of the host entity. Delete the targets when the host is deleted."
I think this is pretty straight forward.

From our current perspective, it should be enabled by default because that (composite relationship) is why people use entity reference revisions module.

jeroen.b’s picture

Project: Paragraphs » Entity Reference Revisions

Yeah, I think using the storage settings for this would be a good solution for now.
Maybe we should create a new issue for tracking entity usage, as that sounds like a nice-to-have.

I'll move this to the Entity Reference Revisions module for now.

miro_dietiker’s picture

We should not forget: (stolen from casey) "We should at least check if there are other revisions referencing the paragraph entity."

miro_dietiker’s picture

Support for nesting have been added to paragraphs... So this issue needs to be checked to support nesting with proper test coverage.

miro_dietiker’s picture

Priority: Normal » Major
Status: Needs work » Active

Raising priority a bit here. We should not stay with orphans in the database.
And setting back to active since we decided to take a different approach.. and the patch was for paragraphs.

jeroen.b’s picture

johnchque’s picture

Status: Active » Needs review
FileSize
1.16 KB

Changes made to delete entities with parent id and type that are introduced on this issue: #2641824: Maintain composite relationship

Status: Needs review » Needs work

The last submitted patch, 15: deletion_of_referenced-2429335-15.patch, failed testing.

miro_dietiker’s picture

  1. +++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
    @@ -249,6 +249,23 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
    +        if (!($parent_entity = $this->getEntity())) {
    +          $entity->setNewRevision(FALSE);
    

    Not needed.

johnchque’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
909 bytes

Changes made. Test coverage will be written as soon as the other issue is committed.

Status: Needs review » Needs work

The last submitted patch, 18: deletion_of_referenced-2429335-18.patch, failed testing.

johnchque’s picture

The other issue has been committed, I will upload a patch as soon as I add test coverage.

miro_dietiker’s picture

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -249,6 +249,23 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+        if (!($parent_entity = $this->getEntity())) {

Still not needed. :-)

johnchque’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
3.07 KB

Added delete function, added tests, should work now. :)

miro_dietiker’s picture

Status: Needs review » Needs work

Looks pretty good. Just a tiny thing.

+++ b/src/Tests/EntityReferenceRevisionsAutocompleteTest.php
@@ -101,6 +103,12 @@ class EntityReferenceRevisionsAutocompleteTest extends WebTestBase {
+    $node = Node::load(1);
...
+    $this->assertNotNull(BlockContent::load(1));

Please do NOT hardcode IDs. :-)

johnchque’s picture

Status: Needs work » Needs review
FileSize
2.67 KB
563 bytes

Changes made based on comment #23. :)

miro_dietiker’s picture

Status: Needs review » Needs work
+++ b/src/Tests/EntityReferenceRevisionsAutocompleteTest.php
@@ -101,6 +103,12 @@ class EntityReferenceRevisionsAutocompleteTest extends WebTestBase {
+    $node = Node::load(1);

Still hardcoded. :-)

johnchque’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
690 bytes

Fixed. :D

miro_dietiker’s picture

Status: Needs review » Fixed

Yes, thx, committed. :-)

Status: Fixed » Closed (fixed)

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

sam.spinoy@gmail.com’s picture

Sorry to comment on an already-closed issue, but I noticed that since this commit there has been added an extra check in the delete method of class EntityReferenceRevisionsItem: if (!$this->getFieldDefinition()->isTranslatable())

Which means that if you have your field set up as translatable, you will not have the cleanup and have orphaned paragraphs in your db. Maybe I'm missing something, but doesn't it make more sense to check if the node is translatable, rather than the field? I have a setup with only one language, which isn't English, so I need to have my fields translatable so I can translate the label of the field. My nodes themselves are created in the default language and are not translatable.

Berdir’s picture

I have a setup with only one language, which isn't English, so I need to have my fields translatable so I can translate the label of the field.

No, you don't. The field translatability setting is only about the content, you can always have the label translated using config translation. completely different mechanism.

sam.spinoy@gmail.com’s picture

I have the following multilingual modules enabled: Configuration Translation, Interface Translation and Language. Content translation is disabled. Field config still says my field is translatable.

Regardless, my point is still valid. Multilingual sites will not have their references cleaned up.

nord102’s picture

I am having the same issue as above. The paragraph references are not cleaned up. We are using a Multilingual site as well.

Anybody’s picture

If the problem still exists please open a new issue referencing THIS issue and link the issue here as follow-up please!