Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This feature doesn't have a UI. It's already implemented in the 8.x branch.
Comments
Comment #1
amitaibuWe still need to deal with entityreference_field_property_callback() - we might need to write our own getter callback.
Comment #2
alexh CreditAttribution: alexh commentedHi,
I'm interested in this and tried the patch.
It is adding a column for the revision id, but it does not store any value there.
Is this supposed to work?
Greetings,
Alex
Comment #3
amirdt22 CreditAttribution: amirdt22 commentedAlso interested in storing revisions.
Comment #4
DamienMcKennaIs the goal to provide a select to let the user select the appropriate revision to use after an initial object is selected, or to provide a field setting that will control whether the reference record will be fixed to a specific revision?
Comment #5
DamienMcKennaFYI the patch still applies cleanly to the current 7.x-1.x codebase.
Comment #6
DamienMcKennaI've confirmed that when the revision field is assigned a specific value that the correct revision of the referenced content *does* load.
Comment #7
DamienMcKennaThis would help: #1939802: Enable / disable revisions per Bean Block type
Comment #8
DamienMcKennaThis patch adds a new option to the two included field selectors (EntityReference_SelectionHandler_Generic and EntityReference_SelectionHandler_Views) that will cause the specific revision_id to be saved when the referencing object is saved. This can be used to provide a rudimentary content workflow for referenced objects when coupled with e.g. Bean.
Comment #9
Albert Volkman CreditAttribution: Albert Volkman commentedUpdating reference to variable location.
Comment #10
DamienMcKenna@Albert: Thanks, I forgot that I moved the setting around and it became a field-level setting.
Comment #12
Albert Volkman CreditAttribution: Albert Volkman commentedWrong patch file, my apologies.
Comment #13
Albert Volkman CreditAttribution: Albert Volkman commentedComment #14
DamienMcKennaThe code I was trying to run in entityreference_field_presave() wasn't going to work correctly due to how the various APIs work, so I'm spinning it off as a separate sandbox. This patch includes the 'lock_revisions' option though as food for thought.
Comment #15
Jelle_SPatch based on @DamienMcKenna's work, previously the revision was not 'locked'. It was overwritten with each edit to the latest revision of the referenced entity.
Comment #16
Jelle_SSmall fix: previous patch gave an error when saving new entities with an entityreference field.
Comment #17
rylowry@gmail.com CreditAttribution: rylowry@gmail.com commentedThe patch in #16 seems to be working for me.
Comment #18
Jelle_SThis patch in combination with the patch in #1788568: entity_metadata_wrapper does not load correct revisions (comment #9) adds the metadatawrapper functionality on top of the previous patch.
Comment #19
Jelle_SSmall fix for when the referenced entity no longer exists.
Comment #20
Jelle_SViews support added.
Comment #21
Jelle_SSorry, patch from an other issue got mixed in with the previous one.
Comment #22
Jelle_SReroll
Comment #23
spatical CreditAttribution: spatical commented22: entityreference-n1837650-22.patch queued for re-testing.
Comment #24
spatical CreditAttribution: spatical commentedFor us it makes the most sense to simply always have the current draft of the node save the latest version of the bean. However, old revisions of the node will keep the reference to old revisions of the bean. Meaning the changes to the bean will only be published when the node is published. So this is a different solution but it is a modified the version by Jelle_S. I removed the field setting for revision_lock and all bean revisions will always be used and associated with the node revisions.
Comment #26
Cadila CreditAttribution: Cadila commentedWorking fine at all, but I can't explicitly set new revision id via metadata wrapper or event just setting revision_id in field array, actually I know what to blame. Its
Why do we need this? Maybe it will be better just to put this away?
Comment #27
Cadila CreditAttribution: Cadila commentedAdding patch from #24 with fixed paths to make it more green!
Comment #28
Cadila CreditAttribution: Cadila commentedI've just checked, latest patch working just fine! (for a while maybe)
Comment #29
Cadila CreditAttribution: Cadila commentedActually there is one bug: it doesn't work with metadata wrapper, so that means that it won't work neither with rules nor other good modules. I blame EntityDrupalWrapper, this code just won't use revision ids!
Pay attention to the entity_load call, it called without conditions! Just ids passed, and what happens next in DrupalDefaultEntityController:
So I hope that this is entity module's bug cause it provides MetadataWrappers and I will create an issue for entity module.
Comment #30
DamienMcKenna@cadila: Shouldn't #1788568: entity_metadata_wrapper does not load correct revisions resolve the problem with EntityDrupalWrapper though?
Comment #31
Cadila CreditAttribution: Cadila commentedYes, it solved the problem. The latest patch for this issue works good!
Comment #32
pfrenssenCan you please always post an interdiff so we can easily check the evolution of the patch? It seems like some functionality was removed from the patch in #22 to fit a particular use case in #24. How does this fit into the bigger picture?
This will need tests before we can confidently put this into production :)
Comment #33
Cadila CreditAttribution: Cadila commentedThe functionality that was removed was stupid and it was prohibiting attaching new revision to a field (it awlays was replaced by old revision id). So I don't think that it worths speding time on creating it's interdiff. The last patch is the same as #24 but I only changed file paths to relative. I'm testing the latest patch from #27 (same as #24) for a 2 weeks and it works good
Comment #34
pfrenssenI'm going to work on this. I will start with generating the missing interdiffs between each patch as it passed between developers so that we can get a clear view on how this patch has evolved.
Comment #35
pfrenssenHere are the missing interdiffs.
Comment #36
pfrenssenGoing through the interdiffs I think we should continue working on the latest patch #22 from Jelle_S as it is more fully featured. The 'locking' of the revision appears to be configurable so I don't see why this has been removed again in patches #24 and #27. If this didn't work as expected when the locking is disabled then we should fix it rather than remove it outright. There are use cases for both locking the revision and always having the latest one so we should support both.
Comment #37
pfrenssenRerolled #22 against latest HEAD.
Comment #38
pfrenssenStarted with some cleanups, renamed variables so they match with the rest of the module. Added some documentation.
Comment #39
pfrenssenFurther improved consistency with the existing code base, made the revision locking an optional setting in the interface, and added a test for the interface changes. Setting to needs review for the bot. This still needs work.
Weird is that I'm getting a failure on the last line of the EntityReferenceAdminTestCase in the develop branch, but the this seems to pass on the test bot. Locally the test gives back a table cell containing the text "No fields are present yet". I had to apply this patch to make it pass locally:
Comment #40
pfrenssenStarted working on a test. Also included this as an experiment to figure out why the above is failing locally but not on the bot:
This is only set to Needs Review to get the bot's feedback. This still needs work. The new test will probably fail.
Comment #42
pfrenssenThe puzzling problem with the locally failing test is probably due to a PHP bug: Bug #62639 XML structure broken. I'm running PHP 5.5.13 locally. I'll leave my fix in place since it works on all PHP versions.
Comment #43
pfrenssenWorked some more on the test.
Comment #44
pfrenssenComment #45
pfrenssenThe storing of revision references is now fully tested - in practice the test covers entityreference_field_presave(). The other parts of the patch are not yet tested.
I have also rewritten entityreference_field_presave() to be much more faster. It is now only loading entities when strictly needed, and I removed the nested loop which would take exponentially more time to process as the number of referenced items increases. Now the time needed will increase linearly with the number of referenced items.
Going to unassign myself as I don't have much spare time to work on this in the coming days.
Comment #46
pfrenssenA few questions:
Comment #47
pfrenssenSo while writing the test I mistakenly put the configuration in the field instance rather than in the field, then broke the functionality by focusing on getting the test green. Test driven development-- :)
When editing an entity with a locked node the title of the current revision is shown even though the original revision is saved, which is a bit weird.
Would welcome some input before continuing work on this.
Comment #48
Cadila CreditAttribution: Cadila commentedChuvak, skolko mozhno uzhe? Davai dopilivay normalno!
Comment #49
nagy.balint CreditAttribution: nagy.balint commentedSeems to work fine.
It would be nice if the formatter would output the revision link (if its available like on nodes). As currently it outputs a link to the entity with the correct title, but when i click on it i get the current version since the revision information is not in the link.
Comment #50
fschmitt CreditAttribution: fschmitt commentedHi everyone,
great patch, and works like a charm. I've got one question though regarding views support.
In entityreference.views.inc line#34, function entityreference_field_views_data($field), you limit relationships that bridge to revisions to fields with the "Lock the revision" setting:
if (isset($field['settings']['handler_settings']['lock_revision']) && $field['settings']['handler_settings']['lock_revision'] && isset($entity_info['revision table'])) {
Shouldn't this depend on the "Reference Revisions" setting instead ? It should be possible to build views relationships to referenced revisions whenever the revision id is available, not only if a reference is locked to a fixed revision. Or do i miss a point here ?
Best regards and thanks,
Florian
Comment #51
Cadila CreditAttribution: Cadila commentedLatest patch failed a little to apply on new entityreference version. I made some changes to in order to make it apply correctly.
Comment #53
pfrenssenThe above patch has been provided by @cadila so it can be applied to the latest 7.x-1.1 release. This is helpful for people that need this right now but do not want to update to the development version.
I'm reuploading the patch from #47 that was rolled against the latest 7.x-1.x-dev version so this turns back green and can be set back to "Needs review" status.
Comment #54
maximpodorov CreditAttribution: maximpodorov commentedThis patch should never get into Entity Reference (7.x)! It breaks many contributed modules which assume the field contains no revision information. And it breaks site configuration (think about Rules, Views, Panels) which uses target_id only to build links between entities. This may even lead to security problems (think of entities which can store access rule lists, so linking to incorrect revision is a problem).
Such functionality can be added to a new core version (Drupal 8) only. If it is needed for Drupal 7, please, create another field type!
Comment #55
DamienMcKenna@maximpodorov: The patch makes it optional, and disabled by default, thus would be fine for existing sites.
Comment #56
maximpodorov CreditAttribution: maximpodorov commentedNo, it creates new column in the field tables. To be fully compatible, it should use another tables.
Comment #57
maximpodorov CreditAttribution: maximpodorov commentedIf something new (linking to revisions) is required which should not affect existing code and sites, it must be another field type with good looking widgets and so on.
Comment #58
pfrenssen@maximpodorov, the patch does everything responsibly. It doesn't alter anything that is not fully owned by the module and does not change the existing API. It does not change any settings in existing websites. Everything will keep working as expected. Administrators can enable this at will if they want to start using the new functionality.
Yes it creates a new column in the table, but this does not affect anything, except if another module would have already altered the table and added this field themselves. They would then be in gross violation of the accepted coding practices and definitely not something we should care about.
Can you give an example of a contributed module that would break because of this patch?
Comment #59
maximpodorov CreditAttribution: maximpodorov commentedIt's irresponsible to break 160000 sites by this incompatible change.
Comment #60
DamienMcKenna@maximpodorov: This is not a compatibility change until you enable the new option. Having the extra field doesn't make any difference by default.
Comment #61
Cadila CreditAttribution: Cadila commentedI agree with DamienMcKenna that "Having the extra field doesn't make any difference by default". It just gives us some advantages to control programmaticaly revision changes.
Comment #62
maximpodorov CreditAttribution: maximpodorov commentedThink about some basic concepts: field value equality, entity equality.
If a field has one dimension (one column), field value equality is simple - it means column value equality. If sites use access rules which compares two entity reference fields:
document -(field_doc_org)-> organization
user -(field_user_org) -> organization
a user can access document if field_user_org == field_doc_org. it's very simple concept. For instance, ctools field comparison access plugin can be used for this (which compare all the columns of the field).
If you add another column to entity reference field, you must decide what field value equality is. Does it include revision id equality? Maybe yes in some cases and no in other cases? Can this user access a document if it belongs to an old revision of the same organization? What about the case when field_doc_org contains revision information and field_user_org does not? Can such field values be treated as equal?
The conclusion - field value equality concept will be broken with this patch.
Comment #63
pfrenssen@maximpodorov, there will be an additional column but if you do not enable the option this should not make much difference. Are you perhaps querying all columns of the database and doing a comparison with this, assuming that it will only contain the default column "target_id"?
If that is the case then I'm afraid your code is already broken without this patch. There is a hook that allows behavior plugins to alter the schema at will. See this relevant part in
entityreference_field_schema()
, behavior plugins can use this to add new columns to the schema:Do you have some public code that is affected by this that you can show us? We can help finding a solution.
Comment #64
maximpodorov CreditAttribution: maximpodorov commented@pfrenssen, you talk about reality, but I tried to explain the worry about the concepts.
The problem can be with entity_field_value access rule plugin of ctools patched with my patch: https://drupal.org/node/1630820
Comment #65
pfrenssenThat has not been implemented correctly. It is based around
_field_sql_storage_columnname()
which is a private function and for internal use of the field module only. Instead it should usefield_info_field()
which gives back a structured array of field data. This also contains the columns. Unfortunately using this function won't magically solve it since the plugin simply doesn't account for the fact that field columns can change. That is however a bug in that plugin and not in this patch.Your objection boils down to the fact that we're using
db_add_field()
. This is however perfectly legal, and often used, there are currently 25 calls to db_add_field() in Drupal core and 141 calls to db_add_field() in contrib.Comment #66
maximpodorov CreditAttribution: maximpodorov commentedDo you mean field value equality is equality of all required properties of the field?
Comment #67
pfrenssenThis is offtopic for this issue, let's take this discussion to #1630820: entity_field_value is completely broken.
Comment #68
maximpodorov CreditAttribution: maximpodorov commentedDrupal 8 adds "field's main property" concept
(see \Drupal\Core\TypedData\ComplexDataDefinitionInterface::getMainPropertyName() method).
Maybe something similar can be used here to mark target_id as main property, so the code which wants to know what entity reference is can get the answer. It's especially important since the current patch doesn't provide widgets for changing revision_id, so the code which wants to know whether a field can be treated as scalar value can treat entity reference field so (even with this patch applied). The example of such code is https://drupal.org/project/ddf module.
Comment #69
lwalley CreditAttribution: lwalley commentedTaking a look at the patch from #47 I'm trying to figure out how to save the latest pending revision in
entityreference_field_presave()
when editing a node (with create new revision on edit set).If the field settings are Reference Revision = TRUE and Lock Revisions = FALSE then the revision id will be set to revision id loaded by entity_load_single(), which seems to be the current (live) revision.
If the field settings are Reference Revision = TRUE and Lock Revisions = TRUE then the revision id seems to always be set to revision id of $entity->original.
Both of these settings and results make sense, I may just need an additional result for my use case, which is as follows:
If I set the entityreference field settings to Reference Revision = TRUE and Lock Revisions = FALSE then the entityreference field's revision_id is always saved as the current (live) revision of node B (loaded from
entity_load_single()
), and not to the newly created pending revision.If I set the entityreference field settings to Reference Revision = TRUE and Lock Revisions = TRUE then the entityreference field's revision_id is locked to the first revision (because it uses the value of $entity->original). Even if I specify a revision_id in the entityreference field values before saving
entityreference_field_presave()
will ignore that value and set it to the $entity->original value).So there doesn't seems to be a way to track the latest pending revision at the moment, even when specifically setting a revision_id on the entityreference field before saving the node.
Have I interpreted the current behaviour correctly? Any recommendations on how to reference the latest revision or a specific revision on save?
Many thanks to all for the work on this issue, it is much appreciated!
Comment #70
lwalley CreditAttribution: lwalley commentedAttached is a patch that switches current item and original when locking revisions so that if the current item value is set it will be used instead of the original setting. This completely goes against the purpose of locking a revision, and is only useful for my specific use case described in my previous comment #69 (I forgot to mention in my previous comment that I am using the revisioning module).
This was the quickest way I could find to allow a specific revision id to be saved and is definitely not intended as an improvement or continuation of the patch from #47.
Apologies if this patch throws any one off, if you are new to this issue then use the patch from #47 (#53).
Comment #71
fearlsgroove CreditAttribution: fearlsgroove commentedI've been testing this patch on a project and it seems to be working quite well. I think the code looks good style wise, and the patch has good test coverage that seems logically sound. It seems like any concerns about the patch in terms of BC break have been addressed.
I'm reposting #47 just so it's the top patch -- I think @lwalley's use case can be addressed as a follow up? It seems like creating a new revision might be setup as a behavior, or possibly as part of the logic in the widget.
What else can we do to move this forward? I'm tempted to RTBC this but I'm not sure I've worked with it quite enough.
Comment #72
nagy.balint CreditAttribution: nagy.balint commentedWe are using patch #47 in a project, and have not encountered any issue.
Comment #73
mpotter CreditAttribution: mpotter commentedI started testing this patch in Open Atrium since we want to be able to reference specific revisions in some workflow cases.
Seems to work fine so far. For others trying this, be sure to run update hooks (drush updb) to get the schema updated, otherwise you will get errors on existing sites.
My only request would be for when you are using the Lock revision option to have a button or something in the UI that allows the user to manually force it to point to the latest version. In our use case, we mostly want it locked to the revision used when the field was first created, but then in the future we want people to be able to update it to the latest revision. So, sort of a "manual lock" vs auto-lock.
Is there a way for me to add this myself in code? Or would it be easy to add a button next to the entity reference field for updating the revision? Otherwise, good work on this!
Comment #74
mpotter CreditAttribution: mpotter commentedI figured out how to do what I needed in my own hook_field_attach_presave() hook. Basically, I leave the Lock revision option disabled in the field settings and then apply my own logic to determine if I want to use a revision or not.
I like how this is cleanly architected. If I do an unset() of the 'revision_id' property, then it no longer ties the reference to any revision (always points to latest version). So there is really nice control here of whether to use revisions and how locking works. I love it!
Will let you know if I run into any compatibility issues with other modules. Open Atrium is pretty complex so it's a good test. For reference, I also applied #1788568: entity_metadata_wrapper does not load correct revisions and #2363599: Infinite loop after applying patch to Entity API to fix revision loading since OA2 uses OG. I'd say this patch is close to RTBC!
Comment #75
lwalley CreditAttribution: lwalley commentedI discovered an issue using my patch from #70 that I think will also affect the patch in #71. Basically the issue is that entity_extract_ids() will throw a fatal error if entity is NULL which appears to be the case of the referenced entity has been deleted; entityreference module does not currently removed references to entity see issue #1368386: Delete references to deleted entities.
Attached is an update to my patch from #70, again this is not the current patch for this issue (refer to #70 for more details on why not).
Comment #76
lwalley CreditAttribution: lwalley commentedFix for missing entity applied to patch from #71 (the current patch for this issue).
Comment #77
LGLC CreditAttribution: LGLC commentedThe Patch at #76 works great for me!
Comment #78
pfrenssen@lwalley, I have encountered a similar problem recently. Indeed in entity reference a recently deleted entity will be removed from the static cache, and the instead the value NULL will now be referenced. I would add a line of documentation there explaining that this check is placed there for this exact reason, because now it reads as if we are trying to reference an entity which cannot be loaded.
Comment #79
pfrenssenRemoving tag, I've already added tests for this half a year ago :)
Comment #80
maximpodorov CreditAttribution: maximpodorov commentedAgain, please, do not accept this patch. If this functionality is needed, please create another module: Entity Revision Reference. Switching from scalar data type (entity reference) to two-dimensional data type (entity and revision reference) IS incompatible change which WILL break existing sites. Many modules exist which follow the current metaphor of Entity Reference module (pointing to entities): widgets, formatters, etc. They will fail if he patch will be accepted. Think of a current Views selection handler. It assumes key => value concept of both Entity Reference module and Views result data. This concept can't be expanded to two-dimensional keys. Think of reference equality. Are (target_id => 4, revision_id => NULL), (target_id => 4, revision_id => 4), (target_id => 4, revision_id => 5) the same references? How will yo explain this equality or inequality to other modules which knows about target_id only?
Please do whatever you want in another module, not in Entity Reference module which is the center of a huge number of satellite projects. See the example of https://www.drupal.org/project/entity_reference_multiple project which doesn't try to break Entity Reference module by new concepts, but evolves in the separate project.
Comment #81
fearlsgroove CreditAttribution: fearlsgroove commented@maximpodorov -- none of your fears are justified, and have been thoroughly debunked by @pfresson. The default behavior of this module is completely unchanged by this patch. You can continue to completely ignore revisions unless you're doing something truly crazy.
Perhaps if you actually applied the patch and demonstrated an issue that you theorize this change might cause, that would help your case?
Back to NR, this patch continues to work well for me on production sites, including a backport to an existing site.
Comment #82
DamienMcKennaFYI there's also the Entity Reference Revisions.
Comment #83
maximpodorov CreditAttribution: maximpodorov commentedIf some dozens of truly crazy people need this functionality, they can create another module with necessary ER behaviors which modify the field schema, add new widgets, new selection handlers, etc. It's possible thanks to ER design. Why do they need to inject this crazy functionality into the main ER module, can someone explain? This new module can be advertised on the ER project page on drupal.org, so the people who need this functionality can get it easily.
Is there any other example of so crucial schema changing of a field used on 200 000 sites? It's really crucial since it turns one-column field into many-column one, so you can't think of the field VALUE any more, it becomes, hmm, a vector.
Comment #84
catchI'm also struggling to see what the use case for this is.
If you need to track revisions of content together for content staging, there's CPS module.
Comment #85
DamienMcKenna@catch: CPS is relatively new, this issue is over two years old.
Comment #86
catch@Damien yes but is there any other use case this is fixing?
If you reference specific revision IDs, then you always have to load the specific revision when viewing the reference. This means bypassing entity and field caching which is a big performance hit. It also complicates Views queries since you'd have to join on the entity/field revision tables.
Comment #87
LGLC CreditAttribution: LGLC commentedAh, I spoke too soon. After applying the patch at #76, any views that have aggregation on and include an entity reference field in their 'Fields' section will break. In order for them to work again, you need to re-save the aggregation settings of the field.
As an example, try this after applying the patch:
You can then remove the patch, complete steps 3-5 (and there will be no error). Then, apply the patch and run update.php and go back to the view, and it will show the same SQL error.
I've actually realised that I don't need the functionality required by this patch, but thought I'd report back with the issue I found.
Comment #88
fearlsgroove CreditAttribution: fearlsgroove commentedI use this patch to track specific revisions of entities referenced by nodes, so that when one reverts to previous revisions of the node, it also reverts to the revision of the referenced entity specified by that node revision. This allows nodes to be composed of multiple entities but still permits reviewing revisions (and reverting) to work as one would logically expect.
Comment #89
fearlsgroove CreditAttribution: fearlsgroove commented@catch -- can you give me a link or some insight into what the issue is with entity and field caching relating to tracking revisions?
On the issue of joining on revisions in views, the default behavior would remain unchanged unless we add joining on the revision ID as well -- it would simply join on the entity id from the field_data_* table, so it will always fetch the current revision of the referenced entity, which is the same as the current behavior. Supporting views when entities reference a specific revision could be a follow up enhancement.
Comment #90
catch@fearlsgroove:
https://api.drupal.org/api/drupal/modules%21field%21field.attach.inc/fun...
this is where the field cache happens, and it only happens if you load the current revision. If you load a specific revision this hunk will evaluate to FALSE:
But any other entity referencing the same one, might reference a different revision. Also if the entity being referenced is visible at its own URI, then that will still show the current revision.
Then if the referenced entity is updated independently of the node referencing it, it will also get out of sync.
Comment #91
fearlsgroove CreditAttribution: fearlsgroove commented@catch -- thanks that helps, but I'm not sure it's actually a problem. As far as entityreference itself, this would have an impact where the referenced entities are loaded, which is only in
entityreference_field_formatter_prepare_view
. Here it only loads revisions if a revision is specified. If you don't opt in to referencing a revision, it never calls entity_load_revision.I think there's room to optimize for those who do opt in
butwhen the revision happens to be the current revision for that entity.Does that seem right?
The patch already seems to handle if/else'ing the views behavior as well -- referenced entity revisions are a separate base table, otherwise the behavior is the same.
Is there somewhere else this is relevant as well?
Comment #92
pfrenssenMy use case is that I have entities that contain historical data. I have an Order entity that references many Product entities. A product might change at any time but this should have no effect on historical orders. A common case is that the prices of products change over time.
If a client retrieves the order from a year ago it should show all product details like they were at that moment, with the prices as they were on the day the client placed the order.
Comment #93
maximpodorov CreditAttribution: maximpodorov commentedIf you need this functionality, you can create a module which implements it.
Comment #94
DamienMcKennaWhat is the status on the D8 functionality that the issue description states was already committed, given entity_reference is in core?
Comment #95
fearlsgroove CreditAttribution: fearlsgroove commentedLooks like it was removed here: #2356609: Remove support for "reference a specific revision" with the stated reasons that there was no widget that used it and no use case. I'm not sure it's apples to apples tho, in this case any widget that applies entities may track the current revision of that entity if the reference field is configured to do so independent of the widget itself, and there's a couple decent use cases outlined in this issue. I think extending the reference field in D8 in contrib (or in a future core release) to add the feature is simpler and requires less code duplication than creating a entity reference revisions field in D7.
Comment #96
Leeteq CreditAttribution: Leeteq commentedThis is such a critical module for so many sites, still aiming at more stability and less cross-module conflicts.
I strongly agree that we should not take unnecessary risks, hence this should go into its own, separate, independent module.
What harm might it do to the hard earned Drupal stability and "reputation" if "unforeseen" bugs start cropping up in various semi-related modules that also many sites rely on by now?
Should not jeopardise it with features that are even not yet so critical AND have already been refused in D8 as per #2356609: Remove support for "reference a specific revision" , quoting @yched / issue summary over there:
Comment #97
Leeteq CreditAttribution: Leeteq commented(I suggest moving this issue over at the brand new module for this, although it does not have any branches yet, neither for 7.x nor 8.x - https://www.drupal.org/project/entity_reference_revisions )
Comment #98
catchOK I understand pfrenssen's use case, that's an interesting one if quite specific.
Comment #99
catchOK I understand pfrenssen's use case, that's an interesting one if quite specific.
Comment #100
pfrenssen@Leetec, your fears are not grounded, if we would follow that philosophy we cannot make any change in any module ever because of hypothetical incompatibilities. If the patch does not change the current functionality then there is no problem, which is why the revision tracking is switched off by default. This is also covered by automated tests.
Other modules that depend on entity reference will still simply use the old behavior unless they opt to add support for revisions in their own time.
This patch is not yet ready, it still needs work as indicated by the issue status. For example we need to investigate the report from #87.
In the current state the patch is already quite stable. As a testament to that, Open Atrium is using it in production and that is a very complex project that has been deployed more than 3000 times. I'm also using it in production for 2 fairly complex projects that have extensive test coverage.
Comment #101
maximpodorov CreditAttribution: maximpodorov commented@pfrenssen, why not creating another module? What is the reason to include this functionality in ER?
Comment #102
Leeteq CreditAttribution: Leeteq commentedYes, that is indeed the question; simply "why not" just make it another separate module, very in line with the prevailing Drupal policy for features that are deemed not to be at the heart of the focus for a given module.
And also, yes, it IS a special concern - and responsibility IMHO - to avoid unnecessary risk of breaking modules that are crucial for so many sites. This one in particular.
Comment #103
Cadila CreditAttribution: Cadila commentedNo! Shouldn't be new module cause it will be wrong logic. Entity module has native support of revision so entityreference should have had revisions in the begining. And the mistake is that they didn't add revision support to ER module from the begining and now we have millions of sites running entityreference with wrong logic. I think it should be a new branch for entityreference module and people will know that new sites should run new branch of ER and old sites shouldn't run new version cause it can brake their sites. After that another modules will think about adding support of revisions to their modules and new patches will appear when people will face the problems related to enabling of this module.
P.S: Running this patch on two sites for almost a year. All is OK!
Comment #104
maximpodorov CreditAttribution: maximpodorov commented@Cadila, no, you're not right. ER in D8 dropped this, as we see, so it can't get into D7's ER for migration reasons as well. A simple separate module can do all the logic this patch does.
Comment #105
pfrenssenThis patch belongs in Entity Reference and not in a separate module. FYI this feature request has been created by Amitaibu who is the maintainer of Entity Reference.
If you find anything breaking please report it here so we can address it.
Comment #106
maximpodorov CreditAttribution: maximpodorov commentedYou can't address it if the field schema is changed. The only way is not to change field schema on 200000 sites.
Comment #107
nagy.balint CreditAttribution: nagy.balint commentedFor us the patch also works fine.
Just a question:
If this is something you have to turn on, wouldnt it be possible to do the schema change only when the user turns this functionality on? And then remove the schema changes when the functionality turned off? That way it would have no effect on any previous site, not even a schema change?
Comment #108
maximpodorov CreditAttribution: maximpodorov commentedThe real solution is to do this in a separate module. :)
Comment #109
Cadila CreditAttribution: Cadila commentedThe right solution is to back in time and add this patch to ER before releasing it)
Comment #110
pfrenssen@nagy.balint, schema updates are performed by
db_add_field()
in update hooks. It has already been done hundreds of times without any negative side effects by a whole bunch of popular modules such Backup & Migrate, Drupal Commerce, Display Suite, Feeds, I18N, Migrate API, Panels, Search API, Views, Webform, ... It's really not a big deal.The only sites that will be impacted are sites that grossly violate the Drupal API, e.g. by querying the database tables directly. We can not and should not care about those. Also, they always have the option to simply not enable the functionality so they are totally not impacted by the change.
Comment #111
DamienMcKennaFYI saying that this shouldn't be included in ER because it isn't in D8 is not a valid reason, because D8 only dropped it because it wasn't in ER, it's a catch-22 problem.
How about continuing to improve the patch, identify any problems that arise and fixing them?
Comment #112
maximpodorov CreditAttribution: maximpodorov commentedPlease understand that schema change itself influences other modules, not a checkbox which enables something.
Comment #113
maximpodorov CreditAttribution: maximpodorov commented@DamienMcKenna, the question is unanswered: why not another module.
Comment #114
Cadila CreditAttribution: Cadila commented@maximpodorov, Cause it should be implementing over 9000 hooks! So it will cause the performance. I think this should be a new branch for ER or ER should be deprecated and will be replaced with new module
Comment #115
Leeteq CreditAttribution: Leeteq commentedI think @Cadila has a valid point about the error being made initially. And since this feature request was indeed, as @pfrenssen is pointing out, made by the maintainer, it is interesting to get @amitaibu's take on whether this originally/"really" belongs in ER or is ok in a separate module at this point, given the "bad start" and the current situation with so many sites depending on this now.
I am fine with either a) separate module, or b) completely new branch of ER, which obviously then needs (and will get) lots of testing.
I am just (very) opposed to committing it to the current branch which so many sites depend on and have possibly made adjustments for. The main reason beyond caring about not creating "unnecessary havoc" for others, is that this issue may hurt Drupal's overall reputation if something unforeseen creates havoc even for just a minority of the affected sites.
Comment #116
pfrenssen@Leeteq, if you find any problems please report them so we can address them.
Comment #117
Leeteq CreditAttribution: Leeteq commented@pfrenssen/#116; my point is quite the opposite; we should assume that because of the complexity involved, it is releatively likely that this might break lots of sites in ways we dont have time or resources to foresee. That is why I think it is best to put this - at least temporarily, in either a separate module or a new branch.
Comment #118
maximpodorov CreditAttribution: maximpodorov commented@pfrenssen, the real problem is not any incompatibility with other modules that can be fixed in this patch and other modules. The real problem is a field meaning change which is not a subject of contrib module understanding. I already mentioned the example of such meaning change: equality of field values (and many modules can be affected). In normal ER field, you always know that (within one entity type) 17 EQUALS 17 without any exceptions. These field values reference the same entities, so any code (contrib or custom) understands this clearly and knows how to treat this.
If the ER field will have two columns: (target_id, revision_id), things will change dramatically. No code will know whether (17, NULL), (17, 100), (17, 200) are the same or not the same references. It will become dependent on business logic of the site. In some situations, these references can be treated as the same, and in other situations they differ semantically. So the simple concept of field value equality will be broken.
Let me show two absurd examples. Imaging that Dries wants to modify number.module in Drupal 7.35 by adding the second column to the field schema so that number field could store not just numbers but intervals also. Will someone allow him to do this? The whole community will oppose him, and he will not be able to say: it's my project, I'll do what I want.
The second absurd example: boolean field which can be 0 or 1 (which means FALSE and TRUE). If 200000 sites use it and check it with empty(), it's OK, but what if someone will want to add the third value -1 (which means MAYBE). Will you allow him to do this and break all these sites?
Comment #119
pfrenssenThe patch doesn't enable revision tracking for any field. The revision_id column will not be populated. The current situation is that if you reference an entity you always get the latest revision. If the patch is applied, you still always get the latest revision. The field equality remains fully intact.
In your example two references are using different revisions (17, NULL) and (17, 100), but this is something that will never happen with this patch. They will still both reference the same revision.
Comment #120
miro_dietikerHm.. This update will result in a severe schema update that will lead to a required downtime of all sites using entity reference.
I don't see why all sites that are perfectly happy with entity reference in the current state should be forced to update the database.
D8 decided to drop this complexity and it simplified many things. Now contrib can take care of the revision requirement. Referencing to a target revision is not a minor option.
Entities are considered separated first class citizens and entity revision provides loose coupling of entities. IMHO core does not deal with an entity composition relationship and when looking at modules like field collections, we realise step by step the amount of complexity this direction creates.
In the test coverage, i see a bunch of gaps:
- Views integration (and option selection) is untested
- Reverting revisions is untested
- Deleting revisions is untested (some contribs do this)
- Multilingual situations are untested
- Entities might have a different revision than the latest revision set as current. This case is untested.
- No upgrade test
Note with testing the D8 module we ended with this issue: #2427711: Do not allow reference to entities without revision support
Problems when changing settings:
- If a field (lock, revision) setting is changed, existing views references need an update. They are still unchanged. This contains risk of trouble.
- If a reference is locked, there is no way in the UI to update the revision to the latest revision. No indicator about the outdated reference. The (reference and latest / active) target revision is not shown.
My conclusion: Referencing to a target with revision is a different concept. It should not be an option of entity reference. It should be a different field type.
Duplicate unset.
Comment #121
miro_dietikerOh, forgot, and if a field changes from revision to non-revision option with previous data submission, previous data is stale. It is unclear for me if this situation is autofixed on next save or if the previous revision id is staying persisted. Also not tested.
Comment #122
sophiejones CreditAttribution: sophiejones commentedHello all
First, thanks to all of you for contributing your time and expertise and making so many thing possible with drupal.
I do apologize for being blunt but I cannot believe this is held off because some people think it crazy concept which is not by the way.
I have two reason why this is important and must be in this module:
1. Dynamic World
2. Transparency
Ability to use 'locked revision' like this through ER, gives us an edge and a way to move along and some times even ahead in this dynamic world.
The concept of relative time and space is an important concept and this function will allow us to implement it. The usage of 'locked revision' gives such an endeavor creditability in a way that nothing else can to hold the attention of your audience, not to mention their respect.
But since you guys are concerned about databases instead of thinking out of the box and towards the future of this dynamic world with an eye on evolution of transparency, then is it possible to find a back way to do this through views and using views to filter what is available and to choose what revision. Will this work? Can ER in its current structure understand this???
Comment #123
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI guess given that https://www.drupal.org/project/entity_reference_revisions exists now and that the stance for 8.0.x is no, probably a port of this patch to a 7.x-1.x branch of that module should be done.
Comment #124
maximpodorov CreditAttribution: maximpodorov commentedThat would be the very solution of this discussion.
Comment #125
DamienMcKennaOf course, be aware that they haven't solved the language problem yet: #2491247: Point to a specific target language
Right now the D7 module is a locked up in the Paragraphs module.
Comment #126
miro_dietiker@DamienMcKenna: Please check my comment (from 2 months ago) over there and explain why this is a feasible requirement.
With entity translation, you refer to targets as an entity as a whole. With linking to a specific revision, you refer to a possibly outdated revision.
Referring to a specific target language (breaking the current language consistency in entity translation) is a completely different requirement / feature.
Comment #127
sophiejones CreditAttribution: sophiejones commentedHello
First I want to extend an apology to all the people who oppose this patch. I am an open-minded person, so I decided to test what they were saying and they are right.
Because of the current structure of entity module, many entity based module will not work with this patch implemented since it also requires a patch to entity module. and subsequently next thing you know, every single of these modules will require a patch and this is going down a slippery slope entirely.
Although I am pro for this concept because I believe it is not just a fine addition, but a necessity. But I also agree that it has to be a module of its own in such a way that will apply with conditions only.
I tested with some ECK entities as well as couple of other entity based module. Case in point is organic group that in order to get this work, will require a patch as will every other one.
So those of you who oppose this, Please accept my apology.
Regards
Comment #128
mpotter CreditAttribution: mpotter commentedI've been using this patch in Open Atrium successfully, so not sure what the specific issue is with organic groups. The ability to lock a reference to a revision has been super critical to several of our clients (as mentioned in #73 and #74). In our case, having this work with normal Entity Reference fields rather than using a different field type has been a nice advantage, although if https://www.drupal.org/project/entity_reference_revisions gets backported I can certainly look into using that for all of our entity reference needs.
Not sure what Damien meant in #125 by "Right now the D7 module is a locked up in the Paragraphs module.".
Comment #129
DamienMcKenna@mpotter: What I meant was that the "entity reference revisions" functionality is tied at the hip to the Paragraphs module, and there doesn't seem to be any interest in separating them.
Comment #130
NancyDruBased on what I read here, I think this satisfies my customer's request (pretty much the same as in #92, using IEF). But it would be helpful if there was an actual summary of the change at the top.
I did something like this in a custom entity and no other modules broke.
Please, let's either do this (I do like the new branch idea though) or kill it now. I need to either get it installed or start coding around it, and that's going to be a big effort.
Comment #131
DamienMcKennaI opened an issue in core to discuss this: #2812595: Support revision tracking of entity references in core
Comment #132
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedPatch no longer applies cleanly (dev version Oct 24, 2016) https://www.drupal.org/project/entityreference/releases/7.x-1.x-dev
patch -p1 < entityreference-1837650-76-referencing-specific-revision.patch
patching file entityreference.info
Hunk #1 FAILED at 22.
1 out of 1 hunk FAILED -- saving rejects to file entityreference.info.rej
patching file entityreference.install
Hunk #2 FAILED at 167.
1 out of 2 hunks FAILED -- saving rejects to file entityreference.install.rej
patching file entityreference.module
Hunk #1 succeeded at 250 (offset 7 lines).
Hunk #2 succeeded at 743 (offset 31 lines).
Hunk #3 succeeded at 759 (offset 31 lines).
Hunk #4 FAILED at 1298.
1 out of 4 hunks FAILED -- saving rejects to file entityreference.module.rej
patching file plugins/selection/EntityReference_SelectionHandler_Generic.class.php
patching file plugins/selection/EntityReference_SelectionHandler_Views.class.php
Hunk #1 succeeded at 88 (offset 20 lines).
patching file tests/entityreference.admin.test
patching file tests/entityreference.revisions.test
patching file views/entityreference.views.inc
Comment #133
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedReroll of #76 with new dev version of entityreference
Comment #134
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedsmall change
Comment #135
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedComment #136
Kristen PolThanks for the patch! I haven't tested it yet. I did look through the code and can't say I understand it all but I noticed some super minor things that I've listed below. Thanks!
Nitpick. Are there extra spaces before the *?
Nitpick: Looks like extra space in Dreditor.
Nitpick: Looks like extra space in Dreditor.
Nitpick: Looks like extra space in Dreditor.
Nitpick: Isn't this just one revision? Not sure why comment has (s) in revision(s).
Nitpick: IMO "probing" is a weird word. Perhaps "analysis" or something else is better.
Nitpick: Dreditor is showing some weird spaces or something (see pink area).
Nitpick: Indentation is off here.
Nitpick: Is indentation off?
Nitpick: Is indentation off?
Nitpick2: "one by-one" => change to "one by one".
Nitpick3: "revision-ID" => "revision ID"? (And perhaps in other places too.)
Nitpick: Dreditor is showing some weird spaces or something (see pink area).
Nitpick: Dreditor is showing some weird spaces or something (see pink area).
Nitpick: Dreditor is showing some weird spaces or something (see pink area).
Comment #137
Kristen PolIgnore this. Drupal.org was hanging and ended up with a double comment posted.
Comment #138
maximpodorov CreditAttribution: maximpodorov commentedWhy do you continue to taint the agelong established module instead of working on a separate specialized project (https://www.drupal.org/project/entity_reference_revisions) ?
Comment #139
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI tend to agree that (as already stated in the core issue) the best approach is to move the functionality for D7 into a separate field type / project instead as well.
The use cases are very different of what an Entity Reference tries to achieve (independent, re-usable, own workflow) and what ERR tries to achieve (only editable embedded, bound to parent, not re-usable (but cloneable), same workflow as parent, etc.).
In that regard ERR is much more similar to field collection than to ER itself.
And ERR entities would for example also need to opt-out of workflows with drafty / CPS / workbench etc.
Therefore a completely separate field type feels architecturally best to me.
Comment #140
geek-merlinTotally agree with #139 and dare to set wontfox.
I hope we can join forces for ERR then.
Also note that we have a usable D7 ERR field inside the paragraphs module.
Comment #141
a.milkovskyThere are still ongoing old projects, which require support and security updates.
Comment #142
a.milkovskyIn case somebody needs this, I've re-rolled #134.
Comment #143
mpotter CreditAttribution: mpotter at Phase2 commentedThanks for the reroll! Saved me some work in the Open Atrium update this morning. (if somebody ever wants to update Atrium to use a different module for this functionality, feel free, but I just need to keep up with security updates).
Comment #144
xlin CreditAttribution: xlin as a volunteer commentedRe-roll #142 against #75.
Comment #145
xlin CreditAttribution: xlin as a volunteer commentedStay away from the patch in #144 unless you have same use case mentioned in #69, #70.
Comment #146
xlin CreditAttribution: xlin as a volunteer commentedUpdate patch on #144. Again, ONLY use this patch if you have use case mentioned in #69, #70.
Comment #147
delacosta456 CreditAttribution: delacosta456 commentedhi
Please is it necessary to have the dev version of Entity reference module for any of those patch to work , because when i apply any of them to non-dev version i am getting error when saving the parent's node A that enmbed child node B through Inline Entity Form
Thanks
Comment #148
delacosta456 CreditAttribution: delacosta456 commentedhi
Appplying the patch is giving me notice message
Notice: Undefined property: stdClass::$field_qualif_assoc_prof_po_revision_id in field_sql_storage_field_storage_load() (line 426 of /Applications/MAMP/htdocs/brainrhdgddi/modules/field/modules/field_sql_storage/field_sql_storage.module).
Comment #149
maximkashubaUpdated patch #75 for the stable module version 1.5 and development 1.x-dev
Comment #150
maximpodorov CreditAttribution: maximpodorov commentedMaxim, if the doctor said "to mortuary", this means "to mortuary".
Comment #151
seirov CreditAttribution: seirov commentedHello everyone! I have applied this patch (#149) and I think there is a bug, related with the already-discussed extra columns :)
I have a content type with several child entity reference fields: one referencing nodes, other a taxonomy terms and the last one a list of users. For nodes, patch works perfectly; but for the other two it fails when trying to save a new node of the parent content type. It launches the following error:
So, I added columns manually in the database (which make me feel quite uncomfortable) in tables field_data_field_myfield and field_revision_field_myfield. By the moment it works, but I think that these columns should be created for all entity references when patching (not only those for nodes), even they won't be used as there is no revision for terms or users.