Currently, the Field Collection module does not support revisions. To add this support, we need to create a separate revision of each Field Collection item for each revision of the entity host.

Dependencies

Adding full revision support for the Field Collection module requires the Entity API to support revisions:

#996696: Support revisions in Entity API

It has been committed.

History

The initial approach to fixing this issue was to create a simple patch that creates a new Field Collection item when the host entity creates a revision. This patch was discussed and it was decided that the best long term approach is to have "proper" revisions for Field Collection items which requires the Entity API to support revisions. The issue was postponed for a time so that the Entity API module could be updated to support revisions. Once a patch for Entity API revisions became available, a patch was started to create Field Collection item revisions when the host entity creates a revision. This patch is being actively worked on.

Current Status

Per above, there are two patch methods available. Note: if you need a very quick stop-gap method to work with nested Field Collection items, check out comment #90.

Patch method 1

A simple patch that creates a new Field Collection item each time the host entity creates a revision: #37.

Pros:

  • Simple change
  • Appears to work as designed
  • Can be a stop-gap measure until full-revision patch is available

Cons:

  • Not scalable so not advised for content with lots of revisions
  • Not the best-practice approach for supporting revisions
  • Does not work with nested Field Collection items

Patch method 2

A complex patch that creates a new Field Collection item revision each time the host entity creates a revision. This patch is being actively worked on, so check the latest comments for the most recent version.

Pros:

  • Scalable
  • Best-practice approach for supporting revisions

Cons:

  • Complex patch
  • Will require a lot of review and testing
  • Requires vetting from maintainers or others familiar with Field Collection, Entity API, and Field API codebase

How to test

  1. IMPORTANT! Use a test database / site.
  2. Installation:
  3. Setup
    • Add an embedded Field Collection (FC) field to a content type (e.g. Test field for article content type)
    • Add a text field to the FC (e.g. Test Text field)
    • Create a node of the relevant type
    • Fill in the node Title, Body, and Test Text field
    • Save the node
  4. No new revision
    • Edit the node and change the Test Text field text to "TEST ONE"
    • Do not check "Create new revision" and save the node
    • Published node should show "TEST ONE"
  5. New revision
    • Edit the node and change the Test Text field text to "TEST TWO"
    • Check "Create new revision" and save the node
    • Published node should show "TEST TWO"
    • View the previous revision - it should have the Test Text: "TEST ONE"
  6. Changing revisions
    • Revert back to the previous revision
    • Published node should show "TEST ONE"
    • View the 2nd revision - it should have the Test Text: "TEST TWO"
  7. Edit/Delete/Add links
    • TODO: Testing instructions need to be added for these links

Testing with Workbench Moderation

  1. Installation
  2. Setup
    • Make sure that your user has permission to use Workbench and Workbench Moderation
    • Default publishing workflow states are sufficient (e.g Draft, Needs Review, and Published)
    • Configure content type to use moderation
    • Create a node of the relevant type
    • Fill in the node Title, Body, and Test Text field
    • Save the node
  3. Create Draft
    • Create a new Draft
    • Change the Test Text field text to "WB TEST ONE"
    • Draft node should show "WB TEST ONE"
    • Published node should show original text
  4. Publish Draft
    • Publish Draft
    • Published node should show "WB TEST ONE"
    • View the previous revision - it should have the original Test Text
  5. Revert versions
    • Go to the moderation list
    • Revert to original node
    • Should create new draft
    • Publish draft
    • Published node should show original text
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BenK’s picture

Subscribing

yched’s picture

"we just create a separate revision for each revision of the host" seems to make sense - that's more or less what would happen if the fields were directly attached to the host entity ?

servantleader’s picture

Subscribing
Definitely needs "separate revision for each revision of the host". Anything else would be confusing to the user. The user sees it as one entity.

robmalon’s picture

Subscribing

cfennell’s picture

subscribing and will test/report back once this feature is in place.

John Pitcairn’s picture

Subscribe. Just a separate revision for each revision of the host, I think.

jtbayly’s picture

subscribe

ogi’s picture

subscribe

cfennell’s picture

After giving it some thought, I wonder if the current UI might not somewhat complicate the objective of revisions for field collections. That is to say, if you peg revision ids to node revisions, you then have to retroactively update revision ids for every collection item after a node/parent entity has been updated, including things like collections of collections of collections of.... Wouldn't it be a little easier to update a bunch of collection items associated with the parent entity if they were subform widgets (#977890: Display collection fields within entity form (subform/embedded)) and therefore available in one big submit array (not easy, but easier)?

The other thing to keep in mind is that by separating the field collections off into their own page, we would, for many users like me, build the expectation that a new revision would for the entire node would take place and all other field collection items would be updated to this revision. This problem goes away with a subform widget as well and is in line with what most users are used to.

Anyway, I'm still interested in working on this problem and could definitely use some guidance. I guess I'm thinking it makes more sense for us to tackle this problem if we can get subform support into this module as the default UI.

Does this make sense to you?

RobW’s picture

I understand where you're coming from, but I think that the subform widget is fast becoming the default interface for field collections, and should probably be treated as such. Early dev screenshots showed a subform, and I believe the node page edit interface was introduced because of a code conflict in core that's now been fixed - point is, the primary interface was always meant to be a subform and now, although alpha, is on its way to getting there. It makes sense to me to code for what over the lifecycle of D7 will be the primary use case, i.e. subform/revisions per node revision.

stevector’s picture

Status: Active » Needs work
FileSize
879 bytes

This patch makes a new Field Collection when the parent form is creating a new revision. I surprised myself with how little code I needed to make that work. This allows node revisions to hang on to different Field Collections. So when a node revisions are traversed, the Field Collections change as an end user would expect.

I'm marking this as 'needs work' because the Field Collections themselves are not revisioning and they could. Also this patch assumes that Field Collections are going on nodes. I think there are efforts out there to make other entities revisionable. Any effort here should probably accommodate at least user revision which I haven't tested yet.

I think the ideal solution would have the new parent/node revision keep the reference the same Field Collection Entity ID. And the Field Collection Entity itself would also create a new real revision. This would allow for following revisions within a Field Collection instance.

Bevan’s picture

Category: task » feature
Status: Needs work » Needs review

While this still needs work, it needs review to move forward with that work.

valderama’s picture

subscribing

yched’s picture

Status: Needs review » Needs work

This cannot be fixed in the form submission workflow, we also want this to happen on programmatic node_save().
I guess field_collection_field_presave() would be the correct place.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

I have attached a patch the sets the item_id to null in "field_collection_field_presave" if a new host entity revision is being created.

I also created a field setting that lets the admin choose whether to create new Field Collection Item per field instance. I thought there might be special cases where you DON'T want a new Field Collection Item with each host entity revision. The default value is to create a new Item for every revision.

Not sure the if I used the best way to test if an entity supports revisions. I used:

  $entity_info = entity_get_info($instance['entity_type']);
  //check to see if the host entity supports revisions and using the embed form
  if (!empty($entity_info['revision table'])

If works but thought there might be better way.

tedbow’s picture

My last patch had a logical error in the statement when testing whether to create a new Item.

I have fixed and updated it.

AtomicTangerine’s picture

sub

droath’s picture

Subscribing

khanz’s picture

subscribing

kalabro’s picture

Subscribing

Status: Needs review » Needs work

The last submitted patch, field_collections_revisions_in_field_presave-1031010-16.patch, failed testing.

John Pitcairn’s picture

I think the ideal solution would have the new parent/node revision keep the reference the same Field Collection Entity ID. And the Field Collection Entity itself would also create a new real revision. This would allow for following revisions within a Field Collection instance.

I think ideally the Field Collection should create a revision when any field within it changes, not every time the parent changes.

And the parent form should reference the appropriate Field Collection revision, not the FC Entity ID. Then if the user rolls back the parent to an earlier revision, the appropriate Field Collection revision will also be used.

John Pitcairn’s picture

Patch at #16 works for me against 7.x-1.0-beta2. Patch applied from module root folder.

John Pitcairn’s picture

#10: I think that in order to support a node-revisioned workflow, using the embedded subform widget becomes a requirement.

If the hidden widget is used, and the user adds/edits/deletes a field collection from the parent node view, then any node-revision workflow fields are not presented to the user when Field Collection programmatically creates a new revision of the parent node. For example, the user will never be able to add a log entry for the new parent node revision, and that's in core. Contrib revisioning workflow modules may present additional fields of this nature.

That said, I prefer the way the hidden widget works for larger field collections, where it avoids considerable clutter on the parent node form. But I would rather see that UI on the parent node form, not on the parent node view. Then after editing the field collection, the user is returned to the node form, where they can still leave a log message etc. This could be especially effective if the field collection edit form is presented in a modal overlay.

I've filed a separate feature request - see #1374138: Provide option to use "hidden" widget UI from parent form

This would however require that the field collection does not save a new revision for the parent node, since the user will do that when they save the parent. Perhaps that behaviour can be exposed for modification, either as a settings checkbox, or a hook.

kadimi’s picture

#16 worked for me in beta 2

Zen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, field_collections_revisions_in_field_presave-1031010-16.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

I have re-rolled my previous patch against the latest dev branch.

tedbow’s picture

I have re-rolled my previous patch against the latest dev branch.

tedbow’s picture

Sorry don't know what happened there. I re-attaching patch re-named to follow guidelines here: http://drupal.org/node/707484

Don't know if incorrect name can cause automated testing to fail

tedbow’s picture

The patch I made above doesn't make field collections support revisions themselves but creates a new field collection item if the parent host entity is going have a new revision in the save.

This works when saving the host entity and uses hook_field_presave so it works when saving the host entity programmatically or through web edit form as long as field collection is using the embedded form.

It does not work if you have editing the field collection by itself in the hidden method with the "edit" link.

Question: Since my method doesn't actually create a revision for the field collection itself is ultimately not the solution we are looking for? i.e. should the patch be scrapped and start over with something more comprehensive.

doana’s picture

This issue affects me as well. Thanks to everyone who's working on this!

Fidelix’s picture

Question: Since my method doesn't actually create a revision for the field collection itself is ultimately not the solution we are looking for? i.e. should the patch be scrapped and start over with something more comprehensive.

I think revisions for field_collection make much more sense if they work when editing them individually (like in the hidden method with the edit link).

There are a lot of use cases for that.

wmostrey’s picture

Title: Support revisions » Support revisions for field collections
Status: Needs review » Reviewed & tested by the community

The patch in #30 applies cleanly and works as advertised. Personally I would like to see it committed already. It adds the much needed functionality to have field collections for each node revision instead of them being the same for a node, regardless of revisions.

Once this patch is in, we can use it as a base to get actual revisions for individual field collections.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

The patch makes sense, but there are some small bits to clean up. Feel free to set back to RTBC after the reroll.

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+      //check to see if a new collection_item should be created

Space after //, start with capital letter, end with a full stop

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+        if (!isset($instance['settings']['new_revision']) || $instance['settings']['new_revision'] == 1) {

I would change this to !isset() || !empty(), no reason to use 1

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+function field_collection_field_instance_settings_form($field, $instance) {

The function should start with $form = array();

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+  //check to see if the host entity supports revisions and using the embed form

Space after //, start with capital letter, end with a full stop

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+    $settings = $instance['settings'];

No reason to simplify this variable, just use $instance['settings']['new_revision'] later on like the above function

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+      '#default_value' => isset($settings['new_revision']) ? $settings['new_revision'] : '1',

Don't use '1', use TRUE

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+      '#description' => t('If checked a new field collection item will be created when creating a new revision of it\'s host.'),

comma after 'checked', use double quotes to avoid escaping the apostrophe

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+    return $form;

Move this out of the conditional, the hook expects something to be returned.

+++ b/field_collection.moduleundefined
@@ -609,12 +609,34 @@ function field_collection_field_presave($entity_type, $entity, $field, $instance
+}
 /**

Missing a blank line

HyperGlide’s picture

Wishing I knew more PHP to help put #35 into practice. Really looking for this feature ;-)

tedbow’s picture

Attached a patch that fixes issues @tim.plunkett documented in #35

tedbow’s picture

Status: Needs work » Needs review

marking for review

tedbow’s picture

marking for review

HyperGlide’s picture

@tedbow Thank you!

tedbow’s picture

Assigned: Unassigned » tedbow
Status: Needs review » Needs work

Ok I got to talk to @fago at Drupalcon this week and pick his brain about revisions.

My method above will not work properly but I willing to work on the changes necessary.

From my understanding of our conversations here are is what will be necessary for revisions.

  1. Field Collections themselves will need to have a revisions table. Here's why.
    • If we don't have revisions supported by Field Collections themselves the data for fields they contain will always be in the field_data_[field_name] tables and never move into field_revision_[field_name] tables. This would cause performances problems because the field_data_table_* would get to large.
  2. Field Collections should respect the revisions of their parent entity.
    • When you create a new revision of parent a new revision of the field collections will be created.
    • Field Collections will NEVER have revisions of themselves created independent of parent revision. This would get too complicated and get away from the purpose field_collections being dependent field entities. To have entities that are their own revisions there are many other modules that can used such as entity reference and relation
    • When saving a field collection in the embedded form the field collection will create a new revision when the parent does(also should work programmatically saving the parent entity.)
    • The hidden form is more difficult. For nodes we can check if the parent entity's bundle defaults to a new revision and make a new revision of the parent and the field collection. There isn't currently a generic way to determine if an entity's bundle defaults to creating a new revision. Ideas??

I think this is the basic idea. I will look into the the steps necessary for this. @fago, feel to correct me if I miss-represented any of your description of how revisions should work.

This work may have to wait until #996696: Support revisions in Entity API is finished and then we can work on the dev version of Entity API that has the support for revisions.

HyperGlide’s picture

@tedbow Thank for the hard work and detailed follow up.
Thank you also goes to @fago for his support in the Drupal community.

I have another ticket to follow now :-) and if I can be of help in testing let me know.

Our use here is a bit robust as we have a content type with field_collections that is moderated with workbench_moderation module.

liquidcms’s picture

#37 works for me.. and collection with workbench as well as a use case.

let's get this committed.. :)

HyperGlide’s picture

@liquidcms -- Did you read #41 -- Suspect #37 will not get committed and other solutions are in the thought process.

tedbow’s picture

Status: Needs work » Postponed

@HyperGlide @ liquidcms yes #37 is not going to work b/c of comments in #41. I still think that it would be best to wait on #996696: Support revisions in Entity API. If I miss that issue being resolved someone please contact me via my contact form when it does and will get back on this issue. Right now it seems they are still figuring it out.

Setting to status to "postponed" to reflect the fact that we waiting on another issue.

liquidcms’s picture

@tedbox: thanks for the heads up.. it does seem to be working at the moment and i don't care about performance due to large table size (in current project). i don't see in #41 any mention of what actually won't work (i.e. use case that breaks this); but will do more testing.

thanks for the patch and the warning.

tedbow’s picture

@liquidcms, #37 will work for now(though not tested much) but once a different method is put in that deals with #41 it might not.

liquidcms’s picture

understood... but no choice.. launching with Workbench.

will be interesting to see how mix with this method and future fix will work out; suspect if nothing in Draft at the time it should be ok.

crimsondryad’s picture

I'm definitely worried about performance. We've got a high traffic site and our designer / dev LOVES this module...which means he uses it everywhere.

limptimtim’s picture

Status: Postponed » Needs review
tim.plunkett’s picture

Status: Needs review » Postponed
Kristen Pol’s picture

I am very sadly just discovering that revisions aren't supported after using FC in a site that we just pushed live :/ (Yes, I know that we should have tested it better to catch this before go-live but we were under a ridiculously tight deadline. And, that wouldn't have helped with the issue of having to rework the site.)

It would be good to put a *big*, *bold* WARNING on the project page. Please... save someone else the headache I will be going through reworking my site this weekend/week. Most people use revisions.

Perhaps something up at the very top like:

<strong>WARNING</strong>
<a href="/node/1031010">Field Collection does not currently support revisions</a> and this issue won't be addressed until the <a href="/node/996696">Entity API supports revisions</a>. If you want to use <a href="/project/workbench_moderation">Workbench Moderation</a> or other revisioning methods with Field Collection, follow these issues and help out with patch testing, reviews, and rewrites. This warning will be removed once an official version Field Collection supports revisions.

I love this module and I'm very sad I will have to not use it until this issue is resolved.

Kristen Pol’s picture

I'm going to investigate this today and see if I can create a patch for FC based on the latest Entity API revisions patch:

http://drupal.org/node/996696#comment-6043122

Wish me luck...

Kristen Pol’s picture

[comment deleted] Nevermind... dumb comment ("git diff" was showing me the Entity API changes not the Field Collection changes)... need some coffee I guess.

Kristen Pol’s picture

FileSize
5.06 KB

Well, I've started but it needs a lot more work. So far I've done:

  1. Updated install file to:
    • create new revision_id column
    • create new field_collection_item_revision table
    • update these with existing data from field_collection_item
  2. Updated module file to create new revision *if* the host entity is set to create a revision

The code seems to work in that it creates the column and the table and prepopulates them. And, the FC item revisions are created if the host entity is set for a revision (and revisions aren't created otherwise).

What needs to be figured out is how to get the FC item revisions synced up with the host entity revisions.

Note, you need to apply the Field API patch from #53 as well as this one in order to get it to work.

Kristen Pol’s picture

Status: Postponed » Needs work
Kristen Pol’s picture

FileSize
5.31 KB

Last patch was mangled. This one should work.

Kristen Pol’s picture

Since both item_id and revision_id need to be tracked, one way I see how to deal with the mapping between host entity revision and FC item revision would be to:

  1. Change the FC item "value" to a string
  2. Store a key like "5|67" as the value where the first number is the item_id and the second number is the revision_id

That should be sufficient to keep everything in sync. I could update the code to do this but want to see if there are any other ideas for handling this before I take the time to do so.

Thanks!

tedbow’s picture

@Kristen Pol,
Thanks for getting this started.
RE:

Change the FC item "value" to a string
Store a key like "5|67" as the value where the first number is the item_id and the second number is the revision_id

I think you are talking about the "value" column on the "field_data_field_[FIELD_NAME]" tables, right?
I think we should be able to add another column to the table. I forget where you override this but I will try to find out. Maybe we could use 2 columns like

  1. Field_[FIELD_NAME]_item_id
  2. Field_[FIELD_NAME]_revision_id

We also could use these 2 columns also on the "field_revision_field_[FIELD_NAME]"

UPDATE:
Looks like you would set up the definition of the field in hook_field_schema
You can look at image_field_schema for an example of how to use multiple columns.
But I am not sure how you would actually update the existing field table.

Does anybody know how we would go about adding and extra column,Field_[FIELD_NAME]_revision_id to exisiting field_data tables?

Kristen Pol’s picture

Thanks, @tedbow, for the quick response. That would obvious be better! I haven't written anything for the Field API so I was assuming "value" was the way it needed to be stored. I'll take a look at how those columns work as well.

tim.plunkett’s picture

I agree with tedbow.

Kristen Pol’s picture

I see where it's defined now... in the install file: field_collection_field_schema

Ok, I will see if I can figure out how to get everything updated using the item_id and revision_id in the field_data*/field_revision* tables.

I will try to get something out tonight!

Kristen Pol’s picture

FileSize
17.08 KB

Here is a first pass at the complete patch. I have tested the following:

  1. Revisions are not created when host entity (node) has "create new revision" unchecked (tested without Workbench Moderation).
  2. Revisions are created when host entity (node) has "create new revision" checked (tested without Workbench Moderation).
  3. Viewing different revisions works as expected.
  4. Current and draft revisions work as expected with Workbench Moderation.
  5. Changing the current revision works as expected in Workbench Moderation.

Issues:

  1. Doesn't work with diff / view changes.
  2. Get a warning when saving in some situations:
    Notice: Undefined property: FieldCollectionItemEntity::$revision_id in drupal_write_record() (line 6969 of /var/www/xxx/includes/common.inc).
    

I have not tested the patch without embedding (i.e. all my Field Collection fields are embedded in their host entity).

I also have not added additional tests for revisions.

Please review and test and provide feedback. Thanks!

[NOTE: MAKE SURE TO ONLY TEST ON A TEST DATABASE. THIS PATCH ALTERS THE DATA STRUCTURE SIGNIFICANTLY AND YOU WILL NOT BE ABLE TO RECOVER YOUR OLD STRUCTURE WITHOUT WRITING A LOT OF CODE.]

The last submitted patch, fc-revisions-all.patch, failed testing.

Kristen Pol’s picture

FileSize
16.59 KB

Trying again...

[NOTE: MAKE SURE TO ONLY TEST ON A TEST DATABASE. THIS PATCH ALTERS THE DATA STRUCTURE SIGNIFICANTLY AND YOU WILL NOT BE ABLE TO RECOVER YOUR OLD STRUCTURE WITHOUT WRITING A LOT OF CODE.]

The last submitted patch, fc-revisions-all.patch, failed testing.

Kristen Pol’s picture

FileSize
16.4 KB

And 3rd time is charm?

[NOTE: MAKE SURE TO ONLY TEST ON A TEST DATABASE. THIS PATCH ALTERS THE DATA STRUCTURE SIGNIFICANTLY AND YOU WILL NOT BE ABLE TO RECOVER YOUR OLD STRUCTURE WITHOUT WRITING A LOT OF CODE.]

The last submitted patch, fc-revisions-all.patch, failed testing.

Kristen Pol’s picture

Well, it got further and failed on the automated tests. I'm not seeing which tests it's failing. Can anyone enlighten me on how to see that?

NOTE: The patch can still be tested and reviewed even though it obviously needs some fixes.

[NOTE: MAKE SURE TO ONLY TEST ON A TEST DATABASE. THIS PATCH ALTERS THE DATA STRUCTURE SIGNIFICANTLY AND YOU WILL NOT BE ABLE TO RECOVER YOUR OLD STRUCTURE WITHOUT WRITING A LOT OF CODE.]

tedbow’s picture

Status: Needs work » Needs review

@Kristen Pol, "View Details" link under operations in #67. Then you should see the non-pass test is "Field collection (FieldCollectionBasicTestCase)" click that to expanded the individual failed test names.

Sorry, I have couple work projects that I am working on this week that are keeping me busy or I would help more.

Kristen Pol’s picture

When I click it, it doesn't open up... must be a browser issue. I'll try a different browser.

[UPDATE] Works on Chrome (not on Firefox) on Linux. I'll see if I can figure out why those tests aren't working today.

tim.plunkett’s picture

Assigned: tedbow » Unassigned
Status: Needs review » Needs work
FileSize
18.08 KB

This should past tests, but we can't break the schema like this. I fixed the tests to use 'item_id' and not 'value', but...

+++ b/field_collection.installundefined
@@ -27,18 +32,43 @@ function field_collection_schema() {
-    'value' => array(
+    'item_id' => array(
       'type' => 'int',

This one should just stay as 'value'. All of the docs say that, and it would break a lot of custom code. 'value' and 'revision_id' aren't ideal, but its what we have to do.

tim.plunkett’s picture

Status: Needs work » Needs review

Er, status-fail.

Status: Needs review » Needs work

The last submitted patch, field_collection-1031010-72.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
19.57 KB

Well that was the wrong patch, heh.

Status: Needs review » Needs work

The last submitted patch, field_collection-1031010-75.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

Oh that was wrong/worse.

Kristen Pol’s picture

FileSize
44.15 KB

@tim.plunkett - Thanks for the review! I'm fine with leaving it "value" and "revision_id" if that is what makes sense. I ran the tests locally for my patch and all tests passed (see attached).

I'm happy to help out with the patch you've modified. I'll start with testing it locally. If you have any other tasks for me, let me know as I will make myself available today for this.

[UPDATE] I'm re-reading your comment and am unclear how the schema stays as "value" yet the rest of the code gets changed to "item_id". Can you point to the docs on this? Meanwhile, I'll google and see what I can find.

Kristen Pol’s picture

@tim.plunkett - I didn't find any docs, but If I am understanding correctly:

1) 'value' must stay in hook_schema

2) 'value' can be changed in hook_update_N so 'value' => 'item_id' and 'item_id' is used throughout code

3) this works because:

a) New installations will use original schema and then run all the hook_update_N functions so the schema will be updated as expected. If hook_schema is changed to use 'item_id', then there will an error when it tries to change from 'value' to 'item_id' in hook_update_N because 'value' won't exist.

b) Existing installations will run the hook_update_N functions that are appropriate based on their current version. If they update to include this field/column change, the field/column will be modified from 'value' to 'item_id'.

One way I see around the issue with (3)(a), is to do a check before changing the field/column, e.g. something like:

if (db_field_exists($table, 'value')) {
// code here to change from 'value' to 'item_id'
}

But, you mention there might be issues with existing *custom* code. I'm not sure how keeping 'value' in hook_schema helps them when they upgrade the module as the field/column will be changed via the hook_update_N.

Kristen Pol’s picture

@tim.plunkett - Regarding patch #75, I think some of those 'value' strings in the test file are supposed to stay because they are for text strings, not the 'value' for the FC items. I had gone through the test file in #67 and changed certain 'value' parameters to 'item_id' but only where it looked like it was the FC item 'value'. If it was assigning a text string to the 'value', then I didn't change it. I don't know anything about SimpleTest code, but perhaps they are creating a text field for "inside" the FC item and that is the 'value' that is the string being set (e.g. 'foo').

Perhaps whoever wrote the SimpleTest code could review?

Kristen Pol’s picture

Issue summary: View changes

put reason for postponing

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

@Kristen Pol, ignore both patches I uploaded, they were shortsighted and changed the wrong things.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

@tim.plunkett - No worries! Any idea why SimpleTest is passing locally for me but failing on d.o?

Kristen Pol’s picture

Ignore this patch. This is just to test the SimpleTest tests to make sure they pass with a "nothing" patch (added blank line).

Kristen Pol’s picture

I think the issue with the patch is that there is a "path" defined. I added the revision_id to this path, but the path is used all over the place and none of those places have been updated with the revision_id. I'll review further and see if I can understand how to change them.

Kristen Pol’s picture

Status: Needs review » Needs work

I'm creating a new patch that updates the various menu items/paths (e.g. view/edit/delete). Hope to get it up soon.

Kristen Pol’s picture

Status: Needs work » Needs review
FileSize
19.92 KB

Here's an updated patch with changes to deal with view/edit/delete links and the tests have a few revision_id checks in them.

There are a lot of changes and I am still getting some errors in some cases (like mentioned in #63) so this needs a lot of review and testing.

This patch passed tests on my local machine.

At some point soon, I will likely need to hand this over to @fago or someone who's better acquainted with the code as the code is very complex.

Status: Needs review » Needs work

The last submitted patch, field_collection-1031010-86.patch, failed testing.

Kristen Pol’s picture

Status: Needs work » Needs review

It appears that the testbot might not be using the patched code to do the tests and is using the original version instead. In the test log, it complains about:

Reference correctly deleted. - line 73
Added field value is shown. - line 146
Failed to set field field_text[und][0][value] to ilF10xdm Other - line 149
etc.

yet these don't match the lines in the patched *.test file. Also, there is a reference to:

Found the requested form fields at field-collection/field-test-collection/1/delete

Yet, all references to "field-collection/field-test-collection/1/delete" have been changed to "field-collection/field-test-collection/1/1/delete" in the tests.

Is the testbot not using the patched tests? If so, what can be done to get to use the patched tests?

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

I was contacted by Ricochet Consulting and they may be able to work on this patch this week, so I updated the issue summary to help newcomers to this issue.

Kristen Pol’s picture

For those who need a quick *hack* and are using Field Collections inside of Field Collections, like I am, you should be able to add the following to your field_collection.module code:

       // Determine if revision should be created.
       if (isset($entity->revision) && $entity->revision) {
 
         // Save revision flag for sub-items.
         $item['entity']->revision = $entity->revision;
 
         // Clear item_id so new item will be created.
         $item['entity']->item_id = NULL;
       }

in the field_collection_field_presave function, right before calling:

  $item['entity']->save(TRUE);

I don't want to create a patch for this as this is just a stop-gap measure until the real patch is available.

If you choose to use this code, please make sure to test on a *development* or *test* server and back up your database and module code base first. Then, test thoroughly to make sure it is behaving as desired before using the code in production.

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

wizonesolutions’s picture

Kristen: http://drupal.org/project/testbot kind of details why they might be failing generally. As for the specific case of the patched test not being used, trying to find out.

wizonesolutions’s picture

Tests are failing locally after applying #73 from #996696: Support revisions in Entity API and #86, so I don't think it's a testbot issue.

drush test-run FieldCollectionBasicTestCase
Field collection 70 passes, 9 fails, 0 exceptions, and 22 debug messages                                                                                                                                                         [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Failed to set field field_text[und][0][value] to puO9IcD0                                                                                                               [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Found the Save button                                                                                                                                                   [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Found the requested form fields at field-collection/field-test-collection/1/edit                                                                                        [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Field collection saved.                                                                                                                                                 [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Field collection has been edited.                                                                                                                                       [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Field collection can be viewed.                                                                                                                                         [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Found the Delete button                                                                                                                                                 [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Found the requested form fields at field-collection/field-test-collection/1/delete                                                                                      [error]
Test FieldCollectionBasicTestCase->testBasicUI() failed: Field collection item has been deleted.                                                                                                                                 [error]
Removed 73 leftover tables.                                                                                                                                                                                                      [status]
Removed 1 temporary directory.                                                                                                                                                                                                   [status]
Removed 2 test results.                                                                                                                                                                                                          [status]

Note: I didn't pass --uri because I have $options['uri'] = 'example.com'; in sites/all/drush/drushrc.php.

Kristen Pol’s picture

You're right! Argh. <facepalm> I hadn't created a patch from the most recent changes :/ Ok, let's try this again.

Status: Needs review » Needs work

The last submitted patch, field_collection-revisions-1031010-93.patch, failed testing.

wizonesolutions’s picture

Status: Needs work » Needs review
FileSize
22.98 KB

This is a version of the above patch that is fixed up to apply.

Status: Needs review » Needs work

The last submitted patch, field_collection-revisions-1031010-94.patch, failed testing.

wizonesolutions’s picture

Ack, this one *does* pass locally but fails here. Looking into why.

wizonesolutions’s picture

It's because we rely on the Entity API patch - unless I include that in the contents of the patch, it'll fail.

Once we have a functional solution that passes tests locally we can do that if the Entity API patch hasn't landed. I'm going to try to get that to land first now.

Kristen Pol’s picture

Ah! Of course... makes sense. Ok, then we all need to get the Entity API revisions patch committed ASAP! :)

Kristen Pol’s picture

Issue summary: View changes

Updated issue summary.

Kristen Pol’s picture

Issue summary: View changes

Added a note about new comment with stop-gap measure

wizonesolutions’s picture

Assigned: Unassigned » wizonesolutions

Assigning to myself for the moment.

wizonesolutions’s picture

Assigned: wizonesolutions » Unassigned

Tested this. Functionally, it seems sound, so just need to figure out the implications of the TODO: FIXME parts and address them.

wizonesolutions’s picture

Status: Needs work » Needs review
FileSize
22.78 KB

OK, I've removed the TODO: FIXME notes and changed value to item_id in the getter callback.

As far as I can tell, this patch is now sufficiently architecturally sound to be reviewed by a larger audience. Please give it a shot and report any issues. I will be trying this against some more realistic use cases now as well.

Status: Needs review » Needs work

The last submitted patch, field_collection-1031010-revisions-102.patch, failed testing.

wizonesolutions’s picture

A couple points I have noticed about the approach, but I would rather have the existing work reviewed before actioning them:

  1. I think we may need to implement hook_field_attach_delete() though (that is what node_revision_delete calls, for example) and clean up the field_collection_item_revision table there. A separate function to convert the items to revision IDs may be most appropriate.
  2. Another thing to figure out is what happens when a field_collection_item bundle is deleted manually from the node (using the in-place links). Given the URL of said bundle, which contains the revision ID, I think this should only delete that revision of field_collection_item, i.e. the one on that revision of the node, not the entire bundle. Same with editing. Right now it just removes it wholesale.
klonos’s picture

@wizonesolutions: Hey Kevin, I cannot actually code-review this, but if you make a list of what needs to be confirmed as working I'll do some extensive testing on the patch in #102. Let me know.

Kristen Pol’s picture

@klonos - testing would be awesome... I added some test cases in the issue summary at the top of this page awhile back. Hope that helps.

lhridley’s picture

I'm very interested in revisions on field collections.

In reading through the testing notes for the patches, it appears that the current patches provide support for revisions for embedded field collections only.

Are there any plans to extend that to hidden field collections? This is the functionality I am most interested in for a project I have underway where field collection will fit the bill 100% IF revisions can be implemented for hidden field collections.

Kristen Pol’s picture

The patch should support both embedded and hidden. I didn't add testing notes for the hidden, though, because I wasn't using them. Please feel free to update the issue summary with instructions on how to test hidden FC items. Thanks!

alanburke’s picture

This patch isn't working when file fields are included in the field collection item.

caseyc’s picture

@alanburke, what specifically is happening? We are using this patch on a live client site without any problems and we have several images in Field Collections.

Are you sure you're referencing 'item_id' instead of 'value' in the array in your tpl files?

Example:

foreach ($content['field_template2_bookshelf']['#items'] as $k => $v) {
  $item = $content['field_template2_bookshelf'][$k]['entity']['field_collection_item'][$v['item_id']];
  print render($item['field_book_image']);
}

This patch is solid. It passes the tests locally if you patch the entity module too (which you can't do with the d.o test, so it'll always fail).

Kristen Pol’s picture

@caseyc - Awesome to hear that you are using the patch successfully "in the wild!" :)

caseyc’s picture

@Kristen - we have been monitoring it's movements closely and by all measures it seems to be happy in its new home - deep in the Production jungle. We are happy that while we had it in captivity, it's diet consisted of mostly billable hours. :)

Kristen Pol’s picture

@caseyc - I like your style ;) Btw, are you using embedded and/or hidden FC fields? And, are you using Workbench? Any details on your setup are appreciated so we know better on where it is working properly. Thanks!

stella’s picture

I'm having the same problem as alanburke above. It's nothing to do with tpl files. When you go to edit the revision, the image simply isn't there.

From poking around in the database I can see all the correct entries are in the various field collection tables. However, the fids for all but the new draft revision are pointing to deleted files. The fids don't exist in the file_managed table.

Will poke around some more and see if I can figure anything else out.

By the way, I'm using field_collection 7.x-1.0-beta4 with the above patch and entity api 7.x-1.0-rc2 with the patch from comment #111 on issue 996696

stella’s picture

I'll try and debug more over the weekend, but I've also found out that the file_usage entry is removed and that the file deletion is happening in file_field_delete_file() in file.field.inc.

stella’s picture

I found the problem. The code should never have reached file_field_delete_file() but it was because $entity->revision wasn't set, needed in file_field_update(). I've updated the entity api patch over at http://drupal.org/node/996696#comment-6363272

HyperGlide’s picture

@stella +++

fago’s picture

Status: Needs work » Needs review
FileSize
28.05 KB

I took a look at this based upon the latest entity api patch. For saving revisions, we need to save all field-collection revisions as soon as a host entity revision gets saved, what requires overhauling the save logic quite a bit.

Attached patch does so and adds tests for that - seems to work good so far. It misses the view/edit/delete menu callbacks that work with revisions (should not go with revisions by default), for which we also need entity revision access checking support.

Status: Needs review » Needs work

The last submitted patch, entity_revisions.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
29.77 KB

wrong patch...

Status: Needs review » Needs work

The last submitted patch, field_collection_revisions.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

Note: Patch requires the entity api patch to work.

andypost’s picture

Patch requires the entity api patch to work.

Please point to a patch to test it

dagmar’s picture

HyperGlide’s picture

fago committed #125 on issue #996696: Support revisions in Entity API one step closer to field collection revision.

Thank you for all the efforts!

tim.plunkett’s picture

#121: field_collection_revisions.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, field_collection_revisions.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
40.97 KB

ok, worked over this again. Turned out it's even more complex as there might be field collections that are not referenced in any default revision any more, but those still must have a default revision theirself. Also, deletion logic must intelligently handle deletion revision or the whole item in order to prevent loosing history.

I've fixed by introducing an 'archived' flag, which is set for field collections that are not referenced in the default host entity revision. Also, I've implemented the deletion logic and added tests for all that. In regard to the UI, I've disabled the edit/delete/add links for revisions as those would be used for editing non-default revisions what is a) not supported by core and b) troublesome as it would publish the edited revision.

Patch attached. Requires the latest entity API dev version (as of *now*).

Status: Needs review » Needs work

The last submitted patch, field_collection_revisions.patch, failed testing.

fago’s picture

tim.plunkett’s picture

The testbot is checking out an older version of Entity it seems (Call to undefined function entity_revision_is_default())

fago’s picture

Status: Needs review » Fixed

ok, I've moved on and committed this. As mentioned this requires the latest entity api *dev* version.

crimsondryad’s picture

yay! Now us ingrates would like to know when the next release of field collections is going to drop with this patch included. :P

I know this one was a bear, thank you all for working on it. We really needed this!

HyperGlide’s picture

Fago ++++++
tim.plunkett ++++++
drupal ++++++

thanks for this commit

klonos’s picture

Yeah, I'd also like to say thank yous!

infojunkie’s picture

Works for me, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mrfelton’s picture

I'm having an issue with this for field collections that are attached to user entities, and the user entity is created via rules using one existing field_collection to populate the data for the field_collection attached to the user entity. Details in #1810548: "Integrity constraint violation column [xyz]_revision_id cannot be null" when saving entity with attached field collection

Anonymous’s picture

Many thanks to everyone who has worked on this issue, the latest patch appears to have resolved the majority of the problems for me. There's only one problem that I've not been able to resolve.

I've created several field collections and made three revisions. My test results for this using the latest patch:
- Check that each revision (via the revision links /node/XX/revisions/XX/) renders the correct results = PASS
- Check the raw node output (with no panel / view etc) renders the correct field collection value when reverted back = PASS
- Check the field collection value within a custom field_collection view = FAIL
- Check the field collection value when loaded programmatically (code below) = FAIL

Code used to look up field collection value:

<?php
  $node = node_load(24);
  $fc_id = field_get_items('node', $node, 'field_itinerary_events');
  $fc = entity_load('field_collection_item', array($fc_id[0]['value']));
  dpm($fc);
?>

When I view a node (that has been reverted to an earlier revision) either programmatically (as above) or in a view, the field collections always display the most recent data (and not the old data the node was reverted to).

Any idea's how to resolve this?

Thanks
Emma

capellic’s picture

@insparrow - I am able to duplicate your issue.

I always get the most recent revision in the dpm output whether it be the published, draft or otherwise. Upon looking at the field_data_field_*, I see that the value field has the latest revision value. So, it would seem that the problem here is that the data table is being improperly updated-- it should only be updated when a revision is promoted as published.

I am also using Workbench Moderation but turned it off and confirmed the issue is here with Field Collections.

It doesn't appear that the testing script at the top of this case adequately covered the workflows where I have found the bug.

I initially created this screencast for an issue for Workbench Module, #1443604: Ability to Incorporate -- Field collections -- Into Workbench Moderation, but think it's valuable. here.
https://dl.dropbox.com/u/4770698/Drupal-org/Workflow%20Moderation%20with...

Note that the workflow exposed two problems:

1. Use it or lose it. If you don't define the field collection values upon creation of the node, you can never do so.
2. Updating an existing field collection value in draft mode immediately publishes draft field collection values.

I am going to reopen this case.

capellic’s picture

Status: Closed (fixed) » Needs work
capellic’s picture

1. Use it or lose it. If you don't define the field collection values upon creation of the node, you can never do so.

Resolved with the patch on this case: #1844322: Unable to save draft when adding new field values on published node (workbench moderation)

John Pitcairn’s picture

Status: Needs work » Closed (fixed)

Patch referred to in #143 is at #1807460: Field collection doesn't play nice with workbench moderation (patch). Closing again, specific problems should be new issues rather than prolonging this thread.

capellic’s picture

sarjeet.singh’s picture

Subscribing

radiobuzzer’s picture

Status: Closed (fixed) » Needs work

Hi, there is a bug here. The field collection can be modified in two ways, :
(a) through the edit form of the host entity if the widget mode of the field collection is "embedded"
(b) through the view mode of the host entity even if the widget mode of the field collection is not "embedded". In this case, when you look at the node, there are these links:
add field collection
edit field collection
delete field collection

adding a field collection via (a), produces a new revision [OK]
adding a field collection via (b), does not produce a new revision [wrong]
or if the host node already has a revision, it modifies the date of that revision [wrong]

editing a field collection via (a) produces a new revision [ok]
editing a field collection via (b) produces a new revision but makes the previous revision disappear [wrong]

deleting a field collection via (a) produces a new revision [ok]
deleting a field collection via (b) produces a new revision but makes the previous revision disappear [wrong]

It seems the field revisions work ok, only when modifications are done through the edit form of the host node
If modifications are done through the view mode, then there is buggy behaviour which leads into losing/overwriting the previous revision

james.williams’s picture

I ran into another surprising bug yesterday, most likely with FieldCollectionItemEntity::deleteRevision() ... Here are the steps to reproduce:

1. Have an image or file field on a bundle of an entity type *that does not support revisions*, and a field collection field on it too (it may be important to add the field collection field to the entity's bundle last so that fields get processed in that order).
2. Save an entity with values in those fields.
3. Re-save the entity with a different file in the image/file field (so it refers to a file with a different file ID), and remove the field collection(s) (in this case, I was doing this all programmatically, so just by setting $host_entity->field_collection_field = array();).
4. Deep in core's Field API, on saving the host entity:
a) in the file/image field, the old file is removed and the new one saved, and
b) field_collection_field_update() finds the now-removed field_collection_item, and deletes the orphaned field_collection_item entity, by calling $item->deleteRevision(TRUE);.
5. Since the host entity type does not support revisions, deleteRevision() calls through to $this->delete(), regardless of the value of the $skip_host_update argument:

  public function deleteRevision($skip_host_update = FALSE) {
    if (!$this->revision_id) {
      return;
    }
    $info = entity_get_info($this->hostEntityType());
    if (empty($info['entity keys']['revision']) || !$this->hostEntity()) {
      return $this->delete();
    }

6. So that delete() call ends up trying to re-save the host entity, despite that $skip_host_update correctly implying that this shouldn't happen (since we're already in the middle of saving the host entity) in step 3. This appears to be where the bug is?
7. The host entity is loaded in order to save it, but that loads the version of it from before step 3, which contains the old file ID in the file/image field.
8. Saving that host entity includes trying to write a non-existent file in the image/file field, which file module replaces with a NULL entry in doing so. (In file_field_load():

      // If the file does not exist, mark the entire item as empty.
      if (empty($item['fid']) || !isset($files[$item['fid']])) {
        $items[$id][$delta] = NULL;
      }

9. The NULL entry in the file/image field is actually saved as a file by file_field_presave(). If that has happened before, the integrity constraint violation occurs, since the uri of the record being written will be empty, but that database column is a unique key.

Now, there is probably a bug in core there too, in that file_field_presave() should not try to save a NULL file -- see #1443158: file_field_presave assumes that a file object has been loaded for progress on that, but I expect the bug on the side of field collection can be quicker than core.

In step 6 above, I believe that the $skip_host_update argument should be passed along to delete() and checked before calling deleteHostEntityReference() there, to stop the host being updated as per that argument when it is sent to deleteRevision().

I can post a patch if necessary. But I wanted to know first, is there a reason that the $skip_host_update is not checked at all for host entity types that do not support revisions?

james.williams’s picture

(Sorry, posted twice by accident.)

crimsondryad’s picture

@radiobuzzer @james.williams Since this patch has already been committed and marked as fixed, perhaps it would make more sense to enter separate issues into the queue rather than reopening/hijacking this one. If these are separate bugs they will probably get fixed faster individually.

eric_washburn’s picture

I had an issue with this using Field Collection 7.x-1.0beta5+1-dev and Revisioning 7.x-1.4.

New revisions would only be created if I was editing a revision in draft. Initially creating the revision from editing a node would not create a new Field Collection revision.

I was able to get a revision to be created by replacing the following text in field_collection.module inside field_collection_field_presave():

if(!empty($host_entity->revision)){

With:

if((isset($host_entity->revision_operation) && $host_entity->revision_operation != 0) || !empty($host_entity->revision)){

Which solved my problem of a FC revision not being created when a user selected 1 or 2 as the revision_operation, since $host_entity->revision was still false in this case. I am hoping this does not affect some weird corner case though.

John Pitcairn’s picture

$host_entity->revision_operation is a Revisioning-specific property, so I don't think that's such an appropriate place to handle it. Entity API doesn't know anything about those properties.

You should possibly try to do something like implement hook_revisionapi(), test if Revisioning is saving a new revision, and re-save any field collection entities on the node with the $entity->revision flag set. You may need to do that on update, not on presave.

You may find useful entity code in #1901892: Most recent Field Collection revision always appears in View when using Workbench Moderation.

donaldwbabcock’s picture

I can confirm this issue.

It is of particular concern to me because I am forced to use the Add/Edit/Delete mode due to the issue described here. #1549364: Data not being saved on initial creation.

If I use embedded, my field groups get wiped out (or not saved at all), if I use hidden my field groups don't get revision'ed properly.

John Pitcairn’s picture

@donaldwbabcock - are you using Workbench Moderation? If so, have you tried embedded mode with latest dev, applied the patch at #1807460: Field collection doesn't play nice with workbench moderation (patch), and tried the patch (or sandbox module) at #1901892: Most recent Field Collection revision always appears in View when using Workbench Moderation?

donaldwbabcock’s picture

No, no add on revisioning control yet, only core. Site is under development, and "editor > reviewer > approver" is on the "would like to have" feature list, but we are not there yet.

donaldwbabcock’s picture

Issue summary: View changes

Updated issue summary.

lpalgarvio’s picture

the dependency #996696: Support revisions in Entity API has been fixed

#1186552: Support Revisions in ECK hasn't resumed either

lpalgarvio’s picture

It has been committed.

Andy Tawse’s picture

ignore

zhuber’s picture

This is still an issue for me. I'm using the field_collection 7.x-1.x git repo (with the patches for this ticket committed) and the most recent dev versions of the entity and revisioning modules.

Can anyone confirm that this is working for them?

Here is my workflow:

  • Create article
  • Publish
  • Edit article
  • Add items into my field collection (multivalue)
  • Check the revision option to create a new revision
  • Save

The field collection will remain the same, no matter which version you look at. Also, the revision diffs say "no differences".

John Pitcairn’s picture

Possibly a revisioning issue? Disable Revisioning, save with new revision, check the changed field collection appears in the new revision, check the unchanged field collection appears in the previous revision.

zhuber’s picture

Thanks John.

You're right, I've spent more time looking into this issue and I was able to actually get it to work.

I am still having issues with a view however, which only seems to be able to return the latest revision (even when specifying the correct VID via contextual filters). Not sure if this issue is with field collections, views or revisioning.

EDIT: I've confirmed that this is an issue with the revision VID filter, since drupal only keeps track of the last vid in the node table and it cannot join the tables correctly.

Sorry for hi-jacking the thread.

John Pitcairn’s picture

bpadaria’s picture

Hi,

I am using https://drupal.org/project/revisioning module.

I tried going through patch in method 2. It looks like patch is already been applied for dev versions for field collection and entity api both. But still it is not supporting revision in my case.

My node can embed more than one field collection item.

Problem occurs while updating published node and auto publish for moderator option is set on.

Edit: It works well with workbench moderation. So issue seem to be with "Revisioning" module instead.

Thanks!

coderider’s picture

Hi

i tried all field collection versions also worked with provided patches,
Field Collection 7.x-1.x-dev include all required patch with this version you are able to use field Field Collection with Drupal revisions and Workbech moderation module but only one thing this left in this version when you delete old revisions then you lost your field collection data which is add by in old revision. you only get that data which you add as add an other item with your published copy. i have write a one line patch after apply this patch this issue should solve.
field_collection.module function field_collection_field_update
after line number 939

$entity->item_id = NULL;

like this

 if (isset($item['entity']) || !empty($host_entity->revision)) {

      if ($entity = field_collection_field_get_entity($item)) {
        $entity->item_id = NULL;

        if (!empty($entity->is_new)) {
          $entity->setHostEntity($host_entity_type, $host_entity, LANGUAGE_NONE, FALSE);
        }

this change made your already indexed fields new for module and all fields should index with the new revision id relation

tested and working at live site
enjoy :)

michielmeuris’s picture

#164 worked for me. Thanks! :)

sinasalek’s picture

using the latest dev #164 causes the items to disappear from host. Also in my case it wasn't even needed and everything seems to be working just fine

John Pitcairn’s picture

For those having issues with losing data when deleting old revisions, see #2000690: Deleting revisions via node_revision_delete wrongly deletes entire field collection.

fago’s picture

Status: Needs work » Closed (fixed)

Revision support is there for a while now - re-opening this old issue may confuse others. Please keep this issue closed and open separate issues for any troubles. For notifying others interested in this a short comment posting the new issue here should do it.

roynilanjan’s picture

Raised an issue according to the working around #164 @ https://www.drupal.org/node/2634574