This is a follow-up to #2759157: Orphaned field collections break update 7008. That issue added field_collection_update_7008(), which moves field values to the correct translation, and whilst doing so, also deletes orphaned field collections.
However, it considered any field collection that returned nothing for $item->hostEntity() to be an orphan, and therefore safe to delete. BUT a field collection attached to an old revision would return nothing for that method too, because its default_revision property would incorrectly be TRUE. Various other issues exist that are due to that property being incorrect, but the very serious point here is that field_collection_update_7008() will therefore be deleting field collections that it should not be deleting.
Patch to follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | field-collection-revisions-deleted-2942188-5.patch | 636 bytes | james.williams |
| #3 | field-collection-revisions-deleted-2942188-3.patch | 1.29 KB | james.williams |
| #2 | field-collection-revisions-deleted-2942188-2.patch | 840 bytes | james.williams |
Comments
Comment #2
james.williamsThis patch forces the revision tables to be checked.
Another approach would be to just avoid deleting any field collections at all, leaving the possibly-orphaned entities lying around in the database.
Comment #3
james.williamsThe more I think about it, the less confident I am that deleting supposedly-orphaned field collections is a good idea. It's just a tidy-up task, not strictly necessary at all, but the risk of deleting data that isn't actually correct to delete is relatively high. For example, what if a field_data_* or even field_revision_* table for a field collection field does not contain the current revision held in the field_collection_item table? That field collection should not be deleted! But at the moment, it would be. There are a number of edge cases that could be due to inconsistent data in tables like this, which could be accounted for ... but rather than try & work harder to account for them, the best thing to do really is just avoid deleting them.
This new patch avoids deleting them, and just logs a count to watchdog. If the user is keen to clear up their database, they could then use that as a starting point.
Comment #4
james.williams(Meanwhile, I'll do more thinking on how to search harder for host entities (e.g. in the edge case I outlined), as they would still need their translation values copied, but wouldn't currently. But I strongly recommend removing the current code that deletes supposedly-orphaned entities, e.g. using the patch I've provided, as that is in the current dev branch!)
Comment #5
james.williamsSorry for the continual updates... here's a patch that really does only change to log rather than delete supposedly-orphaned entities. I realised that the other chunk of the previous patch would potentially get the wrong revision too, so this final patch really does just focus on avoiding incorrect deletions.
Any field collections that can't be found via
->hostEntity()will either be:1) True orphans (could be deleted, but that's not really necessary, hence this issue/patch)
2) Still used by a non-default revision, despite having their
default_revisionproperty marked as TRUE (because there's no code to ever mark it as FALSE!)3) In use, either by a default or non-default revision, but under a different revision to what is in the
{field_collection_item}table!I think that covers all of the edge cases. I don't know whether they could all be covered off in follow-up issues, because they all involve handling revisions, or revisions that are out-of-sync with the
{field_collection_item}table, and there are a lot of outstanding issues around those (e.g. #2352047: Creating or updating non-standard revisions leads to data loss). There may not even be a general solution that works for all sites. So the best first immediate step is just to avoid deleting field collections since we're not sure that they really are orphaned any more.