Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
After deleting a node which contains paragraph entities, these paragraphs entities are not deleted.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff-2429335-24-26.txt | 690 bytes | johnchque |
#26 | deletion_of_referenced-2429335-26.patch | 2.75 KB | johnchque |
| |||
#24 | interdiff-2429335-22-24.txt | 563 bytes | johnchque |
#24 | deletion_of_referenced-2429335-24.patch | 2.67 KB | johnchque |
| |||
#22 | interdiff-2429335-18-22.txt | 3.07 KB | johnchque |
Comments
Comment #1
danielbeeke CreditAttribution: danielbeeke commentedHere 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.
Comment #2
danielbeeke CreditAttribution: danielbeeke commentedAh.. encoding stuff.
Comment #3
danielbeeke CreditAttribution: danielbeeke commentedComment #4
jeroen.b CreditAttribution: jeroen.b at .VDMi/ commentedI 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.
Comment #5
rcross CreditAttribution: rcross at CrossFunctional commentedIt may be worth consider how this design decision would impact things like this issue around node_clone
Comment #6
miro_dietikerIn 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.
Comment #7
miro_dietikerAh, 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.
Comment #8
BerdirI 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.
Comment #9
miro_dietikerI 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.
Comment #10
jeroen.b CreditAttribution: jeroen.b at .VDMi/ commentedYeah, 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.
Comment #11
miro_dietikerWe should not forget: (stolen from casey) "We should at least check if there are other revisions referencing the
paragraphentity."Comment #12
miro_dietikerSupport for nesting have been added to paragraphs... So this issue needs to be checked to support nesting with proper test coverage.
Comment #13
miro_dietikerRaising 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.
Comment #14
jeroen.b CreditAttribution: jeroen.b at .VDMi/ commentedComment #15
johnchqueChanges made to delete entities with parent id and type that are introduced on this issue: #2641824: Maintain composite relationship
Comment #17
miro_dietikerNot needed.
Comment #18
johnchqueChanges made. Test coverage will be written as soon as the other issue is committed.
Comment #20
johnchqueThe other issue has been committed, I will upload a patch as soon as I add test coverage.
Comment #21
miro_dietikerStill not needed. :-)
Comment #22
johnchqueAdded delete function, added tests, should work now. :)
Comment #23
miro_dietikerLooks pretty good. Just a tiny thing.
Please do NOT hardcode IDs. :-)
Comment #24
johnchqueChanges made based on comment #23. :)
Comment #25
miro_dietikerStill hardcoded. :-)
Comment #26
johnchqueFixed. :D
Comment #27
miro_dietikerYes, thx, committed. :-)
Comment #30
sam.spinoy@gmail.com CreditAttribution: sam.spinoy@gmail.com at Adapt commentedSorry 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.
Comment #31
BerdirNo, 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.
Comment #32
sam.spinoy@gmail.com CreditAttribution: sam.spinoy@gmail.com at Adapt commentedI 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.
Comment #33
nord102I am having the same issue as above. The paragraph references are not cleaned up. We are using a Multilingual site as well.
Comment #34
AnybodyIf the problem still exists please open a new issue referencing THIS issue and link the issue here as follow-up please!