Deleting revisions via node_revision_delete wrongly deletes entire field collection

I tested the patch and it works just fine. This is a rather critical issue so it should get some attention, hope it does.

Patch was made against the beta5, but it does still apply fine in dev.

For everybody who wants to reproduce the problem and wants to help testing the patch:

1. Attach a (single or multiple) field collection (fc) to a node type.
2. Configure maybe only one single text field to the fc.
3. Create a test node with a nested fc item with filling its text field.
4. Create a new node revision (maybe with simple editing the node).
5. Delete the first revision (maybe via node revisions tab).

I get this error message two times:

Notice: Trying to get property of non-object in field_collection_field_get_entity() (line 1599 of /Applications/MAMP/htdocs/fctest/sites/all/modules/field_collection/field_collection.module).

The field entries of the fc item are gone. It's the same with multiple fc items (delta entries).
In the database there is still the single or all delta fc item(s) connected to the node but the nested field entries are gone.

I found this bug by analysing another problem where the node revisions were created by the field_collection rules integration. But this is another issue I will report later if I did't find an existing issue like this. But this problem is without rules on a fresh drupal system (standard install) with only fc and the entity module installed.

The patch of #1 is solving the bad behavior described above in my first tests. I have a lot's of important field_collection data in one project with unwanted revisions produced by the other bug. This important data can be destroyed if the revision deletion is used. So I hope of a soon solution of this problem and do some additional tests of the patch in a copy of this important system.

Re-tested and it seems to work for our project.

#1: field_collection-node-revision-deletion-issues-2.patch queued for re-testing.

I've been testing patch #9 and it is working well. Thanks!

RTBC??

hm, does the problem exist on a new install as well, or this is an issue with ugprades only?

I've written some code for our site that checks whether a field collection and/or it's revision is being referenced by any existing entities before deleting

I don't think we should start caring about a database support we do not support - instead we should do an update function that fixes the database state. (I was thinking there is already one? Not sure).

1. +++ field_collection/field_collection.module	2013-10-11 12:47:56.000000000 +0100
   @@ -403,7 +403,66 @@
   +  protected function fetchAllHostEntityIds($for_current_revision_only = FALSE, $exclude_this_revision_id = NULL) {
   

   This needs lots of coding style fixes.

2. +++ field_collection/field_collection.module	2013-10-11 12:47:56.000000000 +0100
   @@ -534,28 +593,42 @@
   +  public function deleteRevision($skip_host_update = FALSE, $calling_host_entity_revision_id = NULL) {
   

   I'M not a fan of that added paramter, is it needed? Also its not the calling host entity, it's the only host entity - we support just one.

3. +++ field_collection/field_collection.module	2013-10-11 12:47:56.000000000 +0100
   @@ -534,28 +593,42 @@
   +    $host_entities_using_this_revision = $this->fetchAllHostEntityIds(TRUE, $calling_host_entity_revision_id);
   

   This should be handled by an update function instead.

4. +++ field_collection/field_collection.module	2013-10-11 12:47:56.000000000 +0100
   @@ -1911,3 +1989,18 @@
   +function field_collection_get_revision_id($entity_type, $entity) {
   

   Not sure why we need a wrapper here? A regular list() call should do it?

@fago I am pretty sure the issue exists on new installs as well, I remember trying it out on a brand new install and noticing this behavior.

Can confirm the issue exists in current version. The patch does seem to make things work but I agree that there should be an update to fix the data instead of a check for incorrect data from a previous update.

@jmuzz, not sure I understand your last comment, have you assigned it to you for anything? what are the two files that are referenced there, are those new patches by you? just trying to keep the issue organized so it's better/quicker to review.

I assigned it to me because I think I can find time to work on making the required changes soon.

The files that were hidden are old versions of the patch. Usually only one version of the patch needs to be displayed in the issue summary.

@jmuzz, did you by any chance find some time to fix fago's remarks? I can confirm that the patch works, but agree that it's not what should be commited to the f_c module.

I made some progress on it. It will be more work than I originally thought.

The original patch that introduced revisions should have made a new field collection revision for every revision that its node had, but it really just assigned a single field collection revision to each one of them. On top of that, more revisions could have been created afterwards. The duplicate field collection revisions and their data have to be copied to new revision numbers and then the references involving all of the revisions that came after the original update need to be reassigned to new numbers that keep the revision id's increasing over time.

I can post what I have so far later on tonight.

I had the problem of deleting old revisions with an attached field collection caused all field collection revisions for that node to be removed. This was on a system that had not been upgraded, and is running 7.x-1.0-beta7. The patch above (comment #9) fixed the problem for me.

Thanks :)

I'm using the revision cleanup module to remove old revisions of nodes containing field collections. This is on a database that had pre-existing node revisions before the field collection module was enabled - deleting old revisions on those nodes would cause all field collections on the current revision of the node to be removed, as discussed above.

The patch at #9 allows this to work without deleting field collections from the remaining current revision of the nodes, thanks.

With no old revisions remaining in the database the patch can be removed.

Patch from #9, rolled on latest -dev, fully works for me and have passed all tests. Thank you!

@jmuzz Did you make some progress? This is a critical bug and is assigned to you. Thank you for your efforts.

A fair observation. I won't likely have time to work on it. Sorry if I prevented anybody from working on this.

This bug causes huge problems because it compromises the integrity of having revisions in the first place. Not only do revision-deletes destroy the entire field collection for that node, but anything that invokes user_delete will invoke user_node_delete and cause the same problem. Below is my attempt at addressing the problem and the reasoning for my proposed fix. I reviewed patch #9 and though it does solve the problem, I feel like it is trying to do too much and changes too much.

Problem with logic

I noticed that the primary issue is because of this line of code in field_collection.module:

   * If no revisions are left or the host is not revisionable, the whole item
   * is deleted.
   */
  public function deleteRevision($skip_host_update = FALSE) {

   ....

    if (empty($info['entity keys']['revision']) || !$this->hostEntity()) {
      return $this->delete();
    }

First off, that logic is bad because $this->hostEntity() will always be empty on the deletion of a revision because, as the comments in patch #9 explains, " When called from node_revision_delete, the host entity has already been removed from the database".

Secondly, I can't even find a use case in Drupal via the user interface that will trigger that first condition. When revisions aren't enabled, it doesn't even call that function.

Problem with test cases

Also, this test case I believe is also wrong:

    // Test that deleting the first node revision deletes the whole field
    // collection item as it contains its last revision.
    node_revision_delete($node_vid);
    $this->assertFalse(field_collection_item_load($item->item_id), 'Archived field collection deleted when last revision deleted.');

Deleting the first numerical revision should not delete the entire field collection. Rather, deleting the current revision should delete everything.

Proposed Solution

1. Remove that line of logic from field_collection.module since its just not used
2. Remove that test case
3. Add the test case from Patch #9

Attached is a patch of exactly what I described. I'm actively looking at solving this problem so if you'd like to discuss, I'd be happy to accelerate, feel free to PM me.

So what I don't get is:

Scenario 1:

- Node: 2:r1, FC Item: 4711,r1

If I do a delete revision on r1, I expect the whole node and FCs to be deleted, because there is no revision left:

=> Host Entity is empty after node_revision_delete as the node was deleted as well

I believe the test case tries to cater for that case.

Scenario 2:

- Node: 3:r1, FC Item: 4712,r1

-> Save

- Node: 3:r2, FC Item: 4712,r2

If I do a delete revision on r1 OR revision on r2, I expect the revision to be removed, but nothing else to change.

Why would hostEntity() be empty? If the node:3 would still exist - albeit in another revision?

I think that is the real bug as Scenario 1, should still be supported.

I am not opposed to the quick fix provided in #27, as I don't care if there is data lingering around (does never hurt to have too much data), but just saying that the host issue should be fixed, too.

I checked the host issue and yes the line is just bad, but the test should remain to delete the last field collection as its handled below in the code.

Good review Fabianx. I reviewed the bad line of code of "hostEntity" being empty and have some results to share...

When this is called in deleteRevision:

if (empty($info['entity keys']['revision']) || !$this->hostEntity()) {

Problem #1: $this->hostEntity is not even set, because setEntity was never ever called. So, it gets to this line of code in hostEntity():

$this->hostEntity = entity_revision_load($this->hostEntityType, $this->hostEntityRevisionId);

And those variables have these values:

• $this->hostEntityType = node
• $this->hostEntityRevisionId = the node's revision

All good right? Nope! Problem #2 Entity_revision_load fails because that revision has already been deleted by "node.module:node_revision_delete" which invoked "field_collection.module:deleteRevision" up in the call stack:

    db_delete('node_revision')
      ->condition('nid', $revision->nid)
      ->condition('vid', $revision->vid)
      ->execute();
    module_invoke_all('node_revision_delete', $revision);
    field_attach_delete_revision('node', $revision);

and as a result $this->hostEntity is set to 0

Possible Solution

We need to make sure $this->hostEntity is set properly either by

1. making sure its set during the initial load in field_collection_field_delete_revision
2. making sure its set even if the revision is deleted in hostEntity

I'll probably experiment with each of those soon but I thought I'd see if any one has some feedback on those approaches

I also thought about fixing it that way, but I think we really can just remove the lines as you did, because there is no host entity to load.

Probably we know the host entity though still during hook_field_revision_delete, so we definitely could set it there ...

I only said that removing the test coverage should not be necessary as Scenario 1 of my thing should still be working (even with those 3 lines removed).

So we should fix the tests rather than remove them.

Gotcha. Yah fixing that hostEntity issue might be moot because it never gets there in the first place. Would love to hear the maintainers weigh in on that.

As for the test case I removed, your Scenario 2 actually fails the current test. Are you suggesting that I put the test case back in but adjust the logic so its actually testing the right thing?

Makes sense, if we're on the same page, I can totally create that patch.

Yes, lets adjust the test coverage :).

Attached is my same patch for #27 but with the original test case added back to be what actually happens.

Anyone down for testing this and getting it to RTBC?

+++ b/field_collection.test
@@ -195,10 +195,10 @@ class FieldCollectionBasicTestCase extends DrupalWebTestCase {
-    $this->assertFalse(field_collection_item_load($item->item_id), 'Archived field collection deleted when last revision deleted.');
+    $this->assertTrue(field_collection_item_load($item->item_id), 'Archived field collection deleted when last revision deleted.');

Don't we need to update the test text here?

Yes yes we do!

Looks great to me!

I agree the if statement there is incorrect, but it still needs to check for the case of the final revision of a field collection item being deleted.

+++ b/field_collection.test
@@ -217,6 +217,20 @@ class FieldCollectionBasicTestCase extends DrupalWebTestCase {
+    ¶

Extra white space.

+++ b/field_collection.test
@@ -195,10 +195,10 @@ class FieldCollectionBasicTestCase extends DrupalWebTestCase {
-    // Test that deleting the first node revision deletes the whole field
-    // collection item as it contains its last revision.
+    // Test that deleting the first node revision does not delete the whole field
+    // collection item since the other revision is active.
     node_revision_delete($node_vid);
-    $this->assertFalse(field_collection_item_load($item->item_id), 'Archived field collection deleted when last revision deleted.');
+    $this->assertTrue(field_collection_item_load($item->item_id), 'Field collection was not deleted when the first revision was deleted.');

The tests before this point delete the other two revisions of the field collection item that existed. The first node revision actually does have the final remaining revision of the field collection item as the comments stated.

I'd be willing to commit this by itself, but there is also the case of people who used field collection items before it supported revisions and had nodes with multiple revisions when they upgraded. They now have multiple revisions of nodes that all point to one revision of a field collections item. For them, deleting that field collection item revision or one of the node's revisions will remove the field collection item from all revisions of the node that existed before the upgrade. An update still needs to be made to correct that data. Some code I posted in #21 may be able to be used in such an update.

Thanks for weighing in on it jmuzz, having maintainer feedback is exactly what we needed!

Do you know which version of field collections I could use to reproduce this use case?

So when deleting 2 items, I think we actually have 2 options:

• Option 1: Converting all broken field collections to working ones ( which is what your #21 is referring to, no ? )
• Option 2: If we detect that this a revision that another revision shares field collections with, prevent deletion saying that its not supported.

Honestly, right now those people with these duplicate references to field collections in their revisions know that deleting one reference will delete the entire field collection. So even if we rolled forward with this patch, if they deleted one revision, it wouldn't delete the node anymore just the support for field collections, which is what they assume is going to happen anyway. I think we may be able to separate these issues so we can not have that one hold this one back since its not causing any more harm than what is already there.

What do you think?

To see the problem with the upgrade checkout tags/7.x-1.0-beta4 as it was the final release before revision support. Make a node with a field collection and a few revisions, checkout 7.x-1.x, run update.php, make a few more revisions on the node to make it more interesting, and then try to fix the resulting data.

What fago says in #14 implies that option 1 would be preferable, and I agree. It is what #21 is about.

The nodes don't get deleted, but a revision of a field collection shared by many nodes can get deleted. Node revisions aren't supposed to share the same revision of a field collection item, the update should have made a separate copy of the field collection item for each revision.

I think it's fine to separate that from this destructive problem that happens in the current version from a clean install. It still needs to check for the case of the final revision of a field collection item being deleted though and on a related note I don't think the change that was made to the test is correct.

Yo JMuzz,

I took a look at your patch #21 and totally understand the problem now that we are facing, and I can totally see how these issues are related.

I spent some time refactoring it today so it worked for my base use cases, and I popped it in a module that is attached. This is definitely a work in progress, but I just wanted to let you know that my goal is to get that included in this patch. Hopefully tomorrow if all my tests go well.

If you have time today, I would very much appreciate your initial review of the logic I refactored in patch #21 or any advice / tips that you may have.

Thanks,
James

It's a good start. As far as the inner fields go they should all share the same revision id for a single revision of a field collection item. The field collection item itself should only need to be written once per revision, not for every field it contains.

When it works it looks like this will create a new revision for each time the original revision was referenced after the first time, which may be sufficient. It also may need to:

• Create a new revision to replace each of the revisions that were created after revisions were activated to maintain sequential revision numbers.
• Handle nested field collections. (May already work)

Great, glad to know I'm on the right path.

Do sequential revision numbers matter much? I get the number that is assigned from field_collections_item_revision. It is only just a reference, yah? Right now the code goes something like this:

7.x-1.0-beta4: 3 revisions created: 1, 1, 1
7.x-1.0-beta?? ( After revisioning supported ): 3 new revisions created: 4, 3, 2, 1, 1, 1
7.x-1.x-dev ( this patch we're making ): 4, 3, 2, 1, 5, 6
3 New revisions created9, 8, 7, 4, 3, 2, 1, 5, 6

Are you saying that may be it should be like this:

7.x-1.0-beta4: 3 revisions created: 1, 1, 1
7.x-1.0-beta?? ( After revisioning supported ): 3 new revisions created: 4, 3, 2, 1, 1, 1
7.x-1.x-dev ( this patch we're making ): 6, 5, 4, 3, 2, 1
3 New revisions created9, 8, 7, 6, 5, 4, 3, 2, 1

That's going to be very difficult to do and potentially unnecessary since they are just references. Would love to hear your feedback.

I think what you have as 9, 8, 7, 4, 3, 2, 1, 5, 6 is fine, the revision numbers are still sequential, just not in the order, but there is nothing in core that dictates the order.

just wanted to confirm this bug. deleted revision in the db and now all field collections are gone. using latest beta7
patched with #9
emptied revision table manually (because deleting the fields dintnt).
all fc data is gone. so it didnt work for me here.
rechecking, did something wrong. had a multisite with 2 module versions.

did not work. emptyiing the revision table results in all data not showing up and
used #9

Notice: Trying to get property of non-object in field_collection_field_get_entity() 

@marcoka Yes this still happens in the latest version. #38 should stop the data loss.

The concern I have about the revisions getting out of order is people may be relying on queries that do things like... "For every revision after this one" or "Do something on the revision before this one." Not only would it break these queries, but what could replace them if the revision number couldn't be relied on to indicate the order of the revisions? For example, I am pretty sure that is how the list at /node/%/revision is ordered.

I don't think it would be necessary to reuse the revision numbers already in place. It should be just as good and easier to reassign the revisions that were made after revisions were activated to new numbers.

7.x-1.0-beta4: 3 revisions created: 1, 1, 1
7.x-1.0-beta?? ( After revisioning supported ): 3 new revisions created: 4, 3, 2, 1, 1, 1
7.x-1.x-dev ( this patch we're making ): 9 (was 4), 8 (was 3), 7 (was 2), 6, 5, 1
3 New revisions created: 12, 11, 10, 9 (was 4), 8 (was 3), 7 (was 2), 6, 5, 1

#38 is no patch, just your commenst about test. do you mean #36?

gut latest dev version september 11, applied #36, truncated "field_collection_item_revision". did drush cc all. visited node with fc. all data not showing up, everything is empty.

@jmuzz, totally understand your point about reordering everything if there are SQL commands that depend on that ordering. However, the biggest downside I see is that we are modifying existing working revisions also might have some dependencies somewhere else that these small SQL aren't referencing. By only fixing the "broken" ones we limit the risk of causing more damage than harm.

Both have their trade offs.

For now I'm going to ignore reordering for the sake of moving forwards, and after I finish the patch, we should evaluate.

i now did the following.

1.) i deleted the filed items in field_collection_items
2.) i ran a query that deletes revisions whick have not field collection items

This query kills all entrys in field_collection_item_revision that do not have an entry in field_collection_item. so basically it deletes orphaned revisions.

DELETE FROM field_collection_item_revision WHERE (SELECT count(1) FROM field_collection_item WHERE item_id = field_collection_item_revision.item_id) < 1

@marcoka, we are actively working on this issue, the temporary fix is in comment #36. Apply that patch to the dev version and see if your problem still exists, that extra testing would be very validating! Feel free to read through all the comments and see our progress and jump in if you find it applicable.

i did that :) patch didnt work (meaning same error) see #47

in #47, what do you mean by 'truncated "field_collection_item_revision"' ? It sounds like you just deleted it via SQL or did you do it via the U/I?

i had a content type that used a field collection. i deleted it, then i deleted all nodes of that content type. the problem then was that all filed collection items and revisions (the tables) were still in the DB. so yes, i deleted the tables manually as i had no other choice.

@jmuzz

Just a progress update, I have updated that revision migration test module which now includes nesting of field collections. I still need to stress test it out with field collections with multiple fields and with an arbitrary amount of nesting. If you have a chance to take a look at my work in progress and throw out any feedback before I get to it Monday, I'd much appreciate it!

I feel like this work may have to be in its own ticket, so if you feel ever so behooved to do that, please by all means.

I tested #54 on a super nested field collection with multiple deltas and it failed, and honestly is getting to the point where I'm not sure my approach is the right direction anymore. With that said, I'm spinning this off to its own issue... #2339023: Migrate field collection revisions supported in beta4 to be up to date

@marcoka, I tested out what you said and basically when a revision is deleted ( which is what this issue pertains to ), the field definitely gets deleted. However, as you mentioned, when deleting the content type, field_collection_item and field_collection_item_revision are not erased, which IMO, is a separate issue. I'm going to recommend to you file another bug with that specifically to prevent this issue from getting off track from focusing on supporting deletion of revisions of existing content types.

i already fixed my issues manually here but i created an issue: #2339211: Deleting contenttype doesnt delete field collections

@jmuzz, per our discussion in comments #38-#40, I have:

• Removed whitespace from the patch
• Restored the original test cases in question because I made another edit in the patch
• Created a new issue for migrating old broken field collections here #2339023: Migrate field collection revisions supported in beta4 to be up to date

I'm going to put this back into "Needs Review" because I believe the fixes now meet the immediate need of supporting revision deletion for the current iteration of field collections.

Oh yes and the patch attached..

I´m applied patch # 60 to the Dev version and nothing at continuing the proble edit or publish content with fieldcollection deleted or does not record anything

my collections of text fields are only one taxonomy files

RTBC, nice work! :)

@Draven3166, I'm confused, are you saying the patch works or does not work? And for taxonomies.. are you using the taxonomy_revision? I tested with taxonomy_revision and patch #60 does work.

@Fabianx, thanks.. AGAIN! =)

check the latest dev version you uploaded and checked eh it's working correctly, if you have something new notice. Good work thanks @mcpuddin

Great! Now if we can just get @jmuzz back in here to review =)

+1 RTBC for me. Thank you.

Thanks for the patch and explanations.

There are still issues that may arise if Field Collections was updated from an older version, but that is now being handled in #2339023: Migrate field collection revisions supported in beta4 to be up to date .

You rock @jmuzz!

I also need to post an issue related to that, that I never gotten to do, yet.

Thanks so much mcpuddin!