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
- IMPORTANT! Use a test database / site.
- Installation:
- Install dev versions of Entity API and Field Collection modules
- Apply patch for Entity API revisions
- Apply patch for this issue (find latest version for testing)
- Run update.php
- 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
- 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"
- 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"
- 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"
- Edit/Delete/Add links
- TODO: Testing instructions need to be added for these links
Testing with Workbench Moderation
- Installation
- Install Workbench and Workbench Moderation
- 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
- 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
- Publish Draft
- Publish Draft
- Published node should show "WB TEST ONE"
- View the previous revision - it should have the original Test Text
- Revert versions
- Go to the moderation list
- Revert to original node
- Should create new draft
- Publish draft
- Published node should show original text
Comment | File | Size | Author |
---|---|---|---|
#129 | field_collection_revisions.patch | 40.97 KB | fago |
#121 | field_collection_revisions.patch | 29.77 KB | fago |
#119 | entity_revisions.patch | 28.05 KB | fago |
#102 | field_collection-1031010-revisions-102.patch | 22.78 KB | wizonesolutions |
#95 | field_collection-revisions-1031010-94.patch | 22.98 KB | wizonesolutions |
Comments
Comment #1
BenK CreditAttribution: BenK commentedSubscribing
Comment #2
yched CreditAttribution: yched commented"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 ?
Comment #3
servantleader CreditAttribution: servantleader commentedSubscribing
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.
Comment #4
robmalon CreditAttribution: robmalon commentedSubscribing
Comment #5
cfennell CreditAttribution: cfennell commentedsubscribing and will test/report back once this feature is in place.
Comment #6
John Pitcairn CreditAttribution: John Pitcairn commentedSubscribe. Just a separate revision for each revision of the host, I think.
Comment #7
jtbayly CreditAttribution: jtbayly commentedsubscribe
Comment #8
ogi CreditAttribution: ogi commentedsubscribe
Comment #9
cfennell CreditAttribution: cfennell commentedAfter 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?
Comment #10
RobW CreditAttribution: RobW commentedI 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.
Comment #11
stevectorThis 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.
Comment #12
Bevan CreditAttribution: Bevan commentedWhile this still needs work, it needs review to move forward with that work.
Comment #13
valderama CreditAttribution: valderama commentedsubscribing
Comment #14
yched CreditAttribution: yched commentedThis 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.
Comment #15
tedbowI 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:
If works but thought there might be better way.
Comment #16
tedbowMy last patch had a logical error in the statement when testing whether to create a new Item.
I have fixed and updated it.
Comment #17
AtomicTangerine CreditAttribution: AtomicTangerine commentedsub
Comment #18
droath CreditAttribution: droath commentedSubscribing
Comment #19
khanz CreditAttribution: khanz commentedsubscribing
Comment #20
kalabroSubscribing
Comment #22
John Pitcairn CreditAttribution: John Pitcairn commentedI 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.
Comment #23
John Pitcairn CreditAttribution: John Pitcairn commentedPatch at #16 works for me against 7.x-1.0-beta2. Patch applied from module root folder.
Comment #24
John Pitcairn CreditAttribution: John Pitcairn commented#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.
Comment #25
kadimi CreditAttribution: kadimi commented#16 worked for me in beta 2
Comment #26
Zen CreditAttribution: Zen commented#16: field_collections_revisions_in_field_presave-1031010-16.patch queued for re-testing.
Comment #28
tedbowI have re-rolled my previous patch against the latest dev branch.
Comment #29
tedbowI have re-rolled my previous patch against the latest dev branch.
Comment #30
tedbowSorry 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
Comment #31
tedbowThe 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.
Comment #32
doana CreditAttribution: doana commentedThis issue affects me as well. Thanks to everyone who's working on this!
Comment #33
Fidelix CreditAttribution: Fidelix commentedI 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.
Comment #34
wmostrey CreditAttribution: wmostrey commentedThe 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.
Comment #35
tim.plunkettThe patch makes sense, but there are some small bits to clean up. Feel free to set back to RTBC after the reroll.
Space after //, start with capital letter, end with a full stop
I would change this to
!isset() || !empty()
, no reason to use 1The function should start with
$form = array();
Space after //, start with capital letter, end with a full stop
No reason to simplify this variable, just use
$instance['settings']['new_revision']
later on like the above functionDon't use '1', use TRUE
comma after 'checked', use double quotes to avoid escaping the apostrophe
Move this out of the conditional, the hook expects something to be returned.
Missing a blank line
Comment #36
HyperGlide CreditAttribution: HyperGlide commentedWishing I knew more PHP to help put #35 into practice. Really looking for this feature ;-)
Comment #37
tedbowAttached a patch that fixes issues @tim.plunkett documented in #35
Comment #38
tedbowmarking for review
Comment #39
tedbowmarking for review
Comment #40
HyperGlide CreditAttribution: HyperGlide commented@tedbow Thank you!
Comment #41
tedbowOk 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.
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.
Comment #42
HyperGlide CreditAttribution: HyperGlide commented@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.
Comment #43
liquidcms CreditAttribution: liquidcms commented#37 works for me.. and collection with workbench as well as a use case.
let's get this committed.. :)
Comment #44
HyperGlide CreditAttribution: HyperGlide commented@liquidcms -- Did you read #41 -- Suspect #37 will not get committed and other solutions are in the thought process.
Comment #45
tedbow@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.
Comment #46
liquidcms CreditAttribution: liquidcms commented@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.
Comment #47
tedbow@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.
Comment #48
liquidcms CreditAttribution: liquidcms commentedunderstood... 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.
Comment #49
crimsondryad CreditAttribution: crimsondryad commentedI'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.
Comment #50
limptimtim CreditAttribution: limptimtim commented#37: field_collection-revisions_in_field_presave-1031010-37.patch queued for re-testing.
Comment #51
tim.plunkettComment #52
Kristen PolI 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:
I love this module and I'm very sad I will have to not use it until this issue is resolved.
Comment #53
Kristen PolI'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...
Comment #54
Kristen Pol[comment deleted] Nevermind... dumb comment ("git diff" was showing me the Entity API changes not the Field Collection changes)... need some coffee I guess.
Comment #55
Kristen PolWell, I've started but it needs a lot more work. So far I've done:
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.
Comment #56
Kristen PolComment #57
Kristen PolLast patch was mangled. This one should work.
Comment #58
Kristen PolSince both
item_id
andrevision_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: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!
Comment #59
tedbow@Kristen Pol,
Thanks for getting this started.
RE:
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
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?
Comment #60
Kristen PolThanks, @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.
Comment #61
tim.plunkettI agree with tedbow.
Comment #62
Kristen PolI 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
andrevision_id
in the field_data*/field_revision* tables.I will try to get something out tonight!
Comment #63
Kristen PolHere is a first pass at the complete patch. I have tested the following:
Issues:
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.]
Comment #65
Kristen PolTrying 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.]
Comment #67
Kristen PolAnd 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.]
Comment #69
Kristen PolWell, 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.]
Comment #70
tedbow@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.
Comment #71
Kristen PolWhen 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.
Comment #72
tim.plunkettThis should past tests, but we can't break the schema like this. I fixed the tests to use 'item_id' and not 'value', but...
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.
Comment #73
tim.plunkettEr, status-fail.
Comment #75
tim.plunkettWell that was the wrong patch, heh.
Comment #77
tim.plunkettOh that was wrong/worse.
Comment #78
Kristen Pol@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.
Comment #79
Kristen Pol@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:
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.
Comment #80
Kristen Pol@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?
Comment #80.0
Kristen Polput reason for postponing
Comment #80.1
Kristen PolUpdated issue summary.
Comment #81
tim.plunkett@Kristen Pol, ignore both patches I uploaded, they were shortsighted and changed the wrong things.
Comment #81.0
tim.plunkettUpdated issue summary.
Comment #82
Kristen Pol@tim.plunkett - No worries! Any idea why SimpleTest is passing locally for me but failing on d.o?
Comment #83
Kristen PolIgnore this patch. This is just to test the SimpleTest tests to make sure they pass with a "nothing" patch (added blank line).
Comment #84
Kristen PolI 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.
Comment #85
Kristen PolI'm creating a new patch that updates the various menu items/paths (e.g. view/edit/delete). Hope to get it up soon.
Comment #86
Kristen PolHere'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.
Comment #88
Kristen PolIt 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:
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?
Comment #88.0
Kristen PolUpdated issue summary.
Comment #88.1
Kristen PolUpdated issue summary.
Comment #88.2
Kristen PolUpdated issue summary.
Comment #89
Kristen PolI 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.
Comment #90
Kristen PolFor 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:
in the
field_collection_field_presave
function, right before calling: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.
Comment #90.0
Kristen PolUpdated issue summary.
Comment #91
wizonesolutionsKristen: 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.
Comment #92
wizonesolutionsTests 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.
Note: I didn't pass --uri because I have
$options['uri'] = 'example.com';
insites/all/drush/drushrc.php
.Comment #93
Kristen PolYou're right! Argh. <facepalm> I hadn't created a patch from the most recent changes :/ Ok, let's try this again.
Comment #95
wizonesolutionsThis is a version of the above patch that is fixed up to apply.
Comment #97
wizonesolutionsAck, this one *does* pass locally but fails here. Looking into why.
Comment #98
wizonesolutionsIt'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.
Comment #99
Kristen PolAh! Of course... makes sense. Ok, then we all need to get the Entity API revisions patch committed ASAP! :)
Comment #99.0
Kristen PolUpdated issue summary.
Comment #99.1
Kristen PolAdded a note about new comment with stop-gap measure
Comment #100
wizonesolutionsAssigning to myself for the moment.
Comment #101
wizonesolutionsTested this. Functionally, it seems sound, so just need to figure out the implications of the TODO: FIXME parts and address them.
Comment #102
wizonesolutionsOK, I've removed the
TODO: FIXME
notes and changedvalue
toitem_id
in thegetter 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.
Comment #104
wizonesolutionsA couple points I have noticed about the approach, but I would rather have the existing work reviewed before actioning them:
hook_field_attach_delete()
though (that is whatnode_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.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 offield_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.Comment #105
klonos@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.
Comment #106
Kristen Pol@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.
Comment #107
lhridley CreditAttribution: lhridley commentedI'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.
Comment #108
Kristen PolThe 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!
Comment #109
alanburke CreditAttribution: alanburke commentedThis patch isn't working when file fields are included in the field collection item.
Comment #110
caseyc CreditAttribution: caseyc commented@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:
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).
Comment #111
Kristen Pol@caseyc - Awesome to hear that you are using the patch successfully "in the wild!" :)
Comment #112
caseyc CreditAttribution: caseyc commented@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. :)
Comment #113
Kristen Pol@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!
Comment #114
stella CreditAttribution: stella commentedI'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
Comment #115
stella CreditAttribution: stella commentedI'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.
Comment #116
stella CreditAttribution: stella commentedI 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
Comment #117
HyperGlide CreditAttribution: HyperGlide commented@stella +++
Comment #119
fagoI 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.
Comment #121
fagowrong patch...
Comment #123
fagoNote: Patch requires the entity api patch to work.
Comment #124
andypostPlease point to a patch to test it
Comment #125
dagmarThis is the patch: #996696-145: Support revisions in Entity API
Comment #126
HyperGlide CreditAttribution: HyperGlide commentedfago committed #125 on issue #996696: Support revisions in Entity API one step closer to field collection revision.
Thank you for all the efforts!
Comment #127
tim.plunkett#121: field_collection_revisions.patch queued for re-testing.
Comment #129
fagook, 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*).
Comment #131
fagoRelated issues I discovered while coding this:
#1776424: Removed field-collections are never deleted
#1776928: Leverage multiple entity loading
#1776918: field schema should have index on item_id
Comment #132
tim.plunkettThe testbot is checking out an older version of Entity it seems (
Call to undefined function entity_revision_is_default()
)Comment #133
fagook, I've moved on and committed this. As mentioned this requires the latest entity api *dev* version.
Comment #134
crimsondryad CreditAttribution: crimsondryad commentedyay! 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!
Comment #135
HyperGlide CreditAttribution: HyperGlide commentedFago ++++++
tim.plunkett ++++++
drupal ++++++
thanks for this commit
Comment #136
klonosYeah, I'd also like to say thank yous!
Comment #137
infojunkie CreditAttribution: infojunkie commentedWorks for me, thanks!
Comment #139
mrfelton CreditAttribution: mrfelton commentedI'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
Comment #140
Anonymous (not verified) CreditAttribution: Anonymous commentedMany 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:
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
Comment #141
capellic@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.
Comment #142
capellicComment #143
capellicResolved with the patch on this case: #1844322: Unable to save draft when adding new field values on published node (workbench moderation)
Comment #144
John Pitcairn CreditAttribution: John Pitcairn commentedPatch 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.
Comment #145
capellicJohn, I've moved the issue to a new case: #1901892: Most recent Field Collection revision always appears in View when using Workbench Moderation
Thanks for your help.
Comment #146
sarjeet.singh CreditAttribution: sarjeet.singh commentedSubscribing
Comment #147
radiobuzzer CreditAttribution: radiobuzzer commentedHi, 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
Comment #148
james.williams CreditAttribution: james.williams commentedI 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: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()
: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 todelete()
and checked before callingdeleteHostEntityReference()
there, to stop the host being updated as per that argument when it is sent todeleteRevision()
.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?Comment #149
james.williams CreditAttribution: james.williams commented(Sorry, posted twice by accident.)
Comment #150
crimsondryad CreditAttribution: crimsondryad commented@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.
Comment #151
eric_washburn CreditAttribution: eric_washburn commentedI 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():
With:
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.
Comment #152
John Pitcairn CreditAttribution: John Pitcairn commented$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.
Comment #153
donaldwbabcock CreditAttribution: donaldwbabcock commentedI 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.
Comment #154
John Pitcairn CreditAttribution: John Pitcairn commented@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?
Comment #155
donaldwbabcock CreditAttribution: donaldwbabcock commentedNo, 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.
Comment #155.0
donaldwbabcock CreditAttribution: donaldwbabcock commentedUpdated issue summary.
Comment #156
lpalgarvio CreditAttribution: lpalgarvio commentedthe dependency #996696: Support revisions in Entity API has been fixed
#1186552: Support Revisions in ECK hasn't resumed either
Comment #157
lpalgarvio CreditAttribution: lpalgarvio commentedIt has been committed.
Comment #158
Andy Tawse CreditAttribution: Andy Tawse commentedignore
Comment #159
zhuber CreditAttribution: zhuber commentedThis 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:
The field collection will remain the same, no matter which version you look at. Also, the revision diffs say "no differences".
Comment #160
John Pitcairn CreditAttribution: John Pitcairn commentedPossibly 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.
Comment #161
zhuber CreditAttribution: zhuber commentedThanks 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.
Comment #162
John Pitcairn CreditAttribution: John Pitcairn commentedPerhaps take a look at #1901892: Most recent Field Collection revision always appears in View when using Workbench Moderation for the views issue.
Comment #163
bpadaria CreditAttribution: bpadaria commentedHi,
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!
Comment #164
coderider CreditAttribution: coderider commentedHi
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
like this
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 :)
Comment #165
michielmeuris CreditAttribution: michielmeuris commented#164 worked for me. Thanks! :)
Comment #166
sinasalek CreditAttribution: sinasalek commentedusing 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
Comment #167
John Pitcairn CreditAttribution: John Pitcairn commentedFor those having issues with losing data when deleting old revisions, see #2000690: Deleting revisions via node_revision_delete wrongly deletes entire field collection.
Comment #168
fagoRevision 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.
Comment #169
roynilanjan CreditAttribution: roynilanjan commentedRaised an issue according to the working around #164 @ https://www.drupal.org/node/2634574