Revision flag is ignored
rötzi - February 1, 2007 - 01:44
| Project: | Drupal |
| Version: | 5.7 |
| Component: | node.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Description
When saving a node as a user which has not the permission to change the 'create new revision' status, the flag is ignored altogether. Thus no new revision is created in that case.
To reproduce: Set the 'Create new revision' flag for a node type. Then create a new node as a user which cannot set the flag in the edit form (a user without the 'administer nodes' permission). Then edit the node.
No new revision is created.
The patch changes the loading of the flags and always loads the revision flag from the variables.
| Attachment | Size |
|---|---|
| fix_revision_flag.patch | 959 bytes |
#1
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.
#2
Thanks, the attached patch resolves this issue for me on version 5.1
#3
Code comments should be in "Proper sentence form and capitalization."
#4
is this better?
// Workflow option 'revision' is not saved in the node table
// and has to be loaded from the node type options.
#5
#6
Since no one objected.
#7
I think the code to change is in node_form():
<?php// 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.
#8
If you look at the original code in node_submit where I introduced my change:
<?php// 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.
#9
Just a sec.
This is not undoing Node options behaviour on editing by non-admin user, is it? Because that patch is very necessary.
#10
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.
#11
Got it. Thanks! I will try and test this tonight.
#12
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).
#13
#14
+1 for the patch in #4.
#15
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.
#16
Just keeping an eye on this. If this is critical then will it get rolled into 5 (.2)?
#17
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.
#18
#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.
#19
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.
#20
Are we still waiting on Neil's opinion on this before its wrapped up? If so, someone point him this way please :)
#21
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.
#22
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.
#23
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. :)
#24
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.
#25
I didn't realize that actually happened. Then this can be a bit more simple.
#26
neclimdul: can you confirm that Neil's last patch works? Thanks! :)
#27
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.
#28
I've committed this to CVS HEAD. Thanks guys!
#29
Committed to 5.
#30
#31
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.
#32
the next official release will contain the fix.
#33
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 ;?
#34
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.
#35
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.
#36
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.
#37
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.