[D6] Comment Limit by Role

Hi and welcome,

There are some coding style issues. You can use this online tool to detect the issues automatically: http://ventral.org/pareview/httpgitdrupalorgsandboxsaitanay1635606git-7x-10

* README.txt is missing, see the guidelines for in-project documentation.
* Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
* Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

To the settings:
I think the module becomes more flexible, if the site administrator can select on which period the limit applies:
* Daily Limit
* Weekly Limit
* Monthly Limit
* ...

comment_limit_by_role.install: Please use the variable_del() function instead of the db_query().

It is recommended to use a *.admin.inc file for the settings form.

$items['admin/config/content/comment-limit-by-role'] = array(
    'title' => 'Comment Limit By Role Settings',
    'description' => "Settings page for Comment Limit by Role Module",
    'page callback' => 'drupal_get_form',
    'page arguments' => array('comment_limit_by_role_form'),
    'access arguments' => array('administer comment limit by role settings'),
    'type' => MENU_NORMAL_ITEM,
    'file' => 'comment_limit_by_role.admin.inc',
  );

The comment_limit_by_role_setting() function is then no longer needed.

comment_limit_by_role.module, line 121: The string should be wrapped in a t() function.

Please explain the parameters for the functions: http://drupal.org/node/1354#functions

Regards, Christian

1) Bad line endings were found, always use unix style terminators.
comment_limit_by_role_settings.admin.inc
README.txt

You should set in your editor use the Unix line separator style. See http://stackoverflow.com/questions/986421/how-do-i-switch-between-window...

Here you can find some informations how Netbeans can be configured to be compatible with Drupal. But I have not tested it himself.

----

comment_limit_by_role.module: 91 | ERROR | Missing parameter type at position 1

You should write:

/**
 * Get comment count of the user. Comments made in the last x hours.
 *
 * @param int $uid
 *   The userid of the user whose comment count is to be determined
 */

---

103 | ERROR | An operator statement must be followed by a single space

Maybe the automatic tester comes with the new line in trouble. ;)

---

Also i get this warning The following git branches do not match the release branch pattern, you should remove/rename them.. Is this because my branch is names 7.0 instead of 7.x ?

Yes, your branch should be called "7.x-1.x".

saitanay,

Thank you for taking the time to contribute.

Not carefully reviewing, there is a misspelling on line #172 and on line #155 (remianing vs. remaining). As for your last comment, you can use dos2unix to get rid of those Windows style line endings.

Oh, one more thing. File an issue fıor everything you intend to do. This should also help.

Hi, and welcome to the review process! ;)
I have the following notes:

1) It's possible to use 0 for infinity, but it is not implemented thoroughly in the code. 0 is more than 10, so ($roles_comment_limit_by_role > $comment_limit_by_role) isn't doing the necessary work

2) If logged in as a user that only has roles with infinity, the following will be displayed: "You can make 0 more comments". You are however allowed to make a comment. After you do this, the following is displayed: "You can make -1 more comments".

3) The message "You can make x more comments" doesn't mention that you will be able to post new comments later. Without this information, the user may be misled to thinking he will only have x comments left ever. You could for example display: "You can make x more comments today" (when set to 24 hours)

4) In line 134 in the .module file, there is a nested loop. The outer loop however seems unnecessary. You don't use the variable $record for anything.

5) In comment_limit_by_role_reached() you set a session variable. It is bad practise with functions having side effects when it can be avoided. If you feel you must, then at least document this side effect in the function documentation

6) When the module is installed, it would be nice with a message showing that it did so. The message could link to the configure page. Here is the code I used for my module, which is also in the application queue:
/**
* Implements hook_install().
*/
function webmaster_menu_install() {
// Tell user that the module was installed and how to configure it.
$t = get_t();
$link = l($t('Administration > Configuration > Webmaster menu'), 'admin/config/webmaster_menu');
$text = $t("Webmaster menu was installed. It can be configured here: " . $link);
drupal_set_message(filter_xss_admin($text));
}
Note that you should not use get_t() here. Not t() and not st() (see http://drupal.org/node/1420358#comment-6099242)

7) It would be nice with a "Configure" link next to the module in the module list. To do this, add a "configure = [path to configuration page] to the .info file. Ie in my module, I added this line to the .info file:
configure = admin/config/webmaster_menu

8) It may be a problem that your contribution is small. A great part of the module is taken from an existing module. If you substract this, your contribution might not meet the criteria to get "create full projects" permission. But with this said, it is still possible to get the module published without getting this permission granted. See http://drupal.org/node/1648958 and http://groups.drupal.org/node/195848

9) You should note that the authenticated user role (role id: 1) isn't part of the $user->roles array for users that have other roles. As you run through this array (in line 136 in the .module file), it means that the limit set for authenticated users are only taken into account if the user has no other roles. This may be intended. It should then perhaps be made clear in the configuration page that the value punched in for authenticated user is only relevant for users that have no roles assigned

Hi rosell.dk,

I recently became co-maintainer for this project, thanks a lot for your feedback. First of all let me say that your comments are mostly right but refer to the 7.x-1.x branch while the most active developed branch (which also ) is the 6.x-1.x one. You will see that some of the issues you are raising are already addressed there. Sorry for that but initially we needed this features for a Drupal 6 installation so we are making all the work (included additional feature, see the issue tickets and commits) in the 6.x-1.x branch and will then port the code to the 7.x-1.x.

(1), (2), (4), (9) are already addressed in 6.x branch. I also think that the statement made on (8) is not valid given the additional features and the work made on 6.x branch (more is coming I hope).

I've created #1682298: Specify the time within which the user will be able to do his remaining comments for (3)
I've created #1682318: Install configuration message for (6) but don't tou think it would be better hook_enable so to remember this everytime the module is enabled? (not a big difference anyway)
I've created #1682500: Implement a configure link in the module list for (7) which just applies to 7.x

As for (5) I agree with you, my opinion is that session use can be limited or completely removed maybe using cache at its place and updating the cache just on comment submit and role change. Probably using a per user cache would be more appropriate, don't know about performance, what do you think?

Again, thanks a lot for your feedback, we will try to update the 7.x asap.

rosell.dk

I've fixed all of the issues reported by you, I've also reconsidered (4) and fixed some remaining of this in #1682902: Remove useless query / iteration on all roles in comment_limit_by_role_reached.

Added "javascript support" with #1681474: Add optional javascript support module features.

Still considering what to do with (5), it could be enough to move the $_SESSION out of the method.

All work has been done on 6.x and still has to be ported on 7.x (is there any problem having a 6.x release first?)

I´m on vacation for the next four weeks, so I hope someone else will evaluate the application meanwhile

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1637450
Project 2: http://drupal.org/node/1685484

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

This looks like a feature that could go directly into the existing comment_limit module. We prefer collaboration over competition and we want to avoid fragmentation on drupal.org to not confuse users. Please open an issue in the queue of the existing project to join forces and get in contact with the maintainer(s) to discuss this. I assume you can simply take over the existing project and develop your addition there.

http://drupal.org/project/comment_limit

If that fails for whatever reason please get back to us and set this back to "needs review".

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

added a reviewed project link