Revisioning may not be compatible with certain modules that use hook_node_access_records() (e.g. Domain Access)

Edit: update title

Thanks Rik,

Basis revisioning workflows have become such a common interest/demand that I've needed to find a nice scalable solution. Recent projects have not leveraged nearly the full potential of Revisioning, but knowing that the flexibility is there makes it seem like the general way to go.

Note that my analysis may not be accurate, as I was just stepping through a bunch of trial-and-error in a specific use case, but I have noticed that commenting out line 514 in revisioning.module gets things back on track. I also noticed several comments in the revisioning code about the D7 "flawed" handling of internal revisions, that required some workarounds in this module. Perhaps those workarounds are inadvertently leading to this unwanted behavior?

Humm, I just discovered that commenting-out line 514 may indeed have some functional consequences. Though it does solve the problem that I noted at the start, it seems to remove the ability for a user to override the default publishing status for a content type upon creation (e.g. choosing "create a new revision and moderate" will not force the revision to be unpublished if the default publishing status for the type is "published").

Hummm, I think I may have figured this out, as I just needed to pay more attention to what was happening when a node is being "auto published". From what I can see, whenever a "revision operation" is happening, revisioning is always making a binary decision on what to set the node's status ($node->status) to upon save, and is not considering what it was before the save. So it seems that there are cases when $node->status will be set to "unpublished", when it was previously "published", and should in fact not be changed at all.

This seems to play-out later in the node access stuff as some of the node access operations kick-in while this $node->status variable is set incorrectly.

Anyway, it's looking like simply changing line 514 in revisioning.module from:

$node->status = $auto_publish ? NODE_PUBLISHED : NODE_NOT_PUBLISHED;

to:

if ($node->status == $auto_publish) {
  $node->status = NODE_PUBLISHED;
}

Might take care of this. I'll make that into a patch in a moment, but it should be a quick thing to test either way.

Here is #7 in patch form. It's a tiny patch.

I'm hoping others can review with setups that use node access modules besides domain access. Also, I have yet to really try all the different revisioning options (types of revisioning actions, etc.) under this patch, but so far so good. I'm also hoping Rik can confirm things on a code level.

Cheers

Sorry, I made a syntax mistake in my comment in #7 which carried over into my patch. Please check out this one instead.

Glad to hear this might be a simple fix. I've tested it out in all the different editorial workflows that we use in one of my active projects (that relies on revisioning), and things appear to be working as expected under this patch. Hoping others experience the same with some more testing.

Also, if this does get committed, I'd certainly be honored if I was able to get git commit attribution (assuming it's justified here). I just realized that I didn't roll this one with git format-patch.

I'm happy to investigate further if issues come up (and it's something within my own range of knowledge). This is certainly a module we want to keep robust in D7.

Hi again,

I'm seeing another place where revisioning may not be playing nice with content access permissions. It seems that the fix I mentioned above gets things working correctly when choosing various moderation options via the edit page, but there still seems to be an issue when later publishing a moderated revision via the "publish" tab (that is part of the revisioning options).

Basically if I create a node that's subject to revisioning, and I chose not to publish it immediately (moderate instead), then all works fine... my node goes into a moderated state as expected and access is controlled as expected. However, if I later go to my revision and choose to publish it via the publish tab, I ultimately end up getting an access denied message for all users who do not have the node "bypass access control" permission. So it's looking like hook_node_access_records() may also not be getting invoked correctly within this workflow.

Of course, this is really only an issue when content access permissions are at play. In my case I'm needing to work with Domain Access and I'm wondering if people with other content access modules are also seeing this new problem?

I may have a solution to #14 now. It looks like publishing (via the tab) happens in _revisioning_publish_revision(), which primarily updates node and node_revision tables directly with db_update. The problem seems to be that it does not call anything which invokes hook_node_access_records(), so the node access permissions are not updated via this publishing workflow.

Adding the following at the end of _revisioning_publish_revision() seems to ensure that hook_node_access_records() is invoked (via the node_save()).

// We also want to give various hooks a chance to be invoked upon making these
// changes (such as hook_node_access_records()), which can be done with a
// simple node_save on the actual node.
$node = node_load($node_revision->nid);
node_save($node);

The attached patch captures this change along with the original change in #9. This one was made with git format-patch.

I'm not sure if there is a better way to do this? Maybe Rik will be able to weight-in on that?

Thanks Rik!