Rewrite to use the new Drafty module

This sounds interesting. It would be great if we could focus all efforts of solving those issues (drafts / revisions) in one location.

The main problems I've seen I've seen is with overwriting the current revision somehow. Either saving entities with $entity->revision = FALSE during workflow operations (i.e. publishing a draft revision) and/or using field_attach_update() to workaround core's lack of support for writing revisions without touching base tables etc.

Hope with drafty is to 1. Not have that set of problems since it'll always force a new revision all the time (then at some point deal with revision proliferation) 2. Provide a single low-level module that any workflow creating draft revisions can rely on so if there are issues they can be handled in one place.

I don't have clients using workbench_moderation atm so I won't work on this personally unless that changes, but happy to review if someone else takes this on.

So I'd missed revisioning module when reviewing other work, but it doesn't look like it solves the various issues completely either.

If you look at revisioning_node_update() it's following a slightly different approach to drafty - it 'resets' the {node} and field_data_*() tables manually to match the previous published revision state, as opposed to drafty which always uses entity_save() everywhere. This means it needs to handle file entities, taxonomy index etc. individually. Also incompatible with alternative field storage.

Also it looks like it would suffer from at least one issue I've seen with CPS and field collections - where the field collection archive state isn't updated correctly for the published revision - this is tracked in field_collection_entity_update() (see current version of CPS module - various issues with field_collection is why I started the drafty project).

http://cgit.drupalcode.org/revisioning/tree/revisioning.module#n599

Field collection actually marks the field collection item entities as archived or not archived based on the status of the parent entity referencing them, that's not the case for any of the other reference modules so for the specific problem I'm thinking of it's not an issue. File usage is similar though and that can go horribly wrong with the update-in-place method.

The ability to say 'I want this version of this entity to be referenced by this version of this entity' isn't something that CPS deals with - if you have an edit to one entity that references another entity that you also edit at the same time, that can be handled by keeping both edits in the same changeset.

I'm not convinced per-revision entity references is a good idea - in the published version of the site you just want to reference the default revision, anything else bypasses field cache (or entitycache).

If the entity reference field is referenced by both entity id and revision id, then whenever you load the entity based on that reference you need to specify the revision ID.

Both core field cache and entitycache don't both caching when a specific revision ID is requested, so it'll be a cache miss on the field cache.

See https://api.drupal.org/api/drupal/modules!field!field.attach.inc/functio...

How about "pass the buck" from State Machine to drafty then?
I'd really like to see this solved in a central place.

Another pain point with the current architecture: #1445824: Support for Migrate module

I started working on this locally. But it's not quite there yet

If we do a rewrite, will it get rid of the workbench_moderation_store() function added to drupal_register_shutdown_function(). This has cause major problems, with resaving the live revision. This causes undesirable results when other modules that are using hook_node_save() and/or hook_entity_update()/hook_entity_insert().

Using drafty makes any drupal_register_shutdown_function() obsolete.

We've been working on this internally for a project, and here's the patch we have, which includes proper test coverage.

Patch #21 is for the 7.x-1.x version of Workbench moderation. Just a suggestion - If the hook_entity_presave is changed back to hook_node_presave, and if the function workbench_moderation_set_node_state is moved lower into the file, the diff would be easier to review.

This is very good news. We have been using Workbench moderation module 1.x, but often there are problems with field collections, CER, programmatic saving of nodes, and others. I will test this patch when I find the time.

IIRC, we have to use hook_entity_presave() because of the callback order. Drafty uses it, and if we fire in hook_node_presave() our changes are not respected.

Aha, OK :). Ignore what I said.

OK, testing #21, it does appear to work, and I see no regressions. Great job, agentrickard. I think it is better to focus on that patch, rather than on the 2.x branch.

Unfortunately for me, #21 doesn't automatically resolve all of my problems related to field collections, and synchronization of workbench-moderation-enabled nodes on Drupal sites via services.

I think it is better to focus on that patch, rather than on the 2.x branch

Well, the 2.x branch uses state machine - and for state machine there's the 3.x-drafty branch we're testing for a project atm. :)

So is anyone willing to call this RTBC? Are the test cases sufficient?

I haven't reviewed the tests or run the tests. But I did review the code, tested manually, and I am using the patch in a project. I may look at the tests later but this will be after couple of weeks, probably.

Now that I see the test changes, they are not many. But for some reason I am getting failures. Not sure if there is any regression or they were like that initially. If there is no regression, I say this is RTBC, but need second opinion.

Test failures are attached.

Need testbot to test on 7.x-1.x

FYI: I just merged the Drafty integration into the State Machine 7.x-3.x branch which is used by Workbench Moderation 2.x

No idea what the test failures are, but from a 'drafty' / CPS maintainer perspective, this would be RTBC (ready to be committed) - I did not test it, but Miroslav did ...

Hoping that setting RTBC status will re-run the bot on the patch - can be set back afterwards.

Also not sure how 3.x handles the integration.

Maybe it needs to be set to Needs Review? That test is still stuck. If this doesn't work, anyone know how to get it rolling again? We've got both the old & new QA systems enabled on this project.

I'd re-upload the patch.

reup

Does anyone else feel really uneasy making this dependency/change on the stable 1.x branch? Also confused about the very low number of test passes returned by the new test bot.

Well you are probably right about uneasiness and dependency things. But the testbot failures don't make sense at all. They look like problems with the testbot.

#42: The new number is just the number of tests, but if you look at the detailed results you see that it ran all the tests.

The old testbot counted the assertions - it is probably broken for good now though ...

Should we open an issue in testbot issue queue? I really don't understand testbot problems, and the testbot FAQ is not very helpful.

Really I don't understand what's going on with the testbot, bu let's try this one more time.

Only changed the dependencies in setUp().

The attached patch is based on #46 and adds drafty as a test_dependency in the .info file as per #51, hopefully this will get the test bot to pass.

Just patched & pushed 7.x-1.x with #51 - hope the test bot can run now.

Let's try this again. Same as #53, only change is info file.

After applying this patch, translating content with more than one revision doesn't work properly. Once there is more than one revision, the translate tab still shows links to the published revision, and there is no apparent way to translate the current draft revision.

@nicrodgers

Are you sure this patch is what caused the problem? Can you give some steps, or requirements to reproduce this?

@nicrodgers

I tried to see how it works with i18n_node + translation + workbench_moderation (without patches). It is translating the published version and not the current draft, and there is no change/regression caused by this patch.

Please open a separate issue as it is not related. If you still think it is regression introduced with the patch, please provide steps to reproduce.

To me, this is RTBC. I have used the earlier version for a while. And all that's changed is the test dependency to fix the testbot failure.

@MiroslavBanov

This patch doesn't cause the problem specifically, sorry for the confusion.

There is a patch in https://www.drupal.org/node/1707156 that successfully integrates workbench moderation with entity translation, enabling translation of revisions through the workflow process. That issue was marked as postponed, as a number of developers said that this issue (rewriting to use Drafty module) would make that patch unnecessary. I wasn't sure which of the two issues to feedback on, so I added it to both. Sorry for the confusion.

RTBC + 1, patch still looks good to me.

Assuming the concerns from #42 have been alleviated, does anyone have any reservations about this latest patch, or should we commit it?

I'm assuming we'll have to put something in the release notes about installing Drafty.

It should probably be a new branch given the new dependency?

That's what I was wondering. 2.x is used for the State Machine / State Flow implementation. So a new 3.x then?

Is 2.x dead? Then we can overwrite it...

+1 overwriting 2.x branch. #1316314: Workflow Architecture for Workbench Moderation is what 2.x is about and that issue is verrrry unloved

Why not commit to 7.x-1.x ? The other concern is that this work will also get lost, 1.x is VERY active

A 2.x DEV version was never supported. There was also no working upgrade path as I know. They are lost on all ways. If we now use 2.x everyone knows this is the next major version. I hope there is now a working upgrade path and also a upgrade path to D8.

Seems simpletest to make sure 7.x-2.x is marked completely unsupported and has no releases, then commit this to 7.x-3.x. Don't think it's that confusing if there's a dead 2.x branch.

what happens in the future with this new 3.x branch then? how does anyone know it's got anything todo with Drafty? why not merge this into 1.x and close 2.x?

@dgtlmoon it'll need to be in the release notes and 1.x can move to unsupported.

The advantage of a new branch is less chance of people missing they need to do something extra when updating then getting fatal errors if they don't.

@catch sure, just trying to see that this work does not become another 2.x branch left behind

"The advantage of a new branch is less chance of people missing they need to do something extra when updating then getting fatal errors if they don't."

What are those 'something extras', can you write tests for it?

What fatal errors exactly are you referring to? should the issue be moved be to Needs Review? I'm still voting for 7.x-1.x there

I still see no clear way or plan that the existing 1.x branch and this 'new branch' you propose will ever become the one branch

What are those 'something extras', can you write tests for it?

You need to download drafty module before updating.

I still see no clear way or plan that the existing 1.x branch and this 'new branch' you propose will ever become the one branch

1. Create 7.x-3.x branch
2. Commit this patch to it
3. Release 7.x-3.0
4. Move all 7.x-1.x issues to 7.x-3.x
5. Stop committing anything to 7.x-1.x
6. Mark 7.x-1.x unsupported

I'm with @catch on this one. Was just about to post that plan myself.

I have a question about the upgrade path. Does this patch not require a single DB update? Just wondering that such a major change does not require an upgrade... Is it missing or not required?

I also think it is safest to move to 3.x, and like the plan in #83.

@hass

Does this patch not require a single DB update?

> Correct. It really is not needed.

@DamienMcKenna that looks right to me.

Now that this is finalized, can we tag a new 7.x-1.5(-beta?) release?

Don't we need to include #89 in the patch first though, or do we want to do these separately in the new branch?

Here's both patches merged.

Filename scares my build process

I'm still confused why we're adding a major dependency change in a stable 1.x branch. We should be doing this in a 7.x-3.x branch.

I created a new 7.x-3.x branch and reverted the drafty test dependency from the 7.x-1.x branch. I'll assign this issue to the branch once it becomes available.

Will need to be re-rolled because the update hook should be workbench_moderation_update_7300() now.

@davereid that's very heavy handed to just move this to 3.x branch without explaining why you don't understand, not really feeling the love here

Yeah that's unfortunate, would have been great to have it already widely installed on 1.x. How close is 3.x to being as stable as 1.x?

On the 1.x branch can we make it optional so it's enabled for new installs and disabled (but can be opted in to) for existing installs?

New dependencies should not just be added in a branch after it has a stable release. That kind of major change warrants new branches, especially if those changes are not optional. It has been discussed several times already in this issue that we should move this to a new branch, so I was acting on that.

Whilst testing this #2730265: Try to avoid saving a new revision when restoring the published revision

    // Doing this in hook_entity_update() so that the entire process is
    // completed within entity saving. However this results in two entity saves
    // within entity insert. The other option is hook_exit(), which is not
    // better since for example that would happen outside the transaction.

yeah so it seems it's probably well best to keep this code away from the 1.x branch, this could still break things

drafty maintainer says that a double save and race conditions are unavoidable as it currently stands with the drafty module.

where does this leave the work here? it does not resolve the original issue as stated by the owner

@dgtlmoon
I don't get what you're saying in #107 and I don't get why you're making this a needs work in #108. Please explain.
With using Drafty, things are a bit better than without Drafty, because we avoid register_shutdown_function(). There are still two entity_update happening. That's true for both branches.

As for the stability of 3.x, it is exactly the same as 1.x because we branch from 1.x, and there is only this change in it. The question is how quick we can deprecate 1.x and document the upgrade, so the community can focus on 3.x branch.

WIthout any benefit around the double entity-saving, I'm not sure this is worth the effort at this point, especially in fragmenting work and efforts.

@Miroslavbanov see the issue linked iny comment above. It is based on what the drafty maintainer says. There is simply nothing for me to explain "This is unavoidable with forward revisions in 7.x. Workbench,the version of cps before drafty, and other modules were doing this logic in hook_exit() or directly updating the node table both of which lead to data integrity issues or race conditions.

If someone can come up with a non-race-condition alternative that'd be great, but you have to save two revisions and core doesn't have an Api for saving only to the revision tables."

if you read this issues original description you can see this issue has wandered somewhat

Does this patch cause Race conditions? Does it resolve Race conditions? I can't say for sure. I think it helps resolve some of the linked issues that are about batch/bulk updates, and the migration module specifically. The issues seem to be more about data integrity and consistency than about performance.

As for the stability of 3.x, it is exactly the same as 1.x because we branch from 1.x, and there is only this change in it. The question is how quick we can deprecate 1.x and document the upgrade, so the community can focus on 3.x branch.

Then the plan in #83 sounds great :)

The double entity_save() is unavoidable in Drupal 7. It's fixed in Drupal 8 but that doesn't help 7.x users and it's not backportable.

I first suggested the drafty forward revision approach about six years ago in a Drupal 7 core forward revisions issue (that got derailed). To not do that, you'd need to be able to save an entity without touching the main table, which there is no API for - neither for entities themselves or for field API.

The difference with drafty and workbench currently is not doing the logic in hook_exit(). The hook_exit() causes race conditions and data-loss, and is why busy sites get corrupted data and field_collection gets broken. If you look at drafty and CPS's code, it has zero special casing for field_collection, only a couple of workarounds for entity_translation and title module bugs/limitations. This is what the issue summary identifies and is a valid reason for the rewrite. I wrote drafty specifically to address similar race conditions in CPS for a Tag1 client - it was using the hook_exit() approach before that.

To summarize, there are three ways to do this, all Drupal 7 workflow modules do some version of these:

1. drafty's method - no race condition, but double save.
2. Save in hook_exit() - race condition and double save.
3. Manual queries on the node table - race conditions, breaks other modules worse than #1 and #2 and no workaround because it's bypassing APIs altogether.

I'm not currently working on the project that's using CPS, nor any sites using workbench, so I have limited time to help with this. #2379071: Use drafty module for draft revision handling is the equivalent CPS issue if people want more background.

Also this is a data loss bug, not a feature request.

It isn't in 7.x-3.x yet, and it doesn't apply anymore to latest 7.x-3.x. Needs re-roll.

I gave it a shot, re-roll against current 7.x-1.x (52ff3ad818c62a4e5bd9d7e8ad420e22f2ff5e74).

Forgot to include changes in #106, hold on.

I just reviewed the re-rolled patch again and it seems like I botched the merge and some changes are missing. Please don't use the new patch! I don't have time to re-roll it again right now, but if not any one else beats me to it I will give it another go on monday.

Ok, rerolled against 7.x-3.x. I think I got it right this time.

Enabled, need to have the patch re-uploaded to test.

OK, let's get this patch in. I have been using this since #21, and I am now using it on a new project as well.

After this patch, would anything be blocking a 7.x-3.0 still?

To the maintainers - please put some really informative text on the project page about exactly what is in the 3.x branch once this is merged in

As this was essentially stuck I decided to commit and push it despite the agreement I had to only touch the 2.x branch.
I did my best to give proper credit.

@DamienMcKenna Damn, I was afraid that I missed something :( - thanks for clarifying
@agentrickard sorry about that! :|

#137: You can still tick the right checkbox and revert the commit, then redo it with proper credit - if you want to.

That is what we do in core sometimes.

Thanks, everyone, for pushing this forward.

I'm not sure if this is the right thread but I was wondering what is the benefit of Drafty's forward revisioning? Why does it need to save twice?

@smustgrave

It saves twice with or without Drafty.

See also: https://www.drupal.org/node/2376839#comment-11214317 for details.