This feature doesn't have a UI. It's already implemented in the 8.x branch.

Files: 
CommentFileSizeAuthor
#76 interdiff-1837650-71-76.txt723 byteslwalley
#76 entityreference-1837650-76-referencing-specific-revision.patch23.75 KBlwalley
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
#75 entityreference-1837650-75-referencing-specific-revision.patch23.72 KBlwalley
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
#71 entityreference-n1837650-47_0.patch23.71 KBfearlsgroove
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
#70 interdiff-1837650-47-70.txt1 KBlwalley
#70 entityreference-1837650-70-referencing-specific-revision.patch23.67 KBlwalley
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
#53 entityreference-n1837650-47.patch23.71 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]

Comments

amitaibu’s picture

Status:Active» Needs review
StatusFileSize
new4.79 KB
PASSED: [[SimpleTest]]: [MySQL] 114 pass(es).
[ View ]

We still need to deal with entityreference_field_property_callback() - we might need to write our own getter callback.

alexh’s picture

Hi,
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

amirdt22’s picture

Also interested in storing revisions.

DamienMcKenna’s picture

Is 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?

DamienMcKenna’s picture

FYI the patch still applies cleanly to the current 7.x-1.x codebase.

DamienMcKenna’s picture

I've confirmed that when the revision field is assigned a specific value that the correct revision of the referenced content *does* load.

DamienMcKenna’s picture

DamienMcKenna’s picture

StatusFileSize
new8.31 KB
PASSED: [[SimpleTest]]: [MySQL] 119 pass(es).
[ View ]

This 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.

Albert Volkman’s picture

StatusFileSize
new840 bytes
new8.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entityreference-n1837650-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updating reference to variable location.

DamienMcKenna’s picture

@Albert: Thanks, I forgot that I moved the setting around and it became a field-level setting.

Status:Needs review» Needs work

The last submitted patch, entityreference-n1837650-9.patch, failed testing.

Albert Volkman’s picture

StatusFileSize
new672 bytes
new8.33 KB
PASSED: [[SimpleTest]]: [MySQL] 119 pass(es).
[ View ]

Wrong patch file, my apologies.

Albert Volkman’s picture

Status:Needs work» Needs review
DamienMcKenna’s picture

StatusFileSize
new7.31 KB
PASSED: [[SimpleTest]]: [MySQL] 119 pass(es).
[ View ]

The 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.

Jelle_S’s picture

StatusFileSize
new9.06 KB
PASSED: [[SimpleTest]]: [MySQL] 119 pass(es).
[ View ]

Patch 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.

Jelle_S’s picture

StatusFileSize
new9.16 KB
PASSED: [[SimpleTest]]: [MySQL] 119 pass(es).
[ View ]

Small fix: previous patch gave an error when saving new entities with an entityreference field.

rylowry’s picture

The patch in #16 seems to be working for me.

Jelle_S’s picture

StatusFileSize
new12.19 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

This 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.

Jelle_S’s picture

StatusFileSize
new17.11 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Small fix for when the referenced entity no longer exists.

Jelle_S’s picture

StatusFileSize
new18.56 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Views support added.

Jelle_S’s picture

StatusFileSize
new13.68 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Sorry, patch from an other issue got mixed in with the previous one.

Jelle_S’s picture

StatusFileSize
new14.1 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Reroll

spatical’s picture

spatical’s picture

Issue summary:View changes
StatusFileSize
new10.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entityreference-reference_revisions-1837650-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

For 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.

Status:Needs review» Needs work

The last submitted patch, 24: entityreference-reference_revisions-1837650-24.patch, failed testing.

Cadila’s picture

Working 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

<?php
// find the original item, which does not have the same delta per se.
     
$original_revision = FALSE;
      foreach (
$original_items as $original_item) {
        if (
$original_item['target_id'] == $val['target_id']) {
         
$original_revision = isset($original_item['revision_id']) ? $original_item['revision_id'] : FALSE;
        }
      }
      if (isset(
$ref_ids[1]) && is_numeric($ref_ids[1]) && !$original_revision) {
       
$items[$key]['revision_id'] = $ref_ids[1];
      }
      else if (
$original_revision) {
       
$items[$key]['revision_id'] = $original_revision;
      }
?>

Why do we need this? Maybe it will be better just to put this away?

Cadila’s picture

StatusFileSize
new10.33 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Adding patch from #24 with fixed paths to make it more green!

Cadila’s picture

Status:Needs work» Reviewed & tested by the community

I've just checked, latest patch working just fine! (for a while maybe)

Cadila’s picture

Status:Reviewed & tested by the community» Needs review

Actually 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!

<?php
/**
   * Overridden.
   *
   * @param $options
   *   An array of options. Known keys:
   *   - identifier: If set to TRUE, the entity identifier is returned.
   */
 
public function value(array $options = array()) {
   
// Try loading the data via the getter callback if there is none yet.
   
if (!isset($this->data)) {
     
$this->setEntity(parent::value());
    }
    if (!empty(
$options['identifier'])) {
      return
$this->id;
    }
    elseif (!
$this->data && !empty($this->id)) {
     
// Lazy load the entity if necessary.
     
$return = entity_load($this->type, array($this->id)); // id = {0 => {id => 123, vid => 123}}
      // In case the entity cannot be loaded, we return NULL just as for empty
      // properties.
     
$this->data = $return ? reset($return) : NULL;
    }
    return
$this->data;
  }
?>

Pay attention to the entity_load call, it called without conditions! Just ids passed, and what happens next in DrupalDefaultEntityController:

<?php
/**
   * Implements DrupalEntityControllerInterface::load().
   */
 
public function load($ids = array(), $conditions = array()) {
   
$entities = array();

   
// Revisions are not statically cached, and require a different query to
    // other conditions, so separate the revision id into its own variable.
   
if ($this->revisionKey && isset($conditions[$this->revisionKey])) { //Here is it! it looks for revisionKey in conditions
     
$revision_id = $conditions[$this->revisionKey];
      unset(
$conditions[$this->revisionKey]);
    }
    else {
     
$revision_id = FALSE;
    }
?>

So I hope that this is entity module's bug cause it provides MetadataWrappers and I will create an issue for entity module.

DamienMcKenna’s picture

@cadila: Shouldn't #1788568: entity_metadata_wrapper does not load correct revisions resolve the problem with EntityDrupalWrapper though?

Cadila’s picture

Yes, it solved the problem. The latest patch for this issue works good!

pfrenssen’s picture

Issue tags:+Needs tests

Can 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 :)

Cadila’s picture

The 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

pfrenssen’s picture

Assigned:amitaibu» pfrenssen
Status:Needs review» Needs work

I'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.

pfrenssen’s picture

StatusFileSize
new1.16 KB
new1.89 KB
new10.56 KB
new6.75 KB

Here are the missing interdiffs.

pfrenssen’s picture

Going 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.

pfrenssen’s picture

StatusFileSize
new14.1 KB

Rerolled #22 against latest HEAD.

pfrenssen’s picture

StatusFileSize
new14.45 KB
new3.14 KB

Started with some cleanups, renamed variables so they match with the rest of the module. Added some documentation.

pfrenssen’s picture

Status:Needs work» Needs review
StatusFileSize
new18 KB
PASSED: [[SimpleTest]]: [MySQL] 131 pass(es).
[ View ]
new9.65 KB

Further 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:

--- a/tests/entityreference.admin.test
+++ b/tests/entityreference.admin.test
@@ -113,7 +113,8 @@ class EntityReferenceAdminTestCase extends DrupalWebTestCase {
     $this->drupalPost(NULL, array(), t('Save settings'));

     // Check that the field appears in the overview form.
-    $this->assertFieldByXPath('//table[@id="field-overview"]//td[1]', 'Test label', t('Field was created and appears in the overview page.'));
+    $elements = $this->xpath('//table[@id="field-overview"]//td[text()="Test label"]');
+    $this->assertTrue($elements, 'Field was created and appears in the overview page.');

pfrenssen’s picture

StatusFileSize
new20.81 KB
FAILED: [[SimpleTest]]: [MySQL] 132 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new3.87 KB

Started working on a test. Also included this as an experiment to figure out why the above is failing locally but not on the bot:

17 --- a/tests/entityreference.admin.test
18 +++ b/tests/entityreference.admin.test
19 @@ -113,7 +113,13 @@ class EntityReferenceAdminTestCase extends DrupalWebTestCase {
20      $this->drupalPost(NULL, array(), t('Save settings'));
21
22      // Check that the field appears in the overview form.
23 -    $this->assertFieldByXPath('//table[@id="field-overview"]//td[1]', 'Test label', t('Field w    as created and appears in the overview page.'));
24 +    // $this->assertFieldByXPath('//table[@id="field-overview"]//td[1]', 'Test label', t('Fiel    d was created and appears in the overview page.'));
25 +    $elements = $this->xpath('//table[@id="field-overview"]//td');
26 +    /* @var $element SimpleXMLElement */
27 +    foreach ($elements as $element) debug((string) $element);
28 +
29 +    $elements = $this->xpath('//table[@id="field-overview"]//td[text()="Test label"]');
30 +    $this->assertTrue($elements, 'Field was created and appears in the overview page.');

This is only set to Needs Review to get the bot's feedback. This still needs work. The new test will probably fail.

Status:Needs review» Needs work

The last submitted patch, 40: entityreference-n1837650-40.patch, failed testing.

pfrenssen’s picture

The 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.

pfrenssen’s picture

StatusFileSize
new22.51 KB
new4.66 KB

Worked some more on the test.

pfrenssen’s picture

StatusFileSize
new23.06 KB
new5.41 KB
  • Reworked the test to not use entity metadata wrappers. I tried the patch in #1788568: entity_metadata_wrapper does not load correct revisions but hit a problem: the wrapper caches the entity but doesn't clear the cache when the entity is updated, causing stale revisions to be referenced.
  • Removed the code that tries to retrieve the unchanged entity from the database when the $entity->original property is empty. I did some research and the Entity API module itself trusts that this property is set (e.g. in EntityAPIController::save()), and I checked all core entities and they all support this property, except the User entity which does not support revisioning. @Jelle_S, do you remember if there was a particular reason you put this fallback in?
pfrenssen’s picture

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new23.63 KB
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
new4.38 KB

The 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.

pfrenssen’s picture

A few questions:

  1. What is the intended behaviour if a field references the same entity multiple times? With the current implementation they will all point to the same revision after saving the entity.
  2. What is the intended behaviour if entities were being referenced, but this option is subsequently disabled? Currently the reference will keep tracking the revision. I guess that it would be better to clear the revision after the entity is resaved, that would better match site builder's expectations.
pfrenssen’s picture

StatusFileSize
new23.71 KB
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
new3.71 KB

So 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.

Cadila’s picture

Chuvak, skolko mozhno uzhe? Davai dopilivay normalno!

nagy.balint’s picture

Seems 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.

fschmitt’s picture

Hi 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

Cadila’s picture

StatusFileSize
new33.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entityreference-n1837650-51.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Latest patch failed a little to apply on new entityreference version. I made some changes to in order to make it apply correctly.

Status:Needs review» Needs work

The last submitted patch, 51: entityreference-n1837650-51.patch, failed testing.

pfrenssen’s picture

Status:Needs work» Needs review
StatusFileSize
new23.71 KB
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]

The 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.

maximpodorov’s picture

This 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!

DamienMcKenna’s picture

@maximpodorov: The patch makes it optional, and disabled by default, thus would be fine for existing sites.

maximpodorov’s picture

No, it creates new column in the field tables. To be fully compatible, it should use another tables.

maximpodorov’s picture

If 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.

pfrenssen’s picture

@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?

maximpodorov’s picture

It's irresponsible to break 160000 sites by this incompatible change.

DamienMcKenna’s picture

@maximpodorov: This is not a compatibility change until you enable the new option. Having the extra field doesn't make any difference by default.

Cadila’s picture

I 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.

maximpodorov’s picture

Think 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.

pfrenssen’s picture

@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:

<?php
function entityreference_field_schema($field) {
  if (
$field['type'] == 'entityreference') {
    ...
   
// Invoke the behaviors to allow them to change the schema.
   
foreach (entityreference_get_behavior_handlers($field) as $handler) {
     
$handler->schema_alter($schema, $field);
    }
    return
$schema;
  }
}
?>

Do you have some public code that is affected by this that you can show us? We can help finding a solution.

maximpodorov’s picture

@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

pfrenssen’s picture

That 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 use field_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.

maximpodorov’s picture

Do you mean field value equality is equality of all required properties of the field?

pfrenssen’s picture

This is offtopic for this issue, let's take this discussion to #1630820: entity_field_value is completely broken.

maximpodorov’s picture

Drupal 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.

lwalley’s picture

Taking 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:

  1. There are two node types A and B. Node type A contains an entityreference field that references nodes of type B.
  2. Anytime a node of type A is edited and saved it should create a new revision of both A and B nodes and the entityreference field in A should point to the newly created revision of B.

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!

lwalley’s picture

StatusFileSize
new23.67 KB
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
new1 KB

Attached 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).

fearlsgroove’s picture

StatusFileSize
new23.71 KB
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]

I'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.

nagy.balint’s picture

We are using patch #47 in a project, and have not encountered any issue.

mpotter’s picture

I 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!

mpotter’s picture

I 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!

lwalley’s picture

StatusFileSize
new23.72 KB
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]

I 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).

lwalley’s picture

StatusFileSize
new23.75 KB
PASSED: [[SimpleTest]]: [MySQL] 138 pass(es).
[ View ]
new723 bytes

Fix for missing entity applied to patch from #71 (the current patch for this issue).

LGLC’s picture

The Patch at #76 works great for me!

pfrenssen’s picture

@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.

pfrenssen’s picture

Issue tags:-Needs tests

Removing tag, I've already added tests for this half a year ago :)

maximpodorov’s picture

Status:Needs review» Needs work

Again, 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.

fearlsgroove’s picture

Status:Needs work» Needs review

@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.

DamienMcKenna’s picture

FYI there's also the Entity Reference Revisions.

maximpodorov’s picture

If 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.

catch’s picture

I'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.

DamienMcKenna’s picture

@catch: CPS is relatively new, this issue is over two years old.

catch’s picture

@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.

LGLC’s picture

Status:Needs review» Needs work

Ah, 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:

  1. Add an entity reference field called field_article_reference (that references Articles) to the Basic Page content type.
  2. Add a dummy article, then a dummy page that references that article.
  3. Create a new View with table display, filtered to the Page content type.
  4. Add in the field for field_articile_reference.
  5. Now switch aggregation on.
  6. An SQL error will appear.
  7. Re-save the aggregation settings of that field and the error will go away.

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.

fearlsgroove’s picture

I 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.

fearlsgroove’s picture

@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.

catch’s picture

@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:

<?php
 
$cache_read
= $load_current && $info['field cache'] && empty($options['deleted']);
?>

it also reverts to the revision of the referenced entity specified by that node revision.

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.

fearlsgroove’s picture

@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 but when 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?

pfrenssen’s picture

My 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.

maximpodorov’s picture

If you need this functionality, you can create a module which implements it.

DamienMcKenna’s picture

What is the status on the D8 functionality that the issue description states was already committed, given entity_reference is in core?

fearlsgroove’s picture

Looks 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.

Leeteq’s picture

This 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:

"- The behavior (field display - if the user doesn't have access to previous/unpulblished revisions, or views relationships) is still largely unclear / TBD
- No core widget supports referencing a specific revision anyway.
- Core has no use for it
- This really doesn't feel like a "80%" (nor 95%, for that matter) use case in terms of "data modeling toolbox for end users.
- The concept was never actually battle tested since it wasn't part of entityreference D7.
- I see no technical reason for which it *has* to be done in core native's reference field type.

This is an unfinished piece of wishful thinking, we should remove it from core and leave it to contrib to build a solid "reference a specific revision" field type."

Leeteq’s picture

(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 )

catch’s picture

OK I understand pfrenssen's use case, that's an interesting one if quite specific.

catch’s picture

OK I understand pfrenssen's use case, that's an interesting one if quite specific.

pfrenssen’s picture

@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.

maximpodorov’s picture

@pfrenssen, why not creating another module? What is the reason to include this functionality in ER?

Leeteq’s picture

Yes, 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.

Cadila’s picture

No! 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!

maximpodorov’s picture

@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.

pfrenssen’s picture

This 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.

maximpodorov’s picture

You can't address it if the field schema is changed. The only way is not to change field schema on 200000 sites.

nagy.balint’s picture

For 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?

maximpodorov’s picture

The real solution is to do this in a separate module. :)

Cadila’s picture

The right solution is to back in time and add this patch to ER before releasing it)

pfrenssen’s picture

@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.

DamienMcKenna’s picture

FYI 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?

maximpodorov’s picture

Please understand that schema change itself influences other modules, not a checkbox which enables something.

maximpodorov’s picture

@DamienMcKenna, the question is unanswered: why not another module.

Cadila’s picture

@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

Leeteq’s picture

I 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.

pfrenssen’s picture

@Leeteq, if you find any problems please report them so we can address them.

Leeteq’s picture

@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.

maximpodorov’s picture

@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?

pfrenssen’s picture

The 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.

miro_dietiker’s picture

Hm.. 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.

+++ b/entityreference.module
@@ -1172,42 +1267,67 @@ function entityreference_field_formatter_settings_summary($field, $instance, $vi
         unset($items[$id][$delta]);
         $rekey = TRUE;
+        unset($items[$id][$delta]);

Duplicate unset.

miro_dietiker’s picture

Oh, 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.

sophiejones’s picture

Hello 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???

Fabianx’s picture

I 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.

maximpodorov’s picture

That would be the very solution of this discussion.

DamienMcKenna’s picture

Of 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.

miro_dietiker’s picture

@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.

sophiejones’s picture

Hello
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