[7] Limit Publishing Options

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxTanvirAhmad2204859git

I'm a robot and this is an automated message from Project Applications Scraper.

Please take another look at the automated output of PAreview, there are coding issues to be addressed there.

@TanvirAhmad, 2 things worth mentioning here:

1) Your git clone URL has issues (see below):

$ git clone http://git.drupal.org:sandbox/TanvirAhmad/2204859.git limit_publishing_options
Cloning into 'limit_publishing_options2'...
fatal: repository 'http://git.drupal.org:sandbox/TanvirAhmad/2204859.git/' not found

2) Issues raised in #5 and #8 still stands (see attached).

Please fix these first before requesting for a review. thanks!

Being not application release blocker(s), leaving you at "Needs review" with the hope that these are fixed!

@TanvirAhmad: did a general review of your project:

- Files do not need to be executable: limit_publishing_options.info, limit_publishing_options.install, limit_publishing_options.module are in 775.
- Remove the package entry in your info file as it is not supposed to be part of any package yet.
- Your hook implementation comments are not valid. Actually, a lot of errors are still raised by PAreview and need to be fixed.
- There are typos in the @file bloc, in limit_publishing_options.check.inc

Hi,

Pareview looks good indeed.

I've checked the module functionality and it looks fine.

No remarks here.

• It would be REALLY helpful to get some more fleshed-out descriptions of what the items in the main limit_publishing_options form do. A checkbox that's only labelled "Authenticated User" doesn't really explain what it is that I'm checking that box to do.
• Similar situation for the "Enable Limit Publishing Options" checkbox at the top of the form, how is that checkbox different from what the user might expect to happen out of the box when enabling the module.
• (Future feature request): I see this uses a db table as well as some variable settings, is it possible to export the settings for limit_publishing_options to a features module or using ctools exportables?

Hi TanvirAhmad,

I have installed and configured your module as per stated in the description. My configuration for the module is

Role Limit (authenticated user)
Limit the Promote/day. => 1
Limit the Sticky/day. => 1
Role Limit (administrator)
Limit the Promote/day. => 1
Limit the Sticky/day. => 1

When I created content of type article with "admin" log in, it is not working as per configuration. It gives me the message for sticky on 3rd node creation, whereas I created total of 4 nodes but never got the message for "promoted to front page".

When I create content by log in with a authenticated user, I have never got the messages neither for "Sticky" nor "Promoted to front page"

Hi Tanvir nice effort, here are some points
1- you are using module_load_include('inc', 'limit_publishing_options', 'limit_publishing_options.check'); module_load_include is not best practice its better to add this in info files.
2-in presave you are returning return limit_publishing_options_po_notification(); i guess return is not necessary, your ajax commands will work fine without it.
3-it is very simple select

 $result = db_select('limit_publishing_options', 'n')
      ->fields('n')
      ->condition('uid', $user->uid, '=')
      ->condition('publication_op', $publication_op, '=')
      ->execute();

you could use db_query for it.
4- removing review tag. you can add it again. if you done 3 or more reviews.

Hi @alinouman

I am not agree with your arguments which you have raised in your post

1- you are using module_load_include('inc', 'limit_publishing_options', 'limit_publishing_options.check'); module_load_include is not best practice its better to add this in info files.
asghar: First go to this page(https://drupal.org/node/542202#files) and read *file* content info carefully.

2-in presave you are returning return limit_publishing_options_po_notification(); i guess return is not necessary, your ajax commands will work fine without it.
asghar: Not necessary.

3-it is very simple select
asghar: Drupal community prefer more db_select because easy read, PDO formatting etc.

Hi Asghar,
1- I agree with your point 1. Sorry about that.
2-node_Presave is a hook and returning something on it doesnot make sense since his if statements are checking all conditions. he could call only limit_publishing_options_po_notification(); and i guess code will still works because ajax command will execute.
3-db_query is much faster than db_select check this https://drupal.org/node/1067802 .
4- he is also missing config link in info file too.

Hi Tanvir,
You are missing config link in info file.

Working fine as designed.

this file should be removed: .limit_publishing_options.module.swp

.inc file:
Typo in @file:

/**
 * @file
 * Provieds the custom functions available.
 */

tabs should be two spaces, not four.

  $result = db_select('limit_publishing_options', 'n')
      ->fields('n')
      ->condition('uid', $user->uid, '=')
      ->condition('publication_op', $publication_op, '=')
      ->execute();
....
  db_insert('limit_publishing_options')
      ->fields(array(
        'uid' => $user->uid,
        'date' => $date,
        'publication_op' => $publishing_option,
      ))
      ->execute();

.install file:

      'date' => array(
        'description' => 'The date for promotion.',
        'mysql_type' => 'timestamp',
        'not null' => FALSE,
      ),

This doesn't support other databases. Either note that on the project page and readme or fix it. date.module uses these formats:

      'type' => 'datetime',
        'mysql_type' => 'datetime',
        'pgsql_type' => 'timestamp without time zone',
        'sqlite_type' => 'varchar',
        'sqlsrv_type' => 'smalldatetime',

No need to call drupal_uninstall_schema in uninstall hook.

.module file:

/**
 * Implements hook_form_validate().
 */
/**
 * Implements hook_form_submit().
 */

These aren't valid hooks. See https://drupal.org/coding-standards/docs#functions. I typically call these things Form callback: submit handler for foo_form().. But call it what you want.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

@TanvirAhmad, thank you for your contribution!

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Few minor issues.

FILE: .../www/drupal-7-pareview/pareview_temp/limit_publishing_options.check.inc
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
10 | ERROR | [x] Whitespace found at end of line
50 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...var/www/drupal-7-pareview/pareview_temp/limit_publishing_options.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
5 | ERROR | [x] Whitespace found at end of line
6 | ERROR | [ ] Doc comment short description must be on a single line,
| | further text should be a separate paragraph
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation. As I googled not found any module that provides sticky related feature and looks similar to this.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

(+) No: Follows the guidelines for in-project documentation and the README Template. Some special characters are there » People » Permissions and also wrong spellings, make sure to go through README Template again.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) Your module is not working as intended. As I created a new user with administrative role and this module unable to restrict me to promote/publish/sticky content beyond limit(2/1).
2. hook_help() is missing in your module.
3. (+) Limit Publishing Options admin settings form fields can hold malicious data as there is no validations found as it should be.
4. (+) Also I found you have defined limit_publishing_options schema but no insertion in this table, even you also truncating this table in limit_publishing_options_node_validate function db_truncate('limit_publishing_options')->execute();. Please correct me if I am wrong.
5. (+) In following validation function, I am unable to find limit_po_count field, what field value you are validating?
   

   function limit_publishing_options_form_validate($form, &$form_state) {
     if (!empty($form_state['values']['limit_po_count'])) {
       if (!($form_state['values']['limit_po_count'] > 0)) {
         form_set_error('limit_po_count', t('Limit must be a positive number.'));
       }
     }
   }
   

6. (+) limit_publishing_options_node_presave($node): Simply use if-else condition instead of using two if statement.
   It should be something like this.

   if ($node->is_new) {
     // Second if condition code come here.
   } else {
    // First if condition code come here. 
   }
   

   Also I have seen you have number of if-else statement within a function, better to give proper comment and also try to break big functions in small functions (if possible). Currently it is really difficult to debug your code, wherever possible replace multiple if-else statement with switch case (if possible).

7. What is the use of limit_publishing_options_roles_flush_date variable, please reply on this
8. Instead of using time(), should use REQUEST_TIME.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

I appreciate your module idea and your hard work.

Thanks Again!

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Removing review bonus tag, you have not done all manual review, you just pointed to a git clone command that anyone can correct in an issue summary. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

1. what are the differences to existing projects like https://www.drupal.org/project/publishcontent and https://www.drupal.org/project/custom_pub ? Please add that to the project page. This sounds like a feature that should live in the existing projects. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in one of the issue queues to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process. This is recommended to not create duplicate modules on drupal.org.
2. limit_publishing_options_schema(): why do you only define the mysql type here for some fields? So your module will not work on other databases such as Postgres? See https://www.drupal.org/developing/api/schema for how to define DB independent fields.
3. limit_publishing_options_schema(): we usually use integer DB columns to store date timestamps in drupal.
4. limit_publishing_options_form(): doc block is wrong, this is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms
5. limit_publishing_options_form_validate(): if you just want to check that the values are positive integers then you can use element_validate_integer_positive as #element_validate function callback. Then you don't need your validation function at all. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h... and https://api.drupal.org/api/drupal/includes!form.inc/function/element_val...
6. limit_publishing_options_node_validate(): why is this needed? Why do you empty the whole DB table when one node is validated? Please add a comment.
7. "drupal_set_message($limit_notification_message . '(For Promotion)', 'warning');": all user facing text must run through t() for translation with appropriate placeholders.
8. This module has a security vulnerability. As part of our git admin training I'm assigning this to Manjit.Singh so that he can take a look. If he does not find the security problem I'm going to post details about the vulnerability in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

@Klausi Thanks for your assistance

@TanvirAhmad What you have done in submit function, you can do that directly via system setting form, No need to write submit function. Can you please change your admin setting form and follow https://www.drupal.org/node/206761 for reference

OK, looks like Manjit.Singh did not find the security issue, so here are the details:

limit_publishing_options_page_build(): this is vulnerable to XSS exploits. drupal_set_message() expects the message parameter to be sanitized. If I enter <script>alert('XSS');</script> for the limit_publishing_options_notification variable in the admin settings I will get a nasty javascript popup. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again. In you case using t() with the appropriate "@" placeholder will do the sanitization for you.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.