I found after updating to Revisioning 7.x-1.8, when you create a new revision or save an update to an unpublished revision that are moderated, you are redirected to the currently published revision, not the revision you just created or edited.

This is being caused by the patch introduced in #2037655 Node access modules use wrong revision when revision moderation is used. This patch introduces an implementation of hook_node_access_records that essentially overwrites the node revision being save with the currently published one. 

/**
 * Implements hook_node_access_records().
 *
 * Replaces all node properties and fields to those of the current node,
 * so node access modules implementing this hook do not use values
 * from revisions that are still in moderation.
 */
function revisioning_node_access_records($node) {
  if (!revisioning_revision_is_current($node)) {
    $current_node = node_load($node->nid, NULL, TRUE);
    foreach (get_object_vars($node) as $k => $value) {
      unset($node->$k);
      if (isset($current_node->$k)) {
        $node->$k = $current_node->$k;
      }
    }
  }
  return array();
}

A reference to this node object is being stored in the form_state array which later results in an incorrect redirect value being set here

function _revisioning_form_submit($form, &$form_state) {
  // Don't redirect when creating new node, when not moderated or user doesn't
  // have access to the revision.
  if (isset($form_state['node']->nid) && !empty($form_state['node']->revision_moderation) && _revisioning_access_node_revision('view revisions', $form_state['node'])) {
    $form_state['redirect'] = 'node/' . $form_state['node']->nid . '/revisions/' . $form_state['node']->vid . '/view';
  }
}

I think replacing all of the properties of the node being saved with those of the currently published one puts the node object is a state that is counter to what most people would expect to find during the save operation.

Thanks

CommentFileSizeAuthor
#14 revisioning-2241705-14-redirect-after-save.patch925 bytessambonner
#2 revisioning-2241705-2-redirect-after-save.patch1.64 KBmilesw
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

Thanks Frank,
I agree with what you are saying.
So we have to find a way whereby node access is tested against the correct revision without affecting the form redirect....
Rik

milesw’s picture

milesw CreditAttribution: milesw commented
Status: Active » Needs review
FileSize
revisioning-2241705-2-redirect-after-save.patch1.64 KB

Here's a patch to try. This is probably just digging a deeper hole, but it fixes the problem...

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

Thanks Miles,
I have checked this into the dev branch.
Hopefully this will do the trick...
Rik

milesw’s picture

milesw CreditAttribution: milesw commented

FYI, #2 is not an ideal solution. It fixes the redirect issue, but other modules could still get the wrong node from $form_state depending on the order of the submit handlers.

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

Hmm... So, Miles, what you're saying is (I think):

IF there are other submit handlers (i.e. other modules) in play
AND they have a weight such that they come before Revisioning
AND they rely on a field that happens to have a different value on the new revision than on the current revision

THEN something could stuff up during saving of the node....

Rik

fbrooks’s picture

fbrooks CreditAttribution: fbrooks commented

Miles/Rik,

I share the concern that modifying Revisioning like this is going to lead to a bug somewhere else.

I would suggest that the best way to address this issue is to roll back the patch from #2037655 and look at addressing the concern in #2037655 differently. I think that issue might be better addressed in a separate module that adapts Revisioning to the specific node access modules in question, so anyone that does not require those node access modules is not impacted.

Frank

pat_trick’s picture

pat_trick CreditAttribution: pat_trick commented

We have likewise been affected by this issue; is there a status update on either a roll back or other update to resolve it? It is currently 2 weeks and a few days since last comment.

gregwbourne’s picture

gregwbourne CreditAttribution: gregwbourne commented

Yes, we are affected by this as well and would love a resolution!

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

As mentioned in #3 of this thread, Miles' patch was checked in on 21 April.
It is part of 7.x-1.x-dev.
So could you all please test 7.x-1.x-dev and if no one finds an issue with it then we can more confident of using it for the next official release.
Rik

gregwbourne’s picture

gregwbourne CreditAttribution: gregwbourne commented

RdeBoer, I tried 7.x-1.8+4-dev and it seems to work. After editing a page I'm bounced to the revision and not the currently published version. Thanks.

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented
Assigned: Unassigned » RdeBoer
Status: Needs review » Fixed

In the absence of further bug reports, I'm marking this fixed until it is no longer....

gregwbourne’s picture

gregwbourne CreditAttribution: gregwbourne commented

I tried 7.x-1.x-dev and still get the security update warning for revisioning. I also tried 7.x-1.8 on our test site and ran into the problem again (get bounced to currently published version after making edit to page).

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

Well the update warning could be because there is no official 7.x-1.9 yet. That is what we are aiming for if we don't get any reports that 7.x-1.x-dev is not working ok.

sambonner’s picture

sambonner CreditAttribution: sambonner commented
Status: Fixed » Needs review
FileSize
revisioning-2241705-14-redirect-after-save.patch925 bytes

Reopening this as the static node needs to be used in another revisioning submit handler too. Currently the revisioningapi 'post revert' hook incorrectly gets passed the most recently published node revision rather than the new node revision just created which renders it fairly useless.

The node object used in revisioning_revert_confirm_post_submit uses the object in $form['node_revision'] which, after the change in #2037655: Node access modules use wrong revision when revision moderation is used holds the currently published node, not the new revision. Patch attached changes this to use the actual saved node within this submit handler too.

Thanks,
Sam

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

Patch applied wth attribution.
Thanks Sam!
Rik

  • Commit 697510b on 7.x-1.x authored by sambonner, committed by RdeBoer: 
    Additional fix for [#2241705] (comment 14). Thanks Sam
RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

The above patch was undone, in favour of https://drupal.org/node/2037655#comment-8800693, which we hope will fix the two issues at once.
Rik

pat_trick’s picture

pat_trick CreditAttribution: pat_trick commented

It looks like this was rolled into 7.x-1.9. I've been testing it successfully without issue.

RdeBoer’s picture

RdeBoer
Location Melbourne
CreditAttribution: RdeBoer commented

Thanks for confirming it works!