Jump to:
| Project: | Comment Revisions |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | thinkyhead |
| Status: | closed (fixed) |
Issue Summary
I needed this for a client's website and found that a lot of things didn't work properly.
Unfortunately the client was on drupal 5 so this patch will need to be ported to d6 but the d5 & d6 versions of this module are pretty much the same so it shouldn't be hard to port the patch.
The patch includes the following fixes to the 5.x-1.1 version:
* Fixed the permissions so they don't overwrite the node revisions ones - #336022: Node revision permission hidden when this mode is enabled
* Overhauled the permissions so they make more sense (some permissions were actually never even used)
* Code cleanup to adhere better to coding standards
* Fixed incorrect use of system_settings_form() function
* Made so maximum number of automatic revisions can also be zero for infinite.
* Put revisions on comment form in fieldset
* Fixed incorrect cancel path in revert confirm form
* Made the comment_revisions_overview form themable
* On comment_revisions_overview form the operations are now only displayed if the user has the required permissions, fixed a bug with colspan on the operations field
| Attachment | Size |
|---|---|
| comment_revisions_new_D5.patch | 34.17 KB |
Comments
#1
Thanks, and great timing! I am just getting around to addressing a client request today for a D6 site. I will take your patch, apply it to the D5 version, compare it to the 6.x-dev version and run it through Coder to get a solid D6 version. I'll post the D6 patch shortly thereafter, and if I have any issues I'll be sure to mention them here!
#2
Cool that is good timing.
Hopefully when there is a 6 version too this could get committed.
If I come across any bugs with my initial patch I will report them back here too.
#3
Hi!
So, I'm still in the process of porting the module over to D6, and thus far I've taken your patched 5.x module and run it through Coder, cleaned up all the obvious bits, and now I'm in the process of testing it.
In testing the D5 module for comparison, I noticed that isUsrRev was never getting set to 1. The cause was a glitch in the permission name on line 182:
<?phpif ($user->uid == $a1['uid'] && !user_access('create owncomment revisions')) {
?>
Fixing this gets rid of the error.
BTW, just what the heck does the isUsrRev field indicate? The line of code above especially confuses me! "It's my comment, but I don't have permission to create my own revisions, therefore this is a 'user revision.'" What??
One thing I'm curious about is how come the original comment is being copied to the revisions table during the 'validate' op in hook_comment? Would it make more sense to do this in 'update' ... or is it too late by then?
Finally, I think this is a very handy module and I hate to see it being abandoned. Would you be interested in forking the module and calling it "Comment Revisions" instead of "GB Comment Revisions"? The projectname could be changed to 'comment_revs' or something similar. Or, I suppose one or both of us could write to the drupal.org admins and ask them to confirm that it's been abandoned and transfer ownership of the module to one of us - per the procedure outlined at http://drupal.org/node/251466
In the process of polishing it up for a proper release it seems prudent to change the names of some of the permissions to make it clearer what they mean. And maybe it makes sense to rethink them a bit, since, for example, user 'admin' always has 'force comment revision.' I can see why the original author had to create a permission that makes an exception, but there again user 'admin' always gets both! So it seems like this kind of thing should be a user&global setting rather than a permission.
Anyhow, I'm making good progress, and taking your version as authoritative. LasseP's D6 dev version has some additions, but I'm not going to look at those yet until my port is fully stable.
#4
Basically it has the concept of user revisions, which are the ones created automatically for users who don't have permissions to create their own comments (for their own comments). The concept of this in relation to users that have the 'create own comment revisions' is a little strange and could possibly be cleaned up a bit so people can better understand how it all works.
So a user revision and the user doesn't have 'create own comment revisions' then variable_get('comment_revisions_max_usr_revisions', 5) comes into play and will delete old revisions depending on that setting.
(I don't know who would have a use for that setting - if you want revisions why would you want to delete them? - that's why I added the unlimited option for that setting)
In regards to valudate vs update, I remember looking at that too but I think there was a reason it couldn't be moved into update. I can't remember exactly what. Having a quick look it seems that the part in the validate has to run before the update part. Because the validate part is saving the original version (if there isn't already one) and then the update is saving the new revision.
- A solution to this would be to make it so that all comments have revision entries all the time like how node revisions work. This would make more sense I think (people would expect all comments to have at least one entry in the comment revisions table) and would avoid having to do this. It just means having to add some sql into the install to populate the tables to begin with.
Yeah, it would be good to get an updated version in. If this module will be marked abandoned though there should be no need to create a new project, just use this one and rename the project page from GB comment revisions to comment revisions. That would be a better solution I think.
LasseP might be happy to sign it over if he hasn't got time for it anymore.
I don't have time to be the maintainer of another module though so if you want it that would be great. I can help out though if need be.
I definitely agree though that some things should be re-thought for a new version.
Especially permissions and possibly the database tables.
I didn't want to touch the database tables at first as it would impact upgrade path to the d6 version and I didn't have time to work on a d6 version to match. So if we keep the 5 & 6 versions in sync in regards to database and provide an upgrade path from the current version that will be ok.
#5
Patch included in 5.x-1.2 release and upcoming 6.x-1.0 release.
#6
#7
Excellent, thanks :)