Revision flag is ignored [#114822]

Comment	File	Size	Author
#25	node.module_55.patch	2.48 KB	drumm
#22	node.module_54.patch	2.72 KB	drumm
#19	node_revision_flag.patch	1.13 KB	neclimdul
#17	fix_revision_flag_1.patch	1.09 KB	neclimdul
#8	fix_revision_flag_approach2.patch	845 bytes	rötzi
#4	fix_revision_flag_0.patch	922 bytes	rötzi
	fix_revision_flag.patch	959 bytes	rötzi

Comment #1

rötzi commented 1 February 2007 at 01:55

Note that the same problem probably affets version 6.x too, but I couldn't test it since I don't have an installation ready. But the fix should work exactly the same.

Log in or register to post comments

Comment #2

Flu commented 1 February 2007 at 18:06

Thanks, the attached patch resolves this issue for me on version 5.1

Log in or register to post comments

Comment #3

RobRoy commented 1 February 2007 at 21:18

Status:

Needs review

» Needs work

Code comments should be in "Proper sentence form and capitalization."

Log in or register to post comments

Comment #4

rötzi commented 1 February 2007 at 21:39

Status	File	Size
new	fix_revision_flag_0.patch	922 bytes

is this better?

// Workflow option 'revision' is not saved in the node table
// and has to be loaded from the node type options.

Log in or register to post comments

Comment #5

rötzi commented 1 February 2007 at 21:39

Status:

Needs work

» Needs review

Log in or register to post comments

Comment #6

rötzi commented 7 February 2007 at 00:56

Status:

Needs review

» Reviewed & tested by the community

Since no one objected.

Log in or register to post comments

Comment #7

drumm

he/him

NY, US

commented 7 February 2007 at 08:14

Version:	5.x-dev	» 6.x-dev
Status:	Reviewed & tested by the community	» Needs work

I think the code to change is in node_form():

  // These values are used when the user has no administrator access.                                                                                                                                               
  foreach (array('uid', 'created') as $key) {
    $form[$key] = array('#type' => 'value', '#value' => $node->$key);
  }

This will need to be added to Drupal 6 first.

Log in or register to post comments

Comment #8

rötzi commented 7 February 2007 at 21:27

Status:

Needs work

» Needs review

Status	File	Size
new	fix_revision_flag_approach2.patch	845 bytes

If you look at the original code in node_submit where I introduced my change:

  // Process the workflow options and provide defaults. If the user 
  // can not administer nodes, ignore the form and either use the 
  // saved values if the node exists, or force the defaults. 
  if (!$access && $node->nid) { 
    $saved_node = node_load($node->nid); 
  } 
  else { 
    $node_options = variable_get('node_options_'. $node->type, array('status', 'promote')); 
  } 
  foreach (array('status', 'promote', 'sticky', 'revision') as $key) { 
    if (!$access && $node->nid) { 
      $node->$key = $saved_node->$key; 
    } 
    else if (!isset($node->$key) || !$access) { 
      $node->$key = in_array($key, $node_options); 
    } 
  }

you see that wathever node_form has set on the node is overwritten if the user has no access to the workflow options. The node is just loaded and then the settings taken from the saved node. The problem here is that the revision settings are not saved in the node and thus it is set to false no matter what. So a change to node_form would not solve this problem.

Another solution would be to not reset the 'revision' flag on the node since 'node_form' alredy sets it. I attached a patch which uses this apporach. It works, but I find the first one clearer since it doesn't depend on what 'node_form' does.

Log in or register to post comments

Comment #9

webchick

she/they

English

Vancouver 🇨🇦

commented 12 February 2007 at 22:13

Just a sec.

This is not undoing Node options behaviour on editing by non-admin user, is it? Because that patch is very necessary.

Log in or register to post comments

Comment #10

rötzi commented 12 February 2007 at 22:49

No. After I have seen the other issue, this is actually a bugfix for the patch there.
The problem is that the workflow options are always taken from the saved node for unprivileged users. This works for published, sticky and promoted since they are saved in every node, but the revision flag is not stored on a per-node basis.

This actually raises the question if the revision flag should be stored on a per-node basis, thus intorducing a 'revision' flag to the {node} table. This would be the most-consistent behaviour.

Log in or register to post comments

Comment #11

webchick

she/they

English

Vancouver 🇨🇦

commented 12 February 2007 at 23:55

Got it. Thanks! I will try and test this tonight.

Log in or register to post comments

Comment #12

webchick

she/they

English

Vancouver 🇨🇦

commented 13 February 2007 at 16:28

Status:

Needs review

» Reviewed & tested by the community

Patch from #4 tested and solves the problem. I like that approach best, because #8 looks like it could've merely been an oversight, while #4 says specifically "This is why we're doing it this way."

I'm also bumping this to critical: regular users should not be able to knock things out of revision control (especially by accident :P).

Log in or register to post comments

Comment #13

webchick

she/they

English

Vancouver 🇨🇦

commented 13 February 2007 at 18:31

Priority:

Normal

» Critical

Log in or register to post comments

Comment #14

BioALIEN commented 14 February 2007 at 17:46

+1 for the patch in #4.

Log in or register to post comments

Comment #15

dries commented 15 February 2007 at 07:35

I like the patch in #8 better -- #4 seems like a special case to me. I'd like to get Neil's opinion on this first.

Log in or register to post comments

Comment #16

spooky69 commented 18 February 2007 at 20:21

Just keeping an eye on this. If this is critical then will it get rolled into 5 (.2)?

Log in or register to post comments

Comment #17

neclimdul

He/Him

commented 18 February 2007 at 22:00

Status:

Reviewed & tested by the community

» Needs review

Status	File	Size
new	fix_revision_flag_1.patch	1.09 KB

Boy, I had to chew on this one for a while. In the end I kinda felt similar to Dries on #4 and I think #8 might lead to problems since we're not validating the revision field that is being passed straight from form_values. I could be wrong and this might be done by fapi but since this code is here I'm not sure.

So, in the end I put together some code that I think reflects the author of this blocks original intent a little bit better. Patch attached with a good amount of commenting.

Log in or register to post comments

Comment #18

edrex commented 23 February 2007 at 03:02

#17 seems sloppy on cursory inspection, specifically this:

   else {
     $node_options = variable_get('node_options_'. $node->type, array('status', 'promote'));
   }
+  $node_options = variable_get('node_options_'. $node->type, array('status', 'promote'));
+

Not voting a preference to any of the three, just pointing out that #17 seems sloppy.

Superficially tested #8 to work with drupal-5 branch. Also tested to work with Angie's revision_moderation module.

Log in or register to post comments

Comment #19

neclimdul

He/Him

commented 23 February 2007 at 03:51

Status	File	Size
new	node_revision_flag.patch	1.13 KB

Bah, sloppy I guess. I deleted that else at some point but apparently got it back in somehow. Here's another fixed patch against 5. I have the HEAD patch ready as well but not easily testable on head.

Log in or register to post comments

Comment #20

BioALIEN commented 26 February 2007 at 14:40

Are we still waiting on Neil's opinion on this before its wrapped up? If so, someone point him this way please :)

Log in or register to post comments

Comment #21

drumm

he/him

NY, US

commented 28 February 2007 at 08:06

Status:

Needs review

» Needs work

This is changing something I tried to change before and failed to get acceptance for- all node options will persist instead of revert back to the defaults when an unprivileged user edits a node. This is by design and IMO not always the right thing to do. Possible solutions:

- Switch the behavior (this patch), a bit too much of a change for 5.x.
- Provide an option, the concept is awkward and hard to explain.
- Maybe someday some workflow configuration will provide enough UI surface area for a sensical option.
- Persist only revision, this probably makes sense to do.
- Do everything on form-generation, there is no need to check things twice.

Log in or register to post comments

Comment #22

drumm

he/him

NY, US

commented 28 February 2007 at 08:35

Status:

Needs work

» Needs review

Status	File	Size
new	node.module_54.patch	2.72 KB

Here is a patch which removes the logic on submit in favor of doing it on form creation. Unlike the other three flags, the revision flag isn't stored on a per-node basis, so we always want the default value.

If the behavior of the flags is unsatisfactory for a particular site, hook_form_alter() has full reign of the flags for all users.

Log in or register to post comments

Comment #23

dries commented 28 February 2007 at 21:41

Neil's patch looks clean, but I haven't tested it yet. If someone can give it a good work out, then it should be ready to be committed. :)

Log in or register to post comments

Comment #24

neclimdul

He/Him

commented 1 March 2007 at 02:17

Status:

Needs review

» Needs work

I don't know why but this Neil's patch reverts the fateful node options patch.
Test:
You need 2 users, 1 with admin nodes privilege one in a role that is just able to edit nodes.
Unset the promoted to frontpage for a content type(page for me).
With your admin create a node and set it as promoted to the frontpage.
With your editor change some text and submit it.
Result:
The page doesn't show up on the frontpage and webchick is sad.

However I do like the idea of not futzing with this stuff in the _submit so I'm going to look closer.

Log in or register to post comments

Comment #25

drumm

he/him

NY, US

commented 1 March 2007 at 05:58

Status:

Needs work

» Needs review

Status	File	Size
new	node.module_55.patch	2.48 KB

I didn't realize that actually happened. Then this can be a bit more simple.

Log in or register to post comments

Comment #26

dries commented 1 March 2007 at 09:36

neclimdul: can you confirm that Neil's last patch works? Thanks! :)

Log in or register to post comments

Comment #27

neclimdul

He/Him

commented 1 March 2007 at 17:53

Status:

Needs review

» Reviewed & tested by the community

I like how we've removed a lot of code and ended up with better functionality. It worked for the previous test and it also got revision moderation working. Thanks Neil for making the "Right" patch for this and I think it can be committed.

Log in or register to post comments

Comment #28

dries commented 1 March 2007 at 18:18

Version:

6.x-dev

» 5.x-dev

I've committed this to CVS HEAD. Thanks guys!

Log in or register to post comments

Comment #29

drumm

he/him

NY, US

commented 7 March 2007 at 03:29

Status:

Reviewed & tested by the community

» Fixed

Committed to 5.

Log in or register to post comments

Comment #30

(not verified) commented 21 March 2007 at 03:30

Status:

Fixed

» Closed (fixed)

Log in or register to post comments

Comment #31

ak commented 25 March 2007 at 17:24

Hi, sorry but I am running a site on the latest official drupal release (5.1) and got the exact same bug that is driving me and my users crazy. Really annoying. The status is fixed and closed. Is this now fixed in the official release and I must redownload 5.1 or will the fix ship within the next official release and it's just fixed in head? What must I do to solve this critical bug in my 5.1 site. I really need to get this solved quick. Thanks in advice.

Log in or register to post comments

Comment #32

killes@www.drop.org commented 25 March 2007 at 18:12

the next official release will contain the fix.

Log in or register to post comments

Comment #33

JohnG-1 commented 26 March 2007 at 00:28

Sorry to butt in - I'm afraid this is all a bit beyond my mortal comprehension ...

I applied #25 node.module_55.patch (2.48 KB) to a nearly-live D5.1 site but it has not cured the problem ... (reported at http://drupal.org/node/130731)

Do I need to first install the 'other' patch (http://drupal.org/node/38451#comment-176357 #53 - node_default_workflow_options_sanity.patch_2.txt (2.24 KB))?

:( .. when is D5.2 due ;?

Log in or register to post comments

Comment #34

mercmobily commented 11 May 2007 at 03:34

Priority:	Critical	» Minor
Status:	Closed (fixed)	» Active

Hi,

I *hate* reopening bugs.
But...

Well, there is still something that it's not quite right.
Basically, right now if you:

* DON'T have $access to the extra fields
* Don't have the extra fields in the form

Then it all works fine.
However, if you:

* DO have $access to the extra fields
* Don't have the extra fields in the form the form

Then you're in trouble, because the default value still won't be considered.

I am having this problem right now with node_article.module (soon to be released): "chief" can edit some fields of the article using a custom form (which then calls node_save after changing a couple of flags). "Chief" has access to the extra workflow fields, but this custom form doesn't show those fields for chief.

The result? When a normal user saves the node using the custom form, then the default workflow value for revisions is taken into consideration. HOWEVER, if "chief" does it, then the default values are not taken.

At the moment, I have this just before the node_save():

// FIXME: We shouldn't have to do this.
$node->revision=1;

I could even make it smarter, and make it check the default value for this node type by hand. But...

Again, it's now a minor issue - whereas the ORIGINAL issue was indeed critical. But, I still think it's a problem.

Bye,

Merc.

Log in or register to post comments

Comment #35

mrsocks commented 17 July 2007 at 00:33

is this patch currently working correctly?

I need to use this module. Also,
Does anyone have a node.module they can send me? I cannot get the patch function to work correctly.

Thank you.

Log in or register to post comments

Comment #36

just_an_old_punk commented 2 May 2008 at 14:48

Version:	5.x-dev	» 5.7
Priority:	Minor	» Normal

I would like to use this module to moderate revisions on my Drupal 5.7 site. I am using the 'modr8' to moderate my new posts and that is working great. However, I have enabled revision moderation and updated a Page 'content type' to use the following 3 permissions, 'Create new revision', 'In moderation queue' (modr8), 'New revisions in moderation' as the default. I have a basic role 'contributor' which does *not* have 'administer nodes' privileges.

Unfortunately, when my 'contributor' role edits a page, the page is immediately updated and does not do into revision moderation.

Can anyone help? Is there a patch for 5.7 to make the revision moderation module work?

As a side note, I have tried this with both the 'Exempt adminsitrator...' setting on and off - I think I remember reading some post above about that setting. Neither way helped moderate my 'contributor' role revisions.

Log in or register to post comments

Comment #37

just_an_old_punk commented 4 July 2008 at 14:22

I have found a solution to the problem of integrating modr8 and revision_moderation by creating a workflow-ng rule to set the "New revisions in moderation" flag.

I posted it here http://drupal.org/node/253941.

Log in or register to post comments

Comment #38

dpearcefl commented 22 June 2011 at 00:00

Status:

Active

» Closed (won't fix)

Considering the time elapsed between now and the last comment plus the fact that D5 is no longer supported, I am closing this ticket.

Log in or register to post comments

Revision flag is ignored

Comments