Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data streams cannot be deleted or removed from sensor assets #485

Open
mstenta opened this issue Jan 16, 2022 · 7 comments
Open

Data streams cannot be deleted or removed from sensor assets #485

mstenta opened this issue Jan 16, 2022 · 7 comments
Labels

Comments

@mstenta
Copy link
Member

mstenta commented Jan 16, 2022

It appears that the Entity Reference Integrity module is making it impossible to delete data_stream entities when they are referenced by Sensor assets. And when you try to "Remove" the data stream from the sensor asset, it tries to delete the data stream, which then runs into the same issue. So it's a catch-22, and it is not possible to delete data streams without first disabling the Entity Reference Integrity constraint for data_stream entities via /admin/config/content/entity-reference-integrity.

@mstenta mstenta added the bug label Jan 16, 2022
@mstenta
Copy link
Member Author

mstenta commented Jan 16, 2022

FWIW I only found this issue after fixing #486

@mstenta mstenta changed the title Data streams cannot be removed from sensor assets Data streams cannot be deleted or removed from sensor assets Jan 16, 2022
@mstenta
Copy link
Member Author

mstenta commented Jan 16, 2022

Ah we have a relevant @todo:

// @todo Considerations for improved entity reference integrity?

It seems that this hook_ENTITY_TYPE_delete() intends to clean up references on sensor assets, but this code can't actually work because of Entity Reference Integrity currently.

@mstenta
Copy link
Member Author

mstenta commented Jan 16, 2022

I suppose we could consider removing the Entity Reference Integrity constraint for data_stream entities, knowing that that hook_ENTITY_TYPE_delete() logic is in place.

Although... that only covers data_stream entities referenced by asset entities in a field named data_stream. So it would miss any additional data stream reference fields added downstream.

Another idea: the "Remove" option on the Inline Entity Form is currently deleting the referenced entity - not just removing it. I imagine it's possible to configure it such that it only removes and does not delete. This would at least allow you to first remove it, and then delete it. The disadvantage of this, of course, is it assumes a user knows they need to do both. Simply removing the data stream from an asset without deleting the data stream means there would be orphaned data streams in the system. Not the end of the world... and maybe it's time we consider adding a collection route to show all data streams (along with assets that reference them)? Related: #486

@paul121
Copy link
Member

paul121 commented Jan 18, 2022

Another idea: the "Remove" option on the Inline Entity Form is currently deleting the referenced entity - not just removing it.

Hmmm interesting. It seems important to alert the user that the data stream data will be deleted as well. Another option would be to exclude the Remove option from the inline entity form (removing and deleting from the sensor asset). We could add a note to the effect of "You can delete data stream entities from their individual pages." *Although this might conflict with reference integrity. Hmmmm

and maybe it's time we consider adding a collection route to show all data streams (along with assets that reference them)? Related

FWIW we had this 😄 - #486 (comment)

@paul121
Copy link
Member

paul121 commented Jan 18, 2022

It seems that this hook_ENTITY_TYPE_delete() intends to clean up references on sensor assets, but this code can't actually work because of Entity Reference Integrity currently.

Another issue here - this hook should actually be implemented in farm_sensor.module, not data_stream.module. I think this is an artifact from when we added a data_stream reference field to all assets, not only sensor assets.

@mstenta
Copy link
Member Author

mstenta commented Jan 19, 2022

@paul121 created two commits related to this, which are now in this branch: 2.x...mstenta:2.x-data-stream-references

@mstenta
Copy link
Member Author

mstenta commented Jan 19, 2022

Does Inline Entity Form have an option for "removing" without "deleting"? Or is it all-or-nothing?

If it does allow removing without deleting, then the best option is probably to add a collection page for administering data streams, allow "removing" them from assets (without deleting), and add a note like you suggest. Something like:

"This will unlink the data stream from the sensor asset. The data stream will not be deleted. To delete data streams, go to [collection page]."

And on the data stream deletion confirm form, we add:

"This will delete all data associated with this data stream!"

This would keep Entity Reference Integrity working as intended, allow data streams to be removed from sensor assets, and subsequently deleted, making it clear that data will be deleted along with it. And we can remove the hook_ENTITY_TYPE_delete() entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants