Support revisions in Entity API

Tagging.

Edit: er, I guess the Entity CRUD API - main component no longer exists, so it defaulted to Core integration. Apologies if it's supposed to be in a different component.

Marked #1113032: EntityAPIController's save() method does not appear to even try to save revisions? as a duplicate of this issue.

Hi, I'm planning to start working on this feature (I did a quick hack a few months ago to test this feature, and it seems like we want to go with Entity API for RedHen CRM). It seems you've outlined a basic plan already, if I were to start work on this, would you like to see patches in this issue, or links to a sandbox on d.o or github?

Personally, I'd lean towards making all properties on an entity revisioned for now. I didn't know that Entity API handled schema creation for it's entities. I 'm not sure about making all the revision table schema match the base table, but it seems like that would be the simplest approach for now.

Any other considerations before starting work on this?

We are going to need this, so here is a starter patch:

• I merged revision support directly into EntityAPIController. The changes are actually very limited, so it doesn't feel necessary to have a sub-controller for this;
• I followed what the Node module does: a new revision is created only when $entity->revision is true;
• I haven't bothered creating the revision table automatically, it seems that the job of the module creating the entity (and we don't have a hook_schema_alter() that affects creating tables);
• I extended the tests.

Still missing is adding support for deleting revisions.

The patch in #3 didn't apply anymore, so here is a plain re-roll of it. Unfortunately I don't have time to address the issues mentioned in #4 by fago.

But, the patch works perfectly for me so far. I will try to find time checking back on this, so we can get this in, would be awesome!

There needs to be a way for implementations to shuffle properties around before saving the revision record. The specific use case, when following the node paradigm, would be that "created" and "changed" on the node are simply "timestamp" on the revision, and that we want to track the revision "uid" separately from the node's author. Such examples are likely to be common even in non-node situations.

I'd prefer a solution where a preSaveRevision method is added to the controller class, as that's more convenient to implement than a hook imo.

Attached patch builds on #6, adding support for a protected (overridible) method on the controller that allows base classes to perform some preparation on the revision record before it's written. Specifically this would support the use case of users in node, where you'd like the UID of the revision to be set, but not change the UID of the core record.

It also adds support for properly deleting entities. The previous patch left orphaned revision records. Both scenarios probably need tests.

Two examples of overriding the saveRevision method in the entity controller:

Sets the UID of the revision object based on a separately set property that's not an actual schema field:

  protected function saveRevision($entity) {
    if (isset($entity->revision_uid)) {
      $entity->uid = $entity->revision_uid;
    }
    return parent::saveRevision($entity);
  }

.. or based on the current user:

  protected function saveRevision($entity) {
    global $user;
    $entity->uid = $user->uid;
    return parent::saveRevision($entity);
  }

edit: use php instead of code

Can I have revision support exclusively for the embedded field collection with this patch in #7?

If yes, any extra steps for accomplishing it?
If no, what still needs to be done for this?

@Fidelix: Field collection module would need to be updated with support for revisions. this patch doesn't enable revision support automatically for all entities that use entity api, it just enables entities to fairly easily support revisions if they want to. Not likely any modules will get revision support until this or something similar actually lands. I'm sure it'd be quite welcome in some fairly big modules (commerce)

subscribing

@wildkatana, Stop subscribing, start following
http://drupal.org/node/1306444

Please make sure that entity revisions are implemented in such a way that the techniques being employed to moderate content—by modules like Revisioning, Workbench Moderation and State Machine—can be used to moderate other entity types as well. They should be able to separate the "current" revision from the "latest" revision. My own understanding of how this all works is minimal, so please forgive me if this request is out of place.

Oops. Removing random tag.

I did a follow, but updating issue title so it more informative when seen in the Dashboard activity display.

Here is rerolled patch based on #7 with some changes.

I have implemented according to comments of #4 following items:
- entity_revision_delete()
- renamed 'revision' key to 'is_new_revision'
- expanded tests (there are not much good examples of tests in node module about revisions)
- removed $entity->old_revision_id as it never used
- added key $entity->set_active_revision that set revision to be active on $entity->save()
- small update of documentation

@fago, I don't understand your comment about $entity->isRevision(). Could you please explain in more details what do you mean this method should do? And when we should use it?

@fago thank you very much for the review!

• Fixed doxygen comments.
• Renamed entity_delete_revision to entity_revision_delete
• Renamed deleteRevision to revisionDelete
• I do not understand what means 'We should add a "delete revision" callback support analogously to entity_delete().' In controller revisionDelete() we invoke 'revision_delete' hooks.
• Updated entity_type_supports()
• Don't understand what 'node module support' we should add here.
• Hook invokation, renamed 'delete_revision' to 'revision_delete' and made exception for field attacher only.
• Don't understand comment about 'covering field api'.
• Controller save method is built according to node_save, so it updates main table after creating revision if it is needed. So everything looks consistent to me regarding 'Why not save the revision first to avoid having this extra-update?'
• I would suggest to keep saveRevision method protected as we would like new revisions created using save method with setting additional keys: is_new_revision and set_active_revision. Please see _node_save_revision() it is not really meant to be used directly.
• I have implemented isCurrentRevision() and added test for it. It does direct query to database to check whether revision is current.

Thanks for this. I tested this out with a module I'm working on, and it seems to be doing the job for now. Proceeding with caution however.

Also, the revisions for a body field on the entity I'm using it with seems to be saving correctly. I'm going to set something up with my test module tomorrow for restoring revisions. I don't foresee any problems.

I've run into a problem with custom entities with the organic groups group field. OG tries to load the newly created entity when its insert hook is invoked, but can't. Would you say this is a problem with OG or with this patch? Not sure what should be happening here to make this work. Should we be caching the newly created entity? Or can the invoke be moved outside the transaction?

Lines 432 through 441 of entity.controller.inc:

      if (empty($entity->{$this->idKey}) || !empty($entity->is_new)) {
        // For new entities, create the row in the base table, then save the
        // revision.
        $return = drupal_write_record($this->entityInfo['base table'], $entity);
        if (!empty($this->revisionKey)) {
          $this->saveRevision($entity);
          $update_base_table = (isset($entity->set_active_revision)) ? $entity->set_active_revision : TRUE;
        }
        $this->invoke('insert', $entity);
      }

@trevjs, lets not mix this issue with integration with OG issue. Could you please open a new issue for this? I would love to investigate so can you please also publish module that creates your custom entity and write instructions how to reproduce the problem.

Patch #20 applies cleanly, although I havent seen any instant effects. Will have to test integration on top of other modules.

Just a quick pass over and all I have to note is that there are a lot of whitespace fixes that are unrelated to this issue. Although if fago doesn't mind then it's irrelevant.

There are also a couple of indentation issues:

+++ b/includes/entity.controller.inc
@@ -359,6 +375,34 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+      db_delete($this->entityInfo['revision table'])
+          ->condition($this->revisionKey, $revision_id)
+          ->execute();

+++ b/tests/entity_feature.module
@@ -30,3 +30,29 @@ function entity_feature_default_entity_test_type() {
+  $types['main'] = entity_create('entity_test_revision_type', array(
+      'name' => 'main',
+      'label' => t('Main test type'),
+      'weight' => 0,
+      'locked' => TRUE,
+  ));
...
+  $types['test2'] = entity_create('entity_test_revision_type', array(
+      'name' => 'test2',
+      'label' => 'label2',
+      'weight' => 2,
+  ));

I will have a proper test on a website sometime in the next day or so and give proper feedback then.

I threw together a quick patch for profile2 to go along with this and got revisions to work with no errors or anything yet.

It is very basic and I have not tested much yet but I'll continue tomorrow.
The patch is at #1043128: Profile revisions

+++ b/entity.moduleundefined
@@ -96,6 +97,7 @@ function entity_type_supports($entity_type, $op) {
  *   The ID of the entity to load, passed by the menu URL.
  * @param $entity_type
  *   The type of the entity to load.
+ *
  * @return
  *   A fully loaded entity object, or FALSE in case of error.
  */
@@ -215,6 +217,7 @@ function entity_delete($entity_type, $id) {

@@ -215,6 +217,7 @@ function entity_delete($entity_type, $id) {
  * @param $ids
  *   An array of entity ids of the entities to delete. In case the entity makes
  *   use of a name key, both the names or numeric ids may be passed.
+ *
  * @return
  *   FALSE if the given entity type isn't compatible to the CRUD API.

It would be easier to review this patch if you didn't fix all kinds of stuff that doesn't belong to this issue with it :-).

+++ b/includes/entity.controller.incundefined
@@ -319,6 +327,8 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
    *
+   * Delete entity with all its revisions.

Should read "Deletes an entity and all its revisions".

+++ b/includes/entity.controller.incundefined
@@ -359,6 +375,34 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
 
   /**
+   * Delete entity revision.
+   *
+   * @param integer $revision_id
+   *   Revision ID.
+   *
+   * @return boolean
+   *   TRUE in case of success.
+   */
+  public function revisionDelete($revision_id) {
+    if ($entity_revisions = $this->load(FALSE, array($this->revisionKey => $revision_id))) {
+      $entity_revision = reset($entity_revisions);
+      // Prevent deleting the current revision.
+      $entities = $this->load(array($entity_revision->{$this->idKey}));
+      $entity = reset($entities);
+      if ($entity->{$this->revisionKey} == $revision_id) {
+        return FALSE;
+      }
+
+      db_delete($this->entityInfo['revision table'])
+          ->condition($this->revisionKey, $revision_id)
+          ->execute();
+
+      $this->invoke('revision_delete', $entity_revision);
+    }
+    return FALSE;
+  }
+
+  /**
    * Implements EntityAPIControllerInterface.

@@ -377,19 +421,53 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
       }

Should read "Deletes an entity revision."

Let's remove the "integer" part (@param integer ...) as we don't do that anywhere else either :).

The @return is also not correct. a) There is no "return TRUE;" at all and b) It could be a little more descriptive like: "TRUE if the entity revision could be deleted, FALSE otherwise." or something like that.

Also, I think that it would make sense to return TRUE even if no entity revisions could be loaded because technically that would mean that the entity revision with that id never existed or has already been deleted which is actually what we want to achieve... But that is just my 2 cent :).

+++ b/includes/entity.controller.incundefined
@@ -377,19 +421,53 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
-        $this->invoke('update', $entity);
+      // When saving a new revision, unset any existing revision ID so as to
+      // ensure that a new revision will actually be created, then store the old
+      // revision ID in a separate property for use by hook implementations.
+      if (!empty($this->revisionKey) && empty($entity->is_new) && !empty($entity->is_new_revision) && !empty($entity->{$this->revisionKey})) {
+        unset($entity->{$this->revisionKey});

Okay, good idea... But where is that separate property? :)

+++ b/includes/entity.controller.incundefined
@@ -400,6 +478,28 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+  protected function saveRevision($entity) {
+    if (!empty($entity->is_new_revision) || (isset($entity->is_new) && $entity->is_new)) {
+      drupal_write_record($this->entityInfo['revision table'], $entity);
+      return TRUE;
+    }
+    else {
+      drupal_write_record($this->entityInfo['revision table'], $entity, $this->revisionKey);
+    }

This one needs a documentation block.

+++ b/tests/entity_feature.moduleundefined
@@ -30,3 +30,29 @@ function entity_feature_default_entity_test_type() {
+  $types['main'] = entity_create('entity_test_revision_type', array(
+      'name' => 'main',
+      'label' => t('Main test type'),
+      'weight' => 0,
+      'locked' => TRUE,
+  ));
+
+  // Types used during CRUD testing.
+  $types['test'] = entity_create('entity_test_revision_type', array(
+    'name' => 'test',
+    'label' => 'label',
+    'weight' => 0,
+  ));
+  $types['test2'] = entity_create('entity_test_revision_type', array(
+      'name' => 'test2',
+      'label' => 'label2',
+      'weight' => 2,
+  ));

The indentation is wrong here (in 'main' and 'test2').

+++ b/tests/entity_test.installundefined
@@ -121,6 +121,35 @@ function entity_test_schema() {
+    'default' => NULL,
+    'description' => "The current revision ID of the entity.",

Lets use single quotes here.

+++ b/tests/entity_test.installundefined
@@ -121,6 +121,35 @@ function entity_test_schema() {
+    'default' => NULL,
+    'description' => "The ID of the attached entity.",

Here too.

+++ b/tests/entity_test.moduleundefined
@@ -43,6 +43,40 @@ function entity_test_entity_info() {
+      // Make use the class' label() and uri() implementation by default.

Should read "Make use of the class label ..." (remove single quote and add a 'of').

Rerolled patch based on #20 to get patch to work with drush make again and with some changes based on fubhy review.

• Changed "Delete entity with all its revisions." to "Deletes an entity and all its revisions"
• Changed "Delete entity revision." to "Deletes an entity revision."
• Removed integer from documentation block and changed return comment to "TRUE if the entity revision could be deleted, FALSE otherwise."
• Fixed indentation in entity_feature.module
• Changed "Make use the class' label() and uri() implementation by default." to "Make use of the class label() and uri() implementation by default."

+++ b/README.txtundefined
@@ -23,58 +23,58 @@ you may stop reading now.
     necessary metadata is available. The module comes with integration for all
     core entity types, as well as for entities provided via the Entity CRUD API
     (see below). However for any other entity type implemented by a contrib
-    module, the module integration has to be provided the contrib module itself. 
+    module, the module integration has to be provided the contrib module itself.
 
   * Thus the module provides API functions like entity_save(), entity_create(),
-    entity_delete(), entity_view() and entity_access() among others. 
+    entity_delete(), entity_revision_delete(), entity_view() and entity_access()
+    among others.
     entity_load(), entity_label() and entity_uri() are already provided by
     Drupal core.
 
  *  For more information about how to provide this metadata, have a look at the
     API documentation, i.e. entity_metadata_hook_entity_info().
-    

Even though there obviously are whitespace issues currently we shouldn't incorporate the fixes for that in this patch. Let's do that in a separate issue. This is only one short excerpt of the whitespace issues that you are fixing in this patch but you should remove all of those fixes. Please put them into a separate patch in a separate issue. :)

+++ b/entity.moduleundefined
@@ -234,6 +237,24 @@ function entity_delete_multiple($entity_type, $ids) {
 /**
+ * Deletes an entity revision.
+ *
+ * @param $entity_type
+ *   The type of the entity.
+ * @param integer $revision_id
+ *   The revision ID to delete.
+ */

@return is missing in the PHPDoc block. Please also remove the "integer" declaration for @param $revision_id, we don't use that usually, as mentioned before.

+++ b/includes/entity.controller.incundefined
@@ -286,7 +286,15 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+    $field_attach_hook = $hook;
+    if ($hook == 'revision_delete') {
+      $field_attach_hook = 'delete_revision';
+    }
+    if (!empty($this->entityInfo['fieldable']) && function_exists($function = 'field_attach_' . $field_attach_hook)) {

Why do you create a new variable $field_attach_hook here? Just keep using $hook. Like so:

$hook = $hook == 'revision_delete' ? 'delete_revision' : $hook;

+++ b/tests/entity_feature.moduleundefined
@@ -23,9 +23,35 @@ function entity_feature_default_entity_test_type() {
-      'name' => 'test2',
-      'label' => 'label2',
-      'weight' => 2,
+    'name' => 'test2',
+    'label' => 'label2',

This is also unrelated to this issue. Maybe incorporate that in the same issue / patch for fixing the whitespace issues?

+++ b/tests/entity_test.moduleundefined
@@ -22,7 +22,7 @@ function entity_test_entity_info() {
       ),
-      // Make use the class' label() and uri() implementation by default.
+      // Make use of the class label() and uri() implementation by default.

Ok... You are fixing it here (which is unrelated to the patch)... But...

+++ b/tests/entity_test.moduleundefined
@@ -45,6 +45,40 @@ function entity_test_entity_info() {
+      ),
+      // Make use the class' label() and uri() implementation by default.
+      'label callback' => 'entity_class_label',

... You are not fixing it here :)

Just a suggestion: It is much easier for people to review your patches if you keep on topic with the code. That means: Don't fix unrelated stuff (e.g. those whitespaces or unrelated [probably misspelled or incorrect] documentation). That way the git log remains clean and we can tackle one issue after the other.

Other than that (and the stuff mentioned in my review) the patch looks good. Thanks!

Updated patch based on fubhy's review.

Why do you create a new variable $field_attach_hook here? Just keep using $hook

I don't know, but It seems that the original $hook is needed later so if we override it once, we need to override it again later to revert, like this:

$hook = $hook == 'revision_delete' ? 'delete_revision' : $hook;

// some code...

$hook = $hook == 'delete_revision' ? 'revision_delete' : $hook;

and that's confusing.

Oups, forgot to change status.

Okay, cool... I didn't know that $hook was used after your code-changes because I only looked at the patch and not at the function itself ;). I guess its fine then. Only thing that I noticed by looking at the patch ones more is this (the code itself looks good, but I still have to give it a proper test-run):

+++ b/includes/entity.controller.incundefined
@@ -377,19 +418,53 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+        // We update base table only if entity doesn't have revisions or we
+        // are updating active revision.

Let's use proper english. Even in short code comments like this.

"Only update the base table if the entity doesn't have revisions or we are updating the active revision."

+++ b/includes/entity.controller.incundefined
@@ -400,6 +475,28 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+  /**
+   * Check whether entity revision is current.
+   */
+  public function isCurrentRevision($entity) {

"Check whether this is the current / active revision of the entity."

Or something like that.

+++ b/tests/entity_test.moduleundefined
@@ -45,6 +45,40 @@ function entity_test_entity_info() {
+      ),
+      // Make use the class label() and uri() implementation by default.

Make use "OF" the class label ...

One more try :)

+++ b/includes/entity.controller.incundefined
@@ -286,7 +286,12 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
-    if (!empty($this->entityInfo['fieldable']) && function_exists($function = 'field_attach_' . $hook)) {
+    // If we take a look at core example node_revision_delete(), first
+    // node_REVISION_DELETE hook invoked but then field_attach_DELETE_REVISION
+    // is called. So we need to adjust name of the hook, so both hooks will
+    // be invoked.

"In node_revision_delete() core invokes hook_node_revision_delete() and hook_field_attach_delete_revision(), so we need to adjust the name of our revision deletion field attach hook in order to stick to this pattern."

Or something like this.... Sorry I must've missed that part in my first review :(. Thanks for the patch, everything else looks good now.

One more try, again :)

Let's use proper english. Even in short code comments like this.

"Only update the base table if the entity doesn't have revisions or we are updating the active revision."

If we're being proper, I'd argue that "only" is redundant (and ugly at the start of a sentence). "If" is quite specific, "only" does not add meaning.

"Update the base table if the entity doesn't have revisions or we are updating the active revision."

I've been using entity revisions using these patches for a little while now with nothing obvious going wrong (it isn't a live site yet though so very low traffic).

It would be good to get a new review from fago to see what he has to add.

Maybe we should change that one sentence as pointed out by #37. Other than that it is working and looking good to me now.

We will have a BoF and some code sprinting @ Denver and hopefully a stable release before or during DCon. Hopefully this patch will make it into that.

I've changed the comment to #37 suggestion.

Let's leave it at RTBC then so fago can take a look.

I agree. Tagging (fago can remove if he doesn't want it so).

So how will this work for people using State Machine, Workbench Moderation, or any other module that separates the current revision from the latest revision? Does this patch support that?

fago goes into that briefly towards the end of #45.

@rooby: I'm looking for clarification. I have only a minimal understanding of this API, but I have a vested interest in entities being both revisionable and moderatable. I was hoping for a somewhat less technical assessment of the state of Entity API in this regard, assuming this patch is committed.

I believe what he was saying it that the way this patch currently sets the active revision will not work properly with field API.
He then gives an idea for a solution to that problem that has a negative aspect of having to save field API data twice but will still work.

So non-technical answer is, not properly supported in the current state of the patch, but hopefully entirely supported with a few changes to the patch.

@jstoller, those modules are responsible for taking care of the node revision updates themselves. Entity API module should respect what core does, not what we want it to do.

@indytechcook, Core allows for the vid of a node to be pointed at an older revision when it is saved, thus permitting these modules to separate the current revision from the latest revision. I've always considered this a bit of a hack, but never the less, this hack is the only thing allowing for content moderation in Drupal. I want to make sure the same technique can be extended to other entities created with Entity API.

For instance, if this patch gets committed and Beans are made revisionable, then will Workbench be able to moderate beans without too much work? Will content moderation modules be able to have a general solution for all revisionable entities based on Entity API, or will they need unique solutions for each entity?

As I said:
So non-technical answer is, not properly supported in the current state of the patch, but hopefully entirely supported with a few changes to the patch.

Hi @jstroller, thanks for you comments, let me clarify my statements a little.

While it possible to make a new revision that isn't the published revision, it does not work correctly with fields. Fields expect the newest revision to be the publish revision.

I wrote the beans module and help maintain the state machine module. Both State Machine (State flow) and Workbench moderation have to do some nasty tricks to keep the newest revision as the latest. Each time the node is saved, it has to check if the revision being saved is the newest and published. If it isn't it has to reload the published revision and resave that.

I think I understand what you are saying. Let's use Entity API to do this behind the scenes to do what these other modules are doing themselves? While I think this concept is a good idea, Fago has generally be against changing how core works.

Edit:

FYI. I can't wait for this patch to get in. It's so important.

@indytechcook, while I think it would be great if Entity API made content moderation easier, my primary concern is just that this doesn't make it any more difficult for State Machine to do what it does. I'm planning to use State Machine on a project and I'd like to use it with Beans, but I'd also like to use it with other as yet undefined entities.

I would like to update everyone with current status of this patch (see attached).

Tasks done:
1. Updated documentation.
2. Added interface EntityAPIControllerInterface.
3. Now check about whether entity is current revision is done on load.
4. Entity key 'set active revision' added to customize property of the entity that determine whether new revision should be current.
5. Test rewritten completely. Now it uses title text property so debugging is much more pleasant.

6. I have added Field API text field to entity in the test that breaks test completely. This proves @fago quote about Field API won't work. At the moment I am not quite understand solution, but will work on it. If anyone can point me to the right direction I will very appreciate (like @fago advised save field information twice).

Hey, we need this to work for field_collection revisioning. It looks like @ygerasimov has stalled. Can you please provide a status update and where we can help?

@crimsondryad welcome to help. The problem I have now is fields added to revisions. Please check the failing test.

Lets see the failures

Rules failure
I tried to add a new Rule without success and i got this log message
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'id' cannot be null: INSERT INTO {rules_trigger} (id, event) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => [:db_insert_placeholder_1] => node_presave ) in RulesEntityController->storeEvents() (line 173 of /drupal_path/sites/all/modules/contrib/rules/includes/rules.core.inc).

@vasike:
Are you saying you did not have this error, then you applied this patch in #55 and now you have the error?

exactly. sorry if i wasn't so accurate.
i am using the last Rules release, 7.x-2.1
after i downgraded to the last one Entity API release, 7.x-1.0-rc2, it worked.

Trying to get the test results.

Here is another version of the patch. Now Field API works properly.

In case new revision to be created that should become active or not active revision is updated, we invoke field_attach_update() two times now.

All tests pass!

Welcome to review.

Thanks ygerasimov!
I will be trying this out in the next day or two.

#66 is throwing

Fatal error: Cannot access empty property in /commerce_kickstart-7.x-1.4/profiles/commerce_kickstart/modules/entity/includes/entity.controller.inc on line 503 for me

A Question, Do we need to up date our entities to have any additional db cols? for the rid? not quite sure how thats working here, or is entity api handling creating a revision table?

@jucallme:
Yep. Modules that implement entity revisions need to update their schema accordingly.

You would have a revision database table to go with your entity table in a similar fashion to how drupal has a node and a node revision table. (with an added rid column in the main entity table.)

Also, see the tests included in this patch and its entity_test_revision table.

Just as a reference. Automated test will not be executed until this be fixed #1450686: automated tests fail.

10 days later... :)

Rerolled patch attached. Problem was in README.txt :)

Yay! Thanks @ygerasimov!

#73: entity-996696-revisions-73.patch queued for re-testing.

Can anyone provide some instructions on how best to test this patch? Or, perhaps it can't be tested (manually) since it's just an API change? The Field Collection and Bean modules can't get revisions in until this patch is committed and I'd like to help speed that up if possible. Thanks!

@Kristen Pol -- Thanks

Hope this can be tested and committed soon -- would very much like to have revisions within field_collections.

@Kristen Pol:
Someone could implement a patch for support for revisions in those modules that build on the changes in this patch.
I did that with profile2, although the patch is for an old version of this patch and has a few issues, but you get the idea - I need to update that patch.

I don't know the Entity API or the Field Collection (FC) implementation so I am not the best person to try to do the FC patch. Though maybe I will need to try out of necessity.

When applying the patch from #73, I get an error on the README.txt file:

[kristen:entity]$ patch -p1 < entity-996696-revisions-73.patch 
patching file README.txt
Hunk #1 FAILED at 27.
Hunk #2 FAILED at 48.
2 out of 2 hunks FAILED -- saving rejects to file README.txt.rej
patching file entity.api.php
Hunk #1 succeeded at 73 (offset -1 lines).
Hunk #2 succeeded at 199 (offset -1 lines).
patching file entity.module
Hunk #8 succeeded at 351 (offset -14 lines).
Hunk #9 succeeded at 413 (offset -14 lines).
Hunk #10 succeeded at 470 (offset -14 lines).
Hunk #11 succeeded at 1167 (offset -14 lines).
patching file entity.test
patching file includes/entity.controller.inc
Hunk #5 succeeded at 392 (offset 1 line).
Hunk #6 succeeded at 417 (offset 1 line).
Hunk #7 succeeded at 462 (offset 1 line).
Hunk #8 succeeded at 551 (offset 1 line).
patching file tests/entity_feature.module
patching file tests/entity_test.install
patching file tests/entity_test.module

I've attached the .rej file.

It applied cleanly for me, @Kristen Pol were you using the latest dev version?

Good call, @tim.plunkett. Looks good when applying to *dev*.

Btw, I have applied the patch (#73) and am starting to patch the Field Collection module to use revisions but am at a point where I need some feedback in terms of direction:

http://drupal.org/node/1031010#comment-6134700

Feedback is very welcome! I would love to get a patch finished early this week if at all possible.

For me the patch applied clean! Also the functunallity worke'd as expected. It would be good if a more experienced developer would take a look at it to get this RTBC!

+++ b/entity.api.phpundefined
@@ -74,6 +74,8 @@
+ *   - set active revision: (optional) When new revision is created, property
+ *     with this name sets newly created revision to be active.

I had to read this multiple times before I finally understood what exactly it does. Can we rephrase that to be a little bit more descriptive? Also, please make sure that the grammar is right (articles, etc.).

+++ b/entity.api.phpundefined
@@ -198,6 +200,8 @@ function entity_crud_hook_entity_info() {
+ * - deletion revision callback: (optional) A callback that deletes revision
+ *   of the entity.

Here you named it 'deletion revision callback' and in other places you named it 'revision deletion callback'. To me, the second version makes more sense. Please make sure that it is the same everywhere.

+++ b/entity.moduleundefined
@@ -62,6 +62,7 @@ function entity_type_supports($entity_type, $op) {
+    'revision delete' => 'revision deletion callback',

See above.

+++ b/entity.moduleundefined
@@ -96,6 +97,7 @@ function entity_type_supports($entity_type, $op) {
  *   The ID of the entity to load, passed by the menu URL.
  * @param $entity_type
  *   The type of the entity to load.
+ *
  * @return
  *   A fully loaded entity object, or FALSE in case of error.
  */
@@ -215,6 +217,7 @@ function entity_delete($entity_type, $id) {

@@ -215,6 +217,7 @@ function entity_delete($entity_type, $id) {
  * @param $ids
  *   An array of entity ids of the entities to delete. In case the entity makes
  *   use of a name key, both the names or numeric ids may be passed.
+ *
  * @return
  *   FALSE if the given entity type isn't compatible to the CRUD API.

This doesn't really belong in the patch. It doesn't really matter in this case but you should try to stay on topic when creating a patch. Just a hint. :)

+++ b/entity.moduleundefined
@@ -234,6 +237,27 @@ function entity_delete_multiple($entity_type, $ids) {
+ * @return
+ *   FALSE, if there were no information how to delete the entity revision.

What does it return when the entity revision was deleted? Also, please make sure that the grammar is right. (were / was, etc.)

Maybe better rephrase it to something like "FALSE if the entity type doesn't support revisions."

However, I think that in that case it should return NULL anyways because if the entity type supports revisions and the entity revision could be deleted it returns FALSE as well.

Also, the else is not required here.

+++ b/entity.moduleundefined
@@ -234,6 +237,27 @@ function entity_delete_multiple($entity_type, $ids) {
+/**
  * Create a new entity object.
  *
  * @param $entity_type
@@ -241,6 +265,7 @@ function entity_delete_multiple($entity_type, $ids) {

@@ -241,6 +265,7 @@ function entity_delete_multiple($entity_type, $ids) {
  * @param $values
  *   An array of values to set, keyed by property name. If the entity type has
  *   bundles the bundle key has to be specified.
+ *
  * @return
  *   A new instance of the entity type or FALSE if there is no information for
  *   the given entity type.
@@ -270,6 +295,7 @@ function entity_create($entity_type, array $values) {

@@ -270,6 +295,7 @@ function entity_create($entity_type, array $values) {
  *   The entity to export.
  * @param $prefix
  *   An optional prefix for each line.
+ *
  * @return
  *   The exported entity as serialized string. The format is determined by the
  *   respective entity controller, e.g. it is JSON for the EntityAPIController.
@@ -296,6 +322,7 @@ function entity_export($entity_type, $entity, $prefix = '') {

@@ -296,6 +322,7 @@ function entity_export($entity_type, $entity, $prefix = '') {
  * @param string $export
  *   The string containing the serialized entity as produced by
  *   entity_export().
+ *
  * @return
  *   The imported entity object not yet saved.
  */
@@ -338,6 +365,7 @@ function entity_type_is_fieldable($entity_type) {

@@ -338,6 +365,7 @@ function entity_type_is_fieldable($entity_type) {
  * @param $langcode
  *   (optional) A language code to use for rendering. Defaults to the global
  *   content language of the current request.
+ *
  * @return
  *   The renderable array.
  */
@@ -399,6 +427,7 @@ function entity_id($entity_type, $entity) {

@@ -399,6 +427,7 @@ function entity_id($entity_type, $entity) {
  *   of the entity, as returned by entity_uri().
  *   This parameter is only supported for entities which controller is a
  *   EntityAPIControllerInterface.
+ *
  * @return
  *   The renderable array, keyed by the entity type and by entity identifiers,
  *   for which the entity name is used if existing - see entity_id(). If there
@@ -455,6 +484,7 @@ function entity_access($op, $entity_type, $entity = NULL, $account = NULL) {

@@ -455,6 +484,7 @@ function entity_access($op, $entity_type, $entity = NULL, $account = NULL) {
  *   The type of the entity.
  * @param $entity
  *   The entity to show the edit form for.
+ *
  * @return
  *   The renderable array of the form. If there is no entity form or missing
  *   metadata, FALSE is returned.
@@ -1151,6 +1181,7 @@ function entity_views_api() {

@@ -1151,6 +1181,7 @@ function entity_views_api() {
  *      info before it is utilized by the wrapper.
  *    - property defaults: (optional) An array of defaults for the info of
  *      each property of the wrapped data item.
+ *
  * @return EntityMetadataWrapper
  *   Dependend on the passed data the right wrapper is returned.

The code style fixes are getting a little bit out of hand here. This should really be fixed in a separate issue.

+++ b/includes/entity.controller.incundefined
@@ -288,7 +334,11 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+    // our revision deletion field attach hook in order to stick to this pattern.

I nearly missed this, but the "." exceeds the 80 characters limit :P. Sorry buddy! :)

+++ b/includes/entity.controller.incundefined
@@ -402,6 +550,32 @@ class EntityAPIController extends DrupalDefaultEntityController implements Entit
+    else {
+      drupal_write_record($this->entityInfo['revision table'], $entity, $this->revisionKey);
+    }

The else is not required here either. It doesn't reach that point in code anyways if the previous if statement evaluates to TRUE.

I have updated the issue summary for the Field Collection module revision support:

#1031010: Support revisions for field collections

which is being built upon the Entity API patch here.

It would be great if someone who's comfortable with this Entity API patch (@fubhy ?) to take a look and review and comment on the latest Field Collection patch available.

I'll try to pickup at where #84 leaves off this week if I have time.

Rerolling this right now.

Tests are still running locally, but testbot will probably be faster. I made all the changes requested by fuhby - undid out-of-place style fixes, changed FALSE to NULL in that one function, removed unnecessary else statements, clarified what set active revision does.

Oops. OK, let's try this.

+++ b/entity.api.phpundefined
@@ -74,6 +74,10 @@
+ *     with this name sets newly created revision to be active.

This line is a left-over I think :).

+++ b/includes/entity.controller.incundefined
@@ -121,12 +121,39 @@ interface EntityAPIControllerInterface extends DrupalEntityControllerInterface {
+interface EntityAPIControllerRevisionableInterface extends EntityAPIControllerInterface {
+  /**
+   * Delete revision.
+   *
+   * @param $revision_id
+   *   Revision ID.
+   * @return boolean
+   *   TRUE if the entity revision could be deleted, FALSE otherwise.

There should be a newline before @return as well as between the interface declaration and the doc block of the first function in the interface.

Thanks fuhby for the review. Try this one.

Nevermind... I was still asleep :)

I would love to see some integration in the Views and Metadata controllers too!

@fubhy, can we get this patch committed first and then implement new features like views and metadata controller? This patch is hanging here for really a long time and a lot of people are eager to have it landed. Lets create separate issues for new features.

I am fine with that... I need it too for my GSoC project.

+1 on committing. Field Collection really needs this too.

+1 on filing separate issues for other feature requests and not trying to mix them into this issue.

This is not a separate feature request. Instead, it is a very valid request for integrating the new code with existing parts of the code within the same module. This is a must have addition to THIS patch and not a separate feature request. I need this patch commited as well for my own project but what I requested here is something that I consider a must-have addition.

It's up to fago to decide whether he wants to commit this without that. I am fine with both.

I tried the patch provided in #92 in the profile2 module but I am getting the following error:
Notice: Undefined property: Profile::$vid in drupal_write_record()

I think this is caused by the following code in EntityAPIController::save():

<?php
      // When saving a new revision, unset any existing revision ID so as to
      // ensure that a new revision will actually be created, then store the old
      // revision ID in a separate property for use by hook implementations.
      if (!empty($this->revisionKey) && empty($entity->is_new) && !empty($entity->is_new_revision) && !empty($entity->{$this->revisionKey})) {
        unset($entity->{$this->revisionKey});
      }
?>

Am I missing something?

Following up the problem mentioned in #100, the revisionKey of $entity must be set to NULL instead of unsetting:

<?php
      // When saving a new revision, unset any existing revision ID so as to
      // ensure that a new revision will actually be created, then store the old
      // revision ID in a separate property for use by hook implementations.
      if (!empty($this->revisionKey) && empty($entity->is_new) && !empty($entity->is_new_revision) && !empty($entity->{$this->revisionKey})) {
        $entity->{$this->revisionKey} = NULL;
      }
?>

Though, unsetting should work in drupal 8. (refer to the drupal_write_record() implementation of drupal 8 here: http://api.drupal.org/api/drupal/core!includes!schema.inc/function/drupal_write_record/8).

@massud:
How are you doing it with profile2? Are you using the patch at #1043128: Profile revisions?

Because the patch I did there has a couple of issues and is for an older version of this patch and needs to be updated.

@rooby,
I have developed a new patch based on yours. The problem comes from the fact that the property_exists() function of PHP returns TRUE for an "unset" property of a class instance. A piece of code has been added to the drupal 8 implementation of drupal_write_record() to take care of it.

Also it would be very helpful to have an "entity_revision_list" function.

I am experiencing the problem mentioned in #60. I traced the code and figured out that this problem is caused by the changes made in the save() method of EntityAPIController class:
Previous implementation:

<?php
  function save() {
      ...
      if (!empty($entity->{$this->idKey}) && empty($entity->is_new)) {
        $return = drupal_write_record($this->entityInfo['base table'], $entity, $this->idKey);
        $this->resetCache(array($entity->{$this->idKey}));
        $this->invoke('update', $entity);
      }
      else {
        $return = drupal_write_record($this->entityInfo['base table'], $entity);
        $this->invoke('insert', $entity);
      }
      ...
  }
?>

New implementation:

<?php
  function save() {
      ...
      // Create new entity.
      if (empty($entity->{$this->idKey}) || !empty($entity->is_new)) {
        // For new entities, create the row in the base table, then save the
        // revision.
        if (!empty($entity->is_new)) {
          $return = drupal_write_record($this->entityInfo['base table'], $entity);
        }
        if (!empty($this->revisionKey)) {
          $this->saveRevision($entity);
          $update_base_table = (isset($entity->{$this->setActiveRevisionKey})) ? $entity->{$this->setActiveRevisionKey} : TRUE;
        }
        $this->invoke('insert', $entity);
      }
      ...
  }
?>

Rules module does not set the is_new property. So in previous implementation, the else block is executed and entity is saved in the database. But in new implementation, entity is never saved in the database since is_new is not set.

Testing the Field collection patch in tandem with this patch.
Hit the error at http://drupal.org/node/996696#comment-6187452
and resolved using the suggestion at http://drupal.org/node/996696#comment-6189344.

Working well

#73: entity-996696-revisions-73.patch queued for re-testing.

Re #92, one question I have is - should the latest entity revision not also be saved to the base table? Currently only the revision ID is updated in the base table on a new revision.

Temporarily I've worked around this by in a hook_{ENTITY_TYPE}_update() but that's kind of silly:

function hook_ENTITY_TYPE_update($entity) {
  if (isset($entity->is_new_revision) && $entity->is_new_revision) {
    drupal_write_record(ENTITY_TYPE, $entity, ENTITY_ID);
  }
}

I would add this in a new patch, but the current code seems to go to some pains not to update the base table - and somehow I wonder if I'm missing something about this.

I got a similar error to the one in #100 except with the patch for the field collection module from #1031010: Support revisions for field collections

The change suggested in #101 fixed it for me. Patch re-roll just for that change attached.

go testbot

Adding a tag to track this issue as part of the LSD/CSI project (http://groups.drupal.org/large-scale-drupal-lsd-projects-and-plans/conte...)

#111: entity-996696-revisions-111.patch queued for re-testing.

The patch looks good. I have it working well with beans: http://drupal.org/node/1335494#comment-6335816

Great job everyone.

<--- Late comer to this issue!

First of all, big +1 for revision support, it will help me a lot at work. And on a side note, thanks for all the hard work @fago and others, Entity API has saved me plenty of time at work already. Any interest in supporting reverting an entity to a previous revision? This is what I'm working on right now, so I'd be glad to help contribute it here or on top of this patch in a separate issue.

Isn't EntityAPIControllerRevisionableInterface::getCurrentRevision() supposed to be EntityAPIControllerRevisionableInterface::getActiveRevision() as per fago's comments in #109? The terminology is indeed inconsistent, and there are some other variables and documentation that need to be fixed.

The isCurrentRevision()/isActiveRevision() method returns $entity->currentRevision, a boolean that's set when the entity is loaded. This means if the load method is overridden the overriding method needs to be sure to set the currentRevision/activeRevision property. That means more chance for error. Loading properties into the entity object that could later be required by another controller method just seems like bad practice to me, but maybe I'm wrong and maybe it should at least be documented if the isActiveRevision() method will use that property. Also, the name of the property is a little misleading. I would assume that it stores the active revision ID, not a boolean flag indicating whether this is the active revision for the entity. $entity->activeRevision and $entity->isActiveRevision are two entirely different things.

EDIT: By the way if no one gets to it I will try to put together a new patch tomorrow :-)

The currentRevision flag is exactly how it's implemented in Drupal 8 and I think it makes sense to be consistent.

You're right about that. Touché for consistency, though ignoring it, I think it stills seems to me less stable without standard flags being commonplace in entities. Generally the entity API allows developers to specify the data that is expected in an entity via the 'entity keys' array and entity property info with Entity API. But now if a stdClass or other entity is created with the new keyword the developer may need to set a flag as well, and perhaps the create() method should set it as well..

The "current" terminology still needs to be changed to "active" though, as per #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text

Speaking of which, the patch at #218755: Support revisions in different states is now using completely different terminology with the isDefaultRevision flag and isDefaultRevision() method (which emphasizes my point that currentRevision and isCurrentRevision properties say two different things to me as a programmer).

At the very least $entity->currentRevision is a little misleading.

jordojuice: Reverting works - check out #1031010: Support revisions for field collections (though I am not sure if that's field_collection's implementation to thank or Entity API itself).

For clarity...

The Default Revision is the version that will be selected if you load an Entity by ID for viewing, but do not specify a version. node_load($nid); gets back whatever the "default" version is, which may or may not be the most-recently-edited.

The Active Revision is the version that will be selected if you load an Entity by ID for editing, but do not specify a version. Note that this may or may not be the same as the Default version.

Currently Drupal Core thinks Default=Active=Latest Revision. However, modules like Workbench Moderation, Statemachine and Revisioning, split these into separate concepts. The current patch in #218755: Support revisions in different states aims to codify these distinctions in Core, making content moderation more pervasive and easier to implement.

If you want to be consistent with these advancements, then really you should implement both getActiveRevision() and getDefaultRevision() methods. At this point, getActiveRevision() should return the latest version in the revision table for an entity, while getDefaultRevision() should return the version referenced in the entity's base table. I think that should allow this to work as expected with current revisioning implementations.

@ivanjaros +1

So what does one have to do to finally be able to use revisions?

It depends what you want to do revisions for.

This provides the API to be able to do revisions.
Then any module that creates entity API entities has to handle its own revisions.

So for example, if you look at modules like field_collection and profile2, they have their own issues for revisions, which require this patch for entity api and a patch for their own module to implement their revisions.

Think it is clear that:

This is an API module, so it doesn't provide any end-user features.

I believe the desire is to help close this issue and commit a patch that allows the other modules like field_collection and profile2 to progress with implementing revisions respectively.

How can we help to move this along from its' current state of needs work to committed?

Thanks for all the developers hard work and do hope we can have a solid patch to commit soon.

This patch, combined with the one for field collections at http://drupal.org/node/1031010 was working great, until it came to file fields on field collections. In that situation, when creating a new draft revision, the images and files within a field collection on the published revision were deleted. :(

After a bit of debugging, I tracked down the problem to file_field_update(), where it has this snippet of text:

  // On new revisions, all files are considered to be a new usage and no
  // deletion of previous file usages are necessary.
  if (!empty($entity->revision)) {
    foreach ($items as $item) {
      $file = (object) $item;
      file_usage_add($file, 'file', $entity_type, $id);
    }
    return;
  }

However, for field collection entities, there was no $entity->revision boolean flag set, and so rather than adding the usage and returning, it skipped it and went on to delete the file.

A simple one line change to includes/entity.controller.inc fixed this for me. Here when saving a new revision, I set the $entity->revision flag, giving this bit of code in save():

       // When saving a new revision, unset any existing revision ID so as to
       // ensure that a new revision will actually be created, then store the old
       // revision ID in a separate property for use by hook implementations.
       if (!empty($this->revisionKey) && empty($entity->is_new) && !empty($entity->is_new_revision) && !empty($entity->{$this->revisionKey})) {
         $entity->{$this->revisionKey} = NULL;
 +       $entity->revision = TRUE;
       }

Not sure if this was the correct place, but it has fixed the problem.

Updated patch attached.

@stella An interdiff.txt would be handy :)

@Kristen
See http://drupal.org/node/996696#comment-6363272 last code block
Just one extra line added

Ah... didn't catch that is was just that one line... sorry for the slowness ;)

Since it was not RTBC before this update, though, I guess I can't mark RTBC (although I want to!).

I'm not sure that #116 and #121 have been addressed (either that the patch will include this stuff or not and, if so, we need an updated patch).

Per the comment in #121: state machine does not have the concept of default and neither should Drupal. You edit the revision you want to edit. The "active/current/live/whatever" is the one loaded without a revision and it should not matter where that is loaded form.

EDIT: This is allows you to have multiple draft revisions.

The terminology is wrong though and attached is the patch to fix it. I added a few helpers to the entity also. This works great with the bean patch. #1335494: Beans need revisions

Updated Tests.

This does change on assumption. It appeared as if the active check was different in loading then in saving which seemed a little silly to me. So I changed the "set active revision" to "active revision" in the entity info api key and just check that all the time.

Updating status

Re #131: Drupal and State Machine absolutely have the concept of a default revision. They just don't call it that, which is what #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text is trying to fix. Specifying this default revision is what the vid field in the node table is for. Drupal unfortunately screws this up by further assuming that the latest version should always be the default version, which every content moderation module jumps through hoops to work around. That is what #218755: Support revisions in different states is trying to fix. The concept of an active revision is a little less clear, but the safest default behavior when editing an entity would be to edit the latest version of that entity (assuming no specific vid has been requested), as that is currently the most active. This doesn't preclude having multiple draft revisions, but it is a more sane default than loading {node).vid.

Thanks for your comments jstoller. The definition you used for "active" implied a single draft revision assumption. Drupal or the Entity API module should not make this assumption IMO (also State Machine doesn't make any assumptions). This sounds like an implementation detail that should not be in an API module but in the implementing module itself.

I apologize for the discussion bickshed here. We really need to get this patch in. It's holding up a stable release and issues on other modules. Any other comments on the actual patch?

I am all for getting the patch in as so many other modules rely on this, but I also see that if this module goes down the wrong path with terminology it may make fixing it later much harder as those other modules are going to rely on how this works. Case in point, the Entity API fix that was a workaround for a core bug last year that broke a bunch of modules when it got pulled back out.

I second @crimsondryad's concern. I would strongly recommend that this patch adopt the same terminology as #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text. That issue still has some debate over if and when the word "version" should be used instead of "revision", however there seems to be pretty wide-spread agreement on "default", "active" and "published" as standard terminology. Adopting that terminology here now could save much pain down the road.

Can't actually test it right now but I read through the patch twice and couldn't find anything that bothered me. Looks much better now, too. Thanks...

Testbot says it is good so I think this is RTBC

A documentation nitpick

+++ b/README.txtundefined
@@ -27,7 +27,8 @@ developing, you may stop reading now.
   * Thus the module provides API functions like entity_save(), entity_create(),
-    entity_delete(), entity_view() and entity_access() among others.
+    entity_delete(), entity_revision_delete(), entity_view() and entity_access()

@@ -48,17 +49,16 @@ developing, you may stop reading now.
+ * The controller supports fieldable entities and revisions. There is also a
...
+ * There is also an optional ui controller class, which assists with providing

This appears to be evidence of a comment that is too long. (over 80 characters)

congratulations to everyone! :)

Congratulations!

nice work!

@fago et al
Hello,
I cant find where 'EntityAPIController::revisionTable' is instantiated in entity.controller.inc. My thought is that it would have been instantiated using properties from the hook_entityinfo i.e. 'revision table'. Thnx.

Any idea on when the next release of the module will drop?

After a quick look at the topic it seems that Entity API now support versioning (awesome!), so wouldn't it be a good time to update the home page of the module which still claim not to support it?

agree.. perhaps when beta 5 or rc1 comes out.. seems the page was last updated when beta 4 was release in April.

Seems there still isn't an official release that contains the patch. Hopefully soon?

Also I see not change record of this in http://drupal.org/list-changes/entity

@steinmb: You're welcome to create a change record, they don't write themself. Few contrib projects are currently using change records, and even if they do, not as strictly as core. It's a rather time-consuming task to write a good change record and it's hard enough to find people who write them for core, where it's enforced through the critical task threshold.

Re: #157, #158: I have started a documentation page about revisions at http://drupal.org/node/1892478.

I'm looking at this from the angle of custom entities that did their own revisions before and the steps to use the new revision system. It is incomplete and probably worded inaccurately.

Subscribing

Subscribing

There is no need to post to an issue in order to subscribe to it anymore. Just click the "Follow" button at the top of the page. See Stop subscribing, start following for more information.

Thanks @jstoller.