Skip to content
This repository has been archived by the owner on Sep 14, 2024. It is now read-only.

Store base field "entity_id" as entity_reference not string #114

Open
Sam152 opened this issue Dec 31, 2014 · 7 comments
Open

Store base field "entity_id" as entity_reference not string #114

Sam152 opened this issue Dec 31, 2014 · 7 comments

Comments

@Sam152
Copy link

Sam152 commented Dec 31, 2014

The current string storage doesn't allow for importing/exporting of content and would probably also negatively impact REST services. Not sure if this is possible with a reference to any entity type, but perhaps this can be moved from a base field to one which is dynamically created when a user creates a flag type.

@Berdir
Copy link
Contributor

Berdir commented Dec 31, 2014

Core does this for comments and sets the entity type per bundle. That works, but it's tricky and only works for base fields, because the entity type is a storage setting that is set by bundle.

Adding it per bundle is not really possible as you can't have multiple fields with the same name on different bundles, you would need a dynamic field name, which would make queries a lot more complicated.

Another approach would be the dynamic_entity_reference contrib module, which is a field type for a dynamic entity_type/id reference.

@Sam152
Copy link
Author

Sam152 commented Dec 31, 2014

Hm, yeah I can see why the dynamically generated field names would be a massive pain and WTF for developers.

So long as dynamic_entity_reference works with UUIDs instead of IDs, it would technically solve the problem, but it does also add a dependency for what feels like something that should be relatively simple.

What about storing the UUID as a string instead of the nid? I can't think of any major downsides, other than people who would expect or need to deal with NIDs in certain situations? A rest endpoint which only deals with nids?

@joachim-n
Copy link

Core does this for comments and sets the entity type per bundle. That works, but it's tricky and only works for base fields, because the entity type is a storage setting that is set by bundle.

Surely that can be done here too? Each flag is a separate bundle of the flagging entities -- flaggings are attached to entities as comments are.

@Berdir
Copy link
Contributor

Berdir commented Jan 1, 2015

Yes, what works for comment would also work here, same pattern.

My point was that it is fragile and theoretically not supported. That said, I made comment work like it does now and it's the only solution that I can think of short of depending on dynamic_entity_reference.

@socketwench
Copy link
Owner

This would be for the Flaggings right? Not the flag entity?

What about storing the UUID as a string instead of the nid? I can't think of any major downsides, other
than people who would expect or need to deal with NIDs in certain situations? A rest endpoint which
only deals with nids?

It shouldn't be too hard to do that, as the entity_id field is just a string and not stored as a number. We'd need to modify the field definitions and decide if we want to change all the FlagService API methods to use UUIDs and not Entity IDs.

My point was that it is fragile and theoretically not supported. That said, I made comment work like it
does now and it's the only solution that I can think of short of depending on dynamic_entity_reference.

Hm. That doesn't exactly inspire me with confidence. Using dynamic_entity_reference is also a tricky choice. I'm concerned that depending on it for such a core piece of functionality in Flag could result in other problems. UUIDs seem like a safer bet.

@joachim-n
Copy link

That doesn't exactly inspire me with confidence

My take on this is that if it's good enough for core's comment module, then it's good enough for us. If and when Comment module moves to something better, we can follow.

Also, using UUIDs to relate the flagging table to the entity table is going to affect the performance of views that involve flaggings.

@Sam152
Copy link
Author

Sam152 commented Jan 16, 2015

Related: https://www.drupal.org/node/2407587

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants