Problem/Motivation
On a server, for a specific entity, an entity reference field is left empty, on purpose. I haven't found a way for allowing this information to be synced on the client.
I noticed in the JsonapiHelper service, in the relationshipHandleable method, that it filters on fields with no data.
I there a specific reason for not allowing a reference to be removed?
Proposed resolution
Can we return TRUE on empty $field_data['data'] ? Does it have other implications?
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | entity_share-remove_reference-2923632-8.patch | 5.81 KB | grimreaper |
| #2 | allow_references_removal_sync-2923632-2.patch | 1023 bytes | bogdan.racz |
Comments
Comment #2
bogdan.racz commentedI attached a testing purpose patch.
Comment #3
bogdan.racz commentedComment #4
grimreaperHello,
Thanks for the feedback.
Yes it is strange to not be able to remove a reference. I have reproduce the behavior.
I will check why I have done that, if I see no reason, I will remove the check.
Comment #5
grimreaperHello,
OK, now I see/remember why I wrote this code and it can't stay in this state.
The problem:
We don't want to share user entity because how do you handle the password? And in the case of big websites sharing content, most of the time there is some SSO solution implicated. And we also don't want to share config entities to avoid side effect and possible data loss.
A possible situation:
Step 1:
Site A:
- content A has an entity reference field targeting users.
Site B import content A.
Currently the entity reference field will be left empty because the reference fields targets an user entity.
Step 2:
Content A on site B is changed and it references "the equivalent user" (not the same UUID) or another user.
On site B, content A is pulled once again to be synchronized. Currently, the reference on site B to the other user won't be lost, because we skip entity reference to users.
Step 3:
Content A on site A is changed and there is no more reference.
On site B, content A is pulled once again to be synchronized. Currently, the reference on site B to the other user won't be lost because we skip empty fields. With the patch, the reference will be lost.
Possible solution:
If the entity reference field is empty, there is no way to know in the JSON payload that it is a reference field targeting users. So let's check on the field configuration.
BUT... in the case of more complicated entity reference fields, such as a dynamic entity reference field, the field can reference users among other entity types, so we can't skip the field entirely.
Therefore we need to check the entity type indicated in each relationship (entity reference), but we can't check the entity type if there is no value....
Maybe we can add a parameter, global or on each pull operation, to tell if in the case of empty entity reference field on the source site, we want the references removed on the target site (the site that is pulling the content).
But on the long term, that may add some maintenance burden to maintain two behaviors.
What do you think on this proposal?
Comment #6
grimreaperHello,
After some thoughts about this and a discussion with a co-worker, we went to a solution enough simple to be done and to respond to the most usecases.
Instead of checking the entity reference types on the payload, let's check in the field definition/configuration if it targets users or config entities.
And in case of more complicated reference fields such as dynamic entity reference fields, if the field CAN target users or config entities, let's skip the field entirely. Maybe later add an entrypoint (event or plugin system or else) to allow other modules to tell if they allow synchronisation.
I will wait a bit for some reply/suggestion but I can't see another way that preserves the behavior for sites currently using entity share and is simple to implement.
Comment #7
grimreaperAdding #2924099: Dynamic entity reference: no user in related endpoint as a related issue because I found this strange behavior while testing dynamic entity reference.
Comment #8
grimreaperHello,
@rbmboogie can you please test the attached patch?
It implements the logic from comment 6.
Comment #9
bogdan.racz commented@Grimreaper, I've checked the patch and tested it. It worked ok.
+1
Comment #11
grimreaperHello,
Thanks for the review and the test.
Patch is now merged. I will make a new release soon.
Comment #12
grimreaper