Delete redundant revisions

I have been testing workflow transition use cases with drafty (dev versions of drafty, state_machine and workbench_moderation), and found that the identical revisions created by drafty were seriously cluttering up the revision-list views, which makes things difficult for content moderators.

The patch in #3 works well to get cron to clean up the temporary revisions, but I really needed them to be gone immediately after the successful transition - the next cron job is too far away.

Deleting the temporary revision in the restorePublishedRevisions function is not appropriate, because if the call stack involves an entity hook for the revision-to-be-deleted (which saving a new draft of a published revision *does*), then you get exceptions all over the place when other handlers try to access the revision entity that you just deleted.

As a workaround, here's a variation on the patch from #3 which uses a second variable to allow the delete operation to be kicked off as a batch job immediately after drafty forward-publishes its new revision.

#2614060] is a possible approach for this in CPS.

I've re-rolled the patch from #4 against the 7.x-1.x-Beta3 release (which is identical to the current state of the 7.x-1.x-dev branch)

This patch seems to work well, RTBC ?

There is only one thing that does not look right, in my humble option the moderation page should list the published revision at the top as the first entry of the list, would this be possible ? should I open another issue ?

I have tested #6 and can confirm it is working, thanks

I can see one issue with this, the published revision ends up in the wrong position in the revision list. Not sure if this is a drafty or workbench moderation issue. Heres what I did:

• Created a new draft (10174)
• Published draft (10179) in the table this ends up 2nd from the top but should be 2nd bottom
• Created another draft (10176)
• Edited draft again (10178)
• Published the new draft (10180)

Do we need to create a new revision of the published draft every time?

Hello,

I have a question, if we use the delayed removal proposed by this patch by means of the queue system, should not the operation check that the item to be deleted is not the current published version ? If we don't verify it, someone could set the item as published and what would happen then ? the published revision would dissapear !!

On the other hand, I think it would be better to remove the forward copy when the draft is published, better than deleting the original revision.... ( I dunno if this is for workbench moderation or for drafty to consider)

It is annoying to use a batch process to delete the original copy, as far as I can understand, the issue is that the removal can not be done at "restorePublishedRevisions()" because it is inside an entity_update() which is dispatched when creating a new draft, what about doing the deletion in a synchronous way in another hook then, like hook_exit() for exemple ?

@istirk, I have tested this patch for workbench_moderation which creates a new revision when moderating the draft, updating the moderation_node_history properly and then deleting the original useless draft revision, which seems to fix the wrong position issue, feel free to test it

diff --git a/all/modules/contrib/workbench_moderation/workbench_moderation.module b/all/modules/contrib/workbench_moderation/workbench_moderation.module
index 7ef3233..38449a0 100644
--- a/all/modules/contrib/workbench_moderation/workbench_moderation.module
+++ b/all/modules/contrib/workbench_moderation/workbench_moderation.module
@@ -1752,7 +1752,19 @@ function workbench_moderation_moderate($node, $state) {
   $node->workbench_moderation_state_new = $state;
 
   // Save the node and let drafty handle it.
+  $node->revision = TRUE;
+  $node->new_revision = TRUE;
+  $node->is_new_revision = TRUE;
+  $node->default_revision = TRUE;
+
+  $old_vid = $node->vid;
   node_save($node);
+
+  db_query("UPDATE {workbench_moderation_node_history} SET vid = :vid WHERE nid = :nid AND vid = :old_vid",
+      array(':vid' => $node->vid, ':nid' => $node->nid, ':old_vid' => $old_vid));
+
+  entity_revision_delete('node', $old_vid);
+
 }
 
 /**

Patch 6 uses variables to default to not deleting old revisions and to use cron method. I have updated patch as I need old revisions to be deleted by default and to use batch method. Thanks again for the patch in #6.

@unknownguy I tried it but still have the same issue

Deletion of the node causes issues for workflow history for some modules, such as State Machine. I've rerolled this patch to include hooks into the queue functions as well as a hook into the Drafty::restorePublishedRevisions() function immediately after the new item has been published and before the old revision is deleted.

This patch will fail until #2468907-11: Refactor tests to remove dependencies is reviewed and committed.

I've applied the patch from #6 and the patch from #2468907: Refactor tests to remove dependencies and am still getting the issue. Can anyone offer any advice?

I've tried the patch from #15 and the patch from #2468907: Refactor tests to remove dependencies and am still getting the issue. After playing around with it I made some changes for the Drupal Community to review. I started with the patch from #15 and made some tweaks. Most notably setting delete_old_revisions to TRUE and some code to delete from workbench_moderation_node_history. Add to add a loop to delete any old revisions that were created before hand too. If anyone could take a hint and give me some feedback it would be appreciated. Not a Drupal pro so sure there's room for improvement

@smustgrave I think delete_old_revisions was deliberately set to FALSE to not interfere with Draftys standard behavior. To expose this currently "hidden" feature / variable we might want to add an administrative settings page for Drafty and / or add documentation to the readme file.

@das-peter I can try and write something up for the readme file. Still seeing if the patch passes or not.

Updated patch from #22. Will continue to fail until https://www.drupal.org/node/2468907#comment-11841026 is added to dev release. As mentioned before.

Do not change status if this requires any other patch to be committed first.

@smutsgrave Good to see some progress on this issue!
I reviewed your patch, here are some point to work on:

Please do not change the status, even if you upload a new patch, until the mentioned patch in #18 is fixed.

1. The number in the patch file name is not a date but the node id of the issue
2. Upload a interdiff with you patch, the interdiff must be a diff of patch #15 and your new patch, see for more info about creating a interdiff https://www.drupal.org/documentation/git/interdiff
3. Do not fix issues for workbench moderation, but create solutions how workbench moderation can implement this module, do not use module_exist('workbench_moderation')
4. Fix code style issues, use Drupal code standards, it's really hard to check your patch if you need to cleanup many spaces, for more information about code style check: https://www.drupal.org/docs/develop/standards

I'm happy to give some more technical feedback when you upload a new patch with interdiff, no need to change the status ;)

@Tessa Bakker I've added the interdiff does that look correct?

@smustgrave File names are correct now, but your patch is 0kb and in the interdiff needs to be a diff of patch X and patch Y.

Almost there :)

@Tessa Bakker

Think I was trying to compare two branches which isn't the desired approach. How would you recommend the command be?

Also appreciate this help!

@smustgrave: See: https://www.drupal.org/documentation/git/interdiff#patchutils

interdiff old.patch new.patch > interdiff-[issue_id]-[old_comment_number]-[new_comment_number].txt

Instead of using this issue queue for helping, please allow the contact form on your profile or use the contact form on my profile :)

So I decided to take a different approach this go around. I believe I got the interdiff right this time. I added an admin page so the administrator can set the variables being referenced in #15. I also added a button in hopes of cleaning up previous revisions. My understanding is that on the moderation page, the published revision will always be at the top while the current draft will be directly below that. Anything between the draft and published would of been caused by this current bug. I added this feature because this bug is currently live on our site and by the time we get a fix out there, who knows how many nodes could have copies of the published revision. I wrote the query something along the lines of
"SELECT nr.title, nr.nid, nr.vid FROM node_revision nr WHERE vid > (SELECT vid FROM workbench_moderation_node_history WHERE is_current = 1 and published = 0 and nid= nr.nid) AND vid < (SELECT vid from node WHERE status = 1 and nid = nr.nid);"

@smustgrave, why placing published revision as first and others after. Logically they should be in order of the date things happened. Always moving published to first causes great confusion with Workbench_Moderation and Diff modules.

It makes no sense to make a new revision of the published node, just leave it as is and create new revisions.

@Kwfinken I will agree with that 100%. But that's how drafty seems to handle it's revisions and since workbench 3.0 is requiring drafty I don't see any way around it.

2 possiblities:
1) stop using drafty and go back to the 1.x branch that worked well. We were basically forced against our will to go to the 3.x branch since there is a security issue on the 1.x branch, but the 3.x branch is terrible due to this issue!
2) since this comment is in the drafty message queue, do you understand where drafty is doing this so that we can submit the appropriate fix.

BTW, Metatag module removed support of Workbench Moderation 1.x.

Same here, we are required to use 3.x branch because of security issue. I only have a small understanding of drafty so far so not sure if I would be the one to fix the issue. Issue is if the current draft revision is newer then the published revision and you save the current draft to a different moderation state other than published, workbench moderation will then unpublish the published revision. Drafty does fix this issue by keeping the published revision at the top.

Made some tweaks from before. Hope this looks ok.

There are some bugs in this patch.

• Indendation issue.
• Sentences need to end with dots.
• The form should be a fieldset. This would not require the strange item and #markup code.
• histor}y inside query.
• trailing space in menu description
• every value in a query should use placeholders. This is importand for db column binding, prepared statements and cross db support.
• "//Also" Is missing a space

@hass thanks for the feedback! Made some changes let me know if they look better!

Not really. New bugs introduced, old ones not fixed. Indendation in drupal is 2 spaces, not 1, not 3, not 4 or any other.

Instead of return $form for the admin form, use return system_settings_form($form); (documentation)

@Tessa Bakker made the change.

revisioning."',

1. +++ b/drafty.module
   @@ -5,6 +5,32 @@
   +   'description' => 'Configuration settings for how Drafty handles "forward revisioning."',
   

   Place DOT ourside the double quotes.

2. +++ b/includes/drafty.admin.inc
   @@ -0,0 +1,57 @@
   + $form['revision_cleanup'] = array(
   +   '#type' => 'fieldset',
   +   '#title' => t('This is a list of configuration settings for Drafty.'),
   + );
   +
   + $form['revision_cleanup']['drafty_delete_old_revisions'] = array(
   +   '#title' => 'Delete old revisions instantly',
   +   '#type' => 'checkbox',
   +   '#default_value' => variable_get('drafty_delete_old_revisions', FALSE),
   +   '#description' => t('Selecting this delete older revision on save.'),
   + );
   +
   + $form['revision_cleanup']['drafty_delete_with_cron'] = array(
   +   '#title' => 'Delete old revisions on cron',
   +   '#type' => 'checkbox',
   +   '#default_value' => variable_get('drafty_delete_with_cron', FALSE),
   +   '#description' => t('Selecting this will delete older revisions when cron runs.'),
   + );
   +
   + $form['revision_cleanup']['delete_now'] = array(
   +   '#type' => 'submit',
   +   '#value' => t('Remove revisions now'),
   +   '#description' => t('Instantly delete revisions left behind by drafty (those that are mare published->published but not current).'),
   +   '#submit' => array('drafty_delete_now_submit'),
   + );
   +
   + return system_settings_form($form);
   

   Indendation is 1 and not 2.

   t('General settings') is better I think.

3. +++ b/includes/drafty.admin.inc
   @@ -0,0 +1,57 @@
   + $results = db_query('SELECT nr.nid, nr.vid FROM {node_revision} nr WHERE vid > ' . $workbench_query . '  AND vid < ' . $node_query . '', array(
   

   I'm not a fan of this splitting and would revert to a single string, but leave this up to the maintainer to decide.

4. +++ b/includes/drafty.admin.inc
   @@ -0,0 +1,57 @@
   +  //Also want to remove the record from the workbench_moderation_node_history table
   

   Space missing after "//"

+++ b/drafty.module
@@ -5,6 +5,32 @@
+ $items = array();
+
+ $items['admin/config/system/drafty'] = array(
+   'title' => 'Drafty',
+   'description' => 'Configuration settings for how Drafty handles "forward revisioning."',
+   'type' => MENU_NORMAL_ITEM,
+   'page callback' => 'drupal_get_form',
+   'page arguments' => array('drafty_form'),
+   'access arguments' => array('administer drafty'),
+   'file' => 'includes/drafty.admin.inc',
+  );
+ return $items;

Indendation is incorrect.

@smustgrave Thank you very much for pushing this issue. You get a lot of feedback regarding the Coding Standards. Reading them up and applying them can be time consuming and cumbersome. That's why there's a PHP_CodeSniffer library in the Coder module that allows to scan code automatically for Drupal Coding standard issues. This tool can be integrated in many different IDE's which makes working with it even easier. Check out the documentation: and the sub-pages about the IDE integrations.
Questions / Problems - drop me a message using my contact form, I'm happy to help.

Totally agree with #42

The revision history gets broken as the published revision is always on top of the list. Instead of removing the original published revision it would be much better to delete the forward copy when the draft becomes published, this way the revision page would be compatible with the 1.x branch.

@unknownguy

That would be ideal but if you're on the 1.x branch of Workbench Moderation you don't need to use Drafty. Myself and other I believe switched to 3.x of Workbench Moderation because of a security issue. There's a scenario in Workbench if you have a draft revision newer then the published revision, when you save that draft it will unpublish the published revision. This is because there is code in Workbench Moderation that unpublishes everything below the current draft on save. Which is why the published revision needs to be at the top.

Made some changes to the api file. Based on the patch from #15. Understand some standards may be off did my best.

+++ b/drafty.api.php
@@ -1,6 +1,20 @@
+/**
+ * TODO Write description of this hook.
+ *
+ * @param $type
+ *   A node type.
+ * @param $id
+ *   A node ID.
+ * @param $vid
+ *   A revision ID.
+ */
+function hook_drafty_after_restore_published_revision($type, $id, $vid) {
+  //TODO What is the purpose of this hook?
+}

This whole chunk is redundant; A new hook implemented that only contains a TODO asking the purpose of the hook.

Please remove, or if it is needed, remove the TODOs and populate accordingly.

In patch #15 there is a line 'module_invoke_all('drafty_after_restore_published_revision', $type, $id, $vid);' My understanding is that this hook drafty_after_restore_published_revision requires documentation in the drafty.api.php file. I'm just not sure what's suppose to go there.

@smustgrave

I don't see what you mean, in the current state of things, when the draft version becomes published, the published version remains at the second position of the revision's list, why are you saying that the first revision (top of list) must always be the published ?

@unknownguy

I'm not saying it must be published but the published revision must always be above the current draft revision. If you look at the png I uploaded revision2, you'll see there is a current draft above the published. If you move the current draft into any other moderation state the published revision will become unpublished. This is because there is code in Workbench Moderation that says anything below the current draft shouldn't be published. Drafty just makes sure that the published revision is above the current draft. Make sense? This isn't my ideal scenario either but that's my understanding of it.

@smustgrave
IMHO, If this is what Workbench Moderation is doing, it is an error in the way Workbench Moderation works. You should make Drafty work appropriately and then have the people on Workbench Moderation correct their code. Forcing your code to do the wrong thing to fix someone else's dependency is counter productive to the long term stability of the code base. ALL revisions should be in the exact order that they happen to maintain a valid audit trail, once you have this working, it should be fairly simple for the Workbench Moderation people to correct their faulty logic!

I really think this issue should no be marked as postponed.
It's a problem that affects quite some people, it has a proper patch and it needs reviews.
I guess it's somewhat blocked by the inability to run the test suite #2468907: Refactor tests to remove dependencies? but this does not justify to "hide" (postpone) it in the issue queue.
We want maximum exposure for #2468907: Refactor tests to remove dependencies? Keep this issue & patch visible but mention it's dependency on #2468907: Refactor tests to remove dependencies.
Issue summary updated accordingly.

Even thought I don't have any authority and clearly don't want to step on anyone's toes, but back to "needs reviews" it goes ;)

Review of latest patch:

+++ b/drafty.api.php
@@ -1,6 +1,20 @@
 <?php
+
 /**
  * @file
  * API documentation for drafty.
  */
 
+/**
+ * TODO Write description of this hook.
+ *
+ * @param $type
+ *   A node type.
+ * @param $id
+ *   A node ID.
+ * @param $vid
+ *   A revision ID.
+ */
+function hook_drafty_after_restore_published_revision($type, $id, $vid) {
+  //TODO What is the purpose of this hook?
+}

The hook implementation was lost. It was introduced here #2579205-15: Delete redundant revisions by @jfalkner.
See drafty_queue_delete_batch_finished_after / drafty_after_restore_published_revision.
I'm not sure if these are really needed because if you delete a revision hook_ENTITY_TYPE_revision_delete / hook_revision_delete are fired anyway.
And saving the forward-revision will trigger the respective hooks too - I guess the forward revision handling could be covered by using the drafty api e.g. Drafty::isDraftRevision()

The patch in #2579205-59: Delete redundant revisions also contains a bunch of workbench moderation specific code. This wont fly.

It seems like the patches currently are all over the place.
Thus I've create a mash-up of the parts I think are viable.
The new patch bases on #2579205-6: Delete redundant revisions by @LittleRedHen, and incorporates some changes by @smustgrave to provide a settings page to configure the forward revision handling as well as the related permission.
The new hooks are left out because I'm not yet sure we really need them - see my statement above.
And all the workbench_moderation specific things will have to live in workbench_moderation itself.

delete extra revision published when moderating draft->published. By using function workbench_moderation_moderate_submit.

function workbench_moderation_moderate_form_submit($form, &$form_state) {
//add db_delete to delete extra revision of published.
db_delete('node_revision')->condition('vid',$form_state['values']['node']->vid+1)->execute();
         db_delete('workbench_moderation_node_history')->condition('vid',$form_state['values']['node']->vid+1)->execute();
.....
}

#2468907 has been committed, so what's next for this?

The patch needs to be rerolled.

As described in #2824607: Fundamental change in how revision works between 1.x and 3.x? this is probably a bug?

Attaching a reroll

'@error' => print_r($ex),

Looks like this requires second param, isn't it?

+ // Run cron
+ $this->drupalGet($GLOBALS['base_url'] . '/cron.php', array('external' => TRUE, 'query' => array('cron_key' => variable_get('cron_key', 'drupal'))));

Why not $this->cronRun(); ?

How does this work?

Should wok. What about cronrun?

Just came across this issue and am testing these fixes out as it's causing a lot of confusion to content reviewers thinking people are publishing pages when they are actually just creating new drafts.

@DamienMcKenna what version of Drafty does your patch apply to? I've tried applying to beta4 which applies fine, but there is an error when configuring Drafty as file includes/drafty.admin.inc is not part of your patch. Do I need to apply another patch first?

@DamienMcKenna, @ThomWilhelm

Can confirm the issue above with patch #74

Looks like we are missing the inclusion of includes/drafty.admin.inc from patch #66 onwards.

It was last in patch #59.

Attached patch is based on #74 & relevant code in #59.

Apologies... dud file. rerolled.

Marking as Needs review.

Test had failed because Entity Translation is currently at 7.x-1.0-beta6.

Patch includes removal of GetAdminUser function in Drafty Entity Translation #1992010 & Drafty Entity Translation tests as per comments in each file.

Also updated drafty.test to include @hass suggestion for $this->cronRun() .

Marking as needs review.

@bsains working for me. I appreciate cleaning up/improving what I was trying to do before. Still new to patching and drupal standards. This looks really good!

This is working really great in my testing!

However, removing the old revision gets rid of the original revision timestamp (who knows when that revision was created?) and when used with workbench_moderation, the entries on the 'workbench_moderation_node_history' table which describe the history of which transitions occured are also lost.

It'd be really great if drafty would transfer information from the old revision to the new revision when deleting the old one, and also call a hook to allow other modules to do the same (ie. workbench_moderation). Otherwise, this doesn't just clean up, it's deleting useful information.

I looked at trying to implement my suggestion in #84 and found that:

1. There's nothing in core's Entity API to help finding the revision timestamp for the different entity types, although, it seems like most use the 'timestamp' column
2. It's not possible to set a the revision timestamp on a node's revision during save, without doing some hook_node_presave() hackery, because it's always reset to the request time.

So, rather than having drafty try to preserve the revision timestamp, here's a patch that just adds the hook, and then another module (say, workbench_moderation which is node specific?) could implement that hook, and use a database query to preserve the revision timestamp for nodes. The hook example in drafty.api.php shows exactly that!

I've put a patch in the workbench_moderation queue on #2824798-14: Forward revisions break publishing audit trail which takes advantage of the new hook #85 to preserve the revision history

@dsnopek
Related issues: +#2824798: Forward revisions break publishing audit trail

Kudos! [|:)

Have tested both patches and I agree that these updates to both Drafty and Workbench Moderation bring us back in line with the expected functionality of 1.4 of Workbench Moderation.

Marking as Review & Tested... will do for related issue.

Hopefully the maintainers of both can co-ordinate a timely release of both updates.

The functionality is working as expected, but the instructions on the admin form are confusing. In order for deletion to happen on cron, both boxes have to be checked. And the "remove now" button only removes anything if deletions have been queued for cron. I'll update the patch.

Updated patch changes the wording of the instructions and also hides the 2nd button if the 1st button is not checked. See screenshot of collapsed fieldset:

When you check the button that enables deletion, the other fields appear:

@cboyden

Thanks... I agree settings were not entirely clear, appreciate the update.

I'll keep as needs review and hopefully other commenters and maintainers can chime in here and we can promote to reviewed & tested.

@cboyden patch works for me! Sorry about that. I originally wrote the admin portion I should have made it more clearer.

Since all I contributed was a hook definition (which was RTBC'd by someone else) I think it might be OK for me to RTBC @cboyden's changes to the admin form, which look great :-)

1. +++ b/includes/drafty.admin.inc
   @@ -0,0 +1,64 @@
   +    '#title' => t('Configuration settings for Drafty.'),
   

   "General settings" should be easier and is already translated.

2. +++ b/includes/drafty.admin.inc
   @@ -0,0 +1,64 @@
   +    '#title' => 'Enable deleting redundant revisions',
   

   t() missing

3. +++ b/includes/drafty.admin.inc
   @@ -0,0 +1,64 @@
   +    '#title' => 'Delete redundant revisions on cron',
   

   t() missing

Thanks for the review @hass, I've updated the patch.

@cyboyden @hass @dsnopek

All looks good to me.

If anyone else wants to chime in here we can perhaps move it up to RTBC?

Looking at the interdiff on #94, all of @hass' review in #93 has been addressed! There were also some indentation changes which seem mostly fine to me. As a super minor point, I think these two closing parenthesis would be better on one line:

+++ b/includes/drafty.admin.inc
@@ -0,0 +1,67 @@
+  )
+  );

... so, it'd be better as:

));

I'll leave it up to the maintainers to decide if that's actually important or not. In any case, I've definitely reviewed, but haven't tested this new patch.

This patch works for me also. I do have a question for everyone that's testing. We have been using an older patch for this issue and very randomly we have an issue where nodes don't save and users see 'No active batch found' error appear. This is most likely caused when Drafty tries to do a cleanup but there's nothing to cleanup. This problem appears very inconsistently so just wondering if anyone else has seen this happen.

@smustgrave I haven't seen that in my testing, and in fact, looking at the latest patch, it shouldn't be possible to set a batch if there is nothing to clean up:

+++ b/drafty.module
@@ -241,20 +322,51 @@ class Drafty {
+    if (!empty($operations)) {
+      $batch = array(
+        'operations' => $operations,
+        'finished' => 'drafty_delete_batch_finished',
+      );
+      batch_set($batch);
+    }

The batch is only set if there are some operations to perform.

Do you know which patch you're using? If you look in that patch and it's always setting a batch, even when there's nothing to do, then it's the old patch that is the problem :-)

Going to mark as RTBC because there has been review and testing on the latest patch from a couple different people!

I was using the patch from #52 but still see the issue when I apply the newest patch. I'm not saying it may be this patch but one of the symptoms we see is No Active Batch which I think is caused by drafty trying to do cleanup. Maybe there is a conflict with Field Collections? Just guessing just wanted to throw this out there in case anyone else has the same issue.

@dsnopek looking at the patch and your comment in #98 won't $operations always be true since $operations is given a value no matter what? A few lines above it

$operations[] = array(
              'drafty_queue_delete_revision',
              array($drafty_queue_item),
            );

So if there's any issue in creating the $drafty_queue_item won't the batch fail? I've used the patch in #94 and still have instances where we get No Active Bath error. The extra node isn't cleanup when we see this. We haven't narrowed down the steps to cause this consistently but it definitely happens. Thoughts?

My reading of the code is that that operation won't get added if deleting revisions is not enabled OR if deleting via cron is enabled. But, also, if the batch has operations, I see no reason why the batch should fail? Figuring out steps to reproduce would make it a lot easier to say if there's a bug here or not..

The only reason I suspect this patch is. When the node save works and patch does it's cleanup there are no errors and the message says "Your node was saved" and "1 revision deleted." When we see the error I check the moderation and the revision isn't cleaned up. I definitely don't think this patch is the cause just a symptom of the other issue. Maybe the $drafty_queue_item doesn't get all the values and that breaks the operation maybe another check before the batch_set is needed? I our system delete revisions is enabled but delete with cron is disabled.

This issue is definitely caused by something else because I used the patch I wrote in #59 for months before seeing this happen about a month ago.

@smustgrave

When testing the roll out of this patch to one particular site I noticed a failure on the imediate deleting of revisions.

I narrowed that down to the Heartbeat module, in particular the heartbeat_defaults child module that implements Rules on the Node Update & Node Add events. This rule caused the failure. I didn't see any particular watchdog errors.

I didn't debug this as at the time because Heartbeat wasn't actually a required module, so it was disabled.

Can I ask if you have any Rules that act on the Node Save or Node Update events?

I've also done some minor Rules testing on Node Update event and there is an issue with the removal of the forward revisions when a simple rule, such as promote to front page, is inacted on the Node Update event.

More testing to be done to see where the issue lies...

I could imagine a number of ways that contrib modules could break the immediate deletion of revisions. For example, if they wanted to redirect the user or set their own batch. Deleting on cron is safer from a compatibility perspective -- maybe deleting on cron should be the default?

Just wanted to chip in that I applied both patches (WBM and Drafty) without any problem and it works like a charm. Thanks for all your effort and I am looking forward to the next stable releases including these patches

With issue #2468907: Refactor tests to remove dependencies closed/fixed and the most recent patch passing CI testing, is there any progress on further testing and/or committing the patch to a release anytime in the near future?

RTBC +1 for patch #94

Let's get this thing released!

Some tweaks to the comments, no code changes.

One small thing - the "Delete redundant revisions now" button on the admin page uses the {workbench_moderation_node_history} table, which won't exist if Workbench Moderation is not installed. Reading through the query that it runs I'm not completely comfortable with it anyway? Has anyone reviewed that part in detail?

This patch just hides the "delete now" button if the Workbench Moderation module is not installed.

So could someone please talk me through drafty_delete_now_submit() and clarify what it's supposed to actually do? Thanks.

drafty_delete_now_submit()

is a submit handler for the revision cleanup "Delete redundant revisions now" button. So when this button is clicked, the submit handler is called.

What it appears to do is delete current unpublished draft revision history from the workbench_moderation_node_history table. Current in this context would mean only one record (?) would be current at the time according to what I've seen (? but not 100% sure on this).

I'm not sure how much we've actually tested this "Delete redundant revisions now" button.

We've started QA testing on patch #94 and expect to be using it in production soon.

@gdaw , have you tested the "Delete redundant revisions now" button yet?

Rerolled after joseph.olstad's patch was committed to fix the tests.

IMO, I think we should just get rid of the 'delete now' button. After the fact, we're already missing some information (namely, the revision we're replacing) and so it won't have the desired affect in most situations. This is something that really needs be enabled as revisions are being deleted.

Unrelatedly, but if we're getting close to merging, I also think we should be deleting in cron by default (rather than the reverse) because the batch job on save can break other modules (see #105).

@joseph.olstad: I understand the fact the function is a Form API submission callback ;-) the question is more about whether it does what people expect it to do? It doesn't run drafty_queue_delete_revision() so it's running different base logic, it doesn't trigger the same hook, it interacts directly with Workbench Moderation while the other logic does not. So, in short, I'm considering removing that portion of the patch entirely and let it be dealt with in a follow-on issue.

ya maybe drop the "Delete redundant revisions now" button , and commit what's best, and we can open a new issue to follow up for the "Delete redundant revisions now", as it seems like a lesser priority and maybe needs some more thought.
take @dsnopek advice.

This makes a few changes:

• The "delete now" button has been removed.
• Both of the variable_get('drafty_delete_with_cron') calls now default to TRUE, whereas before they were different.
• Adds hook_uninstall to remove the two variables.

@DamienMcKenna, nice work thanks!

Any feedback on the latest patch and changes, folks? The tests pass, which is great, but I'd like to hear some feedback from people who are using the patch. Thanks.

I don't have time to test, but looking at the latest bunch of interdiffs, this all looks great to me! Thanks, @dmckenna, for taking the lead on this :-)

I've openned a followup issue for the removed (postponed) delete revisions now functionality. (with patch)
#2911920-2: "Delete redundant revisions now" button

Committed. Thanks everyone!!

Woohoo!!! :-)

Awesome! Thank you all!

Team - We would have loved to use this module along with the Workbench Moderation module, but since this module is still in beta, we couldn't proceed ahead due to potential security issues.

I have two questions:-
1. What is/are the issues because of which this module is not being pushed to a stable release yet?
2. What's the roadmap toward the D7 stable release of this module?

Appreciate your time to answer these concerns. Thanks!

Please see the Plan issues, specifically #2853676: Plan for Drafty 7.x-1.0-rc1 release.

Thanks, @DamienMcKenna, for providing the right link!

Hi all,

Thanks for everyone's awesome work at fixing this issue! I know there are many people who use workbench moderation and need this fix. I am a little confused on what the proper way to test this solution would be today. I believe I start with updating Drafty to 7.x-1.0-rc1, but there is also mention of a patch for workbench moderation. Could someone point me in that direction?

Thanks again!

My experience when removing the redundant revisions is that instead of the published -> published revision being removed, the published->drafted revision is removed. When viewing revision history or comparing revision history the data does not return as expected.
Looking at the revision history the 'published -> published' revision also sits on top of the currently published revision which is very confusing.

See the known and related issue with Diff: https://www.drupal.org/project/diff/issues/2881771#comment-12787894

Also the patch to address the forward revision issue:
https://www.drupal.org/project/workbench_moderation/issues/2824798#comme...