[D7] Common Body Class

Please review and fix the errors/warnings found at:
http://pareview.sh/pareview/httpgitdrupalorgsandboxsanjaysingh2771679git.

Hi @Sanjay Chauhan,

Please change git URL in this post. so that community members can clone and review your project.

It should be -

git clone --branch 7.x-1.x https://git.drupal.org/sandbox/sanjaysingh/2771679.git common_body_class

Thanks

Hi @Sanjay Chauhan,

As a part of module review on drupal.org you need to do some manual reviews of projects posted by community members at https://www.drupal.org/project/issues/projectapplications

Once you reviewed some projects, you need to add URLs of reviews in this post as part of review bonus system.

For more information please check How it works and Summary section at below URL -

https://www.drupal.org/node/1975228

Thanks

Hi @Sanjay Chauhan,

For this post/issue you have selected your own project which is wrong.

Below is the URL where we can view all the projects which are posted for the review. Updating the project for this post to Drupal.org Project applications so that all the reviewers can see your project and review it.

URL - https://www.drupal.org/project/issues/projectapplications

Thanks

Fixed the git clone URL in the issue summary for non-maintainer users.

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

Hello @Sanjay Chauhan,

Please check following warnings:

$ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,js,css,info,txt,md
 common_body_class/

FILE: ...\sites\all\modules\common_body_class\common_body_class.admin.inc
----------------------------------------------------------------------
FOUND 18 ERRORS AND 6 WARNINGS AFFECTING 19 LINES
----------------------------------------------------------------------
   1 | ERROR   | [x] End of line character is invalid; expected "\n" but found "\r\n"
  30 | WARNING | [x] A comma should follow the last multiline array item. Found: )
  41 | WARNING | [x] A comma should follow the last multiline array item. Found: )
  52 | ERROR   | [x] Separate the @see and @ingroup sections by a blank line.
 117 | ERROR   | [x] Whitespace found at end of line
 118 | WARNING | [ ] Line exceeds 80 characters; contains 81 characters
 119 | ERROR   | [x] There must be exactly one blank line before the tags in a doc comment
 119 | ERROR   | [ ] Missing parameter type
 124 | ERROR   | [x] Separate the @see and @ingroup sections by a blank line.
 138 | WARNING | [x] A comma should follow the last multiline array item. Found: TRUE
 145 | WARNING | [x] A comma should follow the last multiline array item. Found: TRUE
 172 | ERROR   | [ ] If the line declaring an array spans longer than 80 characters, each element should be broken into its own line
 220 | ERROR   | [x] Array indentation error, expected 4 spaces but found 8
 221 | ERROR   | [x] Array indentation error, expected 4 spaces but found 8
 222 | ERROR   | [x] Array closing indentation error, expected 2 spaces but found 6
 238 | ERROR   | [x] No space found before comment text; expected "// Update class data" but found "//Update class data"
 238 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, colons, question marks, or closing parentheses
 247 | ERROR   | [x] No space found before comment text; expected "// First delete all entry of role based on cbcid" but found "//First delete all entry of role based on cbcid"
 247 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, colons, question marks, or closing parentheses
 251 | ERROR   | [x] No space found before comment text; expected "// Insert new roles" but found "//Insert new roles"
 251 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, colons, question marks, or closing parentheses
 273 | ERROR   | [x] There must be exactly one blank line before the tags in a doc comment
 273 | ERROR   | [ ] Missing parameter type
 301 | WARNING | [ ] Empty return statement not required here
----------------------------------------------------------------------
PHPCBF CAN FIX THE 19 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...upal7\sites\all\modules\common_body_class\common_body_class.info
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...l7\sites\all\modules\common_body_class\common_body_class.install
----------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 7 LINES
----------------------------------------------------------------------
  1 | ERROR | [x] End of line character is invalid; expected "\n" but
    |       |     found "\r\n"
 74 | ERROR | [ ] Empty installation hooks are not necessary
 75 | ERROR | [x] Whitespace found at end of line
 76 | ERROR | [x] Expected 1 blank line after function; 0 found
 80 | ERROR | [ ] Empty installation hooks are not necessary
 81 | ERROR | [x] Whitespace found at end of line
 82 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...drupal7\sites\all\modules\common_body_class\common_body_class.js
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  1 | ERROR | [x] Missing file doc comment
  1 | ERROR | [x] End of line character is invalid; expected "\n" but
    |       |     found "\r\n"
 39 | ERROR | [x] Expected 1 newline at end of file; 3 found
 41 | ERROR | [x] Additional whitespace found at end of file
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...al7\sites\all\modules\common_body_class\common_body_class.module
----------------------------------------------------------------------
FOUND 12 ERRORS AND 4 WARNINGS AFFECTING 14 LINES
----------------------------------------------------------------------
   1 | ERROR   | [x] End of line character is invalid; expected "\n" but found "\r\n"
   6 | ERROR   | [x] There must be exactly one blank line after the file comment
  10 | WARNING | [ ] All constants defined by a module must be prefixed with the module's name, expected "COMMON_BODY_CLASS_CBC_VISIBILITY_NOTLISTED" but found "CBC_VISIBILITY_NOTLISTED"
  15 | WARNING | [ ] All constants defined by a module must be prefixed with the module's name, expected "COMMON_BODY_CLASS_CBC_VISIBILITY_LISTED" but found "CBC_VISIBILITY_LISTED"
  20 | WARNING | [ ] All constants defined by a module must be prefixed with the module's name, expected "COMMON_BODY_CLASS_CBC_VISIBILITY_PHP" but found "CBC_VISIBILITY_PHP"
  89 | ERROR   | [x] There must be exactly one blank line before the tags in a doc comment
  89 | ERROR   | [ ] Return type missing for @return tag in function comment
 103 | WARNING | [ ] Line exceeds 80 characters; contains 88 characters
 104 | ERROR   | [x] Whitespace found at end of line
 105 | ERROR   | [ ] Missing parameter type
 109 | ERROR   | [ ] Return type missing for @return tag in function comment
 131 | ERROR   | [x] Whitespace found at end of line
 132 | ERROR   | [x] There must be exactly one blank line before the tags in a doc comment
 132 | ERROR   | [ ] Missing parameter type
 135 | ERROR   | [ ] Return type missing for @return tag in function comment
 153 | ERROR   | [ ] Doc comment short description must be on a single line, further text should be a separate paragraph
----------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...pp\htdocs\drupal7\sites\all\modules\common_body_class\README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 9 | WARNING | Line exceeds 80 characters; contains 124 characters
----------------------------------------------------------------------

Time: 385ms; Memory: 6Mb

Hello @Sanjay Chauhan,

I used the command line version of Coder which is slightly more up to date than pareview.sh.

Thank you,

Hi Sanjay Chauhan,

I manually reviewed your module and please see my comments below.

1. common_body_class_add_class_form_submit(): In submit handler you are checking form has any error using form_get_errors(). You can validate and set errors in common_body_class_add_class_form_validate() function. According to drupal form api, submit handlers only execute when validation did not fail. Also you are setting success message after the if conduction, so it is not relevant the use of form_get_errors() function in this area.
2. common_body_class_admin_class_configure_submit(): You are calling cache_clear_all() function after every create, update and delete class configuration.

   But it is not a good practice implicitly clear cache after each time when admin made a change in admin configuration. It will have impact on high traffic sites. Instead of that, you can show a warning/markup message to clear cache to affect class changes in front-end.

   Refer:
   https://www.lullabot.com/articles/a-beginners-guide-to-caching-data-in-d...
   http://www.pixelite.co.nz/article/debugging-drupal-performance-with-cach...

3. common_body_class_admin_class_configure(): found
   

     $form['cbcid'] = array(
       '#type' => 'value',
       '#value' => $cbc->cbcid,
     );
     

   I think you are trying to use field 'hidden'. If not using this, remove it.
   Refer:
   https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

Thanks,
ARUN AK

Automated Review

Please fix the some basic issues suggested by paraview:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxsanjaysingh2771679git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Does not cause module duplication and/or fragmentation.

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements.

Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
1- (*) hook_uninstall is not necessary. A table created by hook_schema is automatically dropped when the module is disabled.

Thanks.

Hi @Sanjay Chauhan

Please fix errors and warnings mentioned in pareview.
http://pareview.sh/pareview/httpsgitdrupalorgsandboxsanjaysingh2771679git

Thanks.

Hello,

I'd made a little code review.

• $output = theme('table', array(
      'header' => $header, 
        'rows' => $rows,
          "attributes" => array(),
          "sticky" => TRUE,
          "caption" => "",
          "colgroups" => array(),
          "empty" => t("Table has no custom common class!"),
          'attributes' => array('class' => array('common-body-class-list'))
        )) . theme('pager');
  

  1) Dublicate attributes key
  2) Seems there is some problems with a line indents.
  3) I don't see the reasons to add colgroups and caption elements - it will be defined automatically with the same values




• Function common_body_class_add_class_form_submit
  line

    if (!form_get_errors()) {
  

  It will not reach submit function if there will be a form errors. This checking is not necessary.
  The same thing is for common_body_class_admin_class_configure_submit




• Why do you always clear all the cache? As I have understand you need to clear only few bins (probably you need only
  

  cache_clear_all(NULL, 'page');
  




• Please use current_path() instead of the $_GET['q']




• [CODE STYLE] Please separate return from the other code with the blank line




• function common_body_class_list_alter is called like "alter" function but it isn't it. Consider adding a drupal_alter() instead of it to extend module capabilities or rename this function. Probably some other modules could call list_alter hook and it will crash a process of this alter.

Thanks for attention.

Nice module And very helpfull

Hello i have checked
when more than one class will add on the same table like display in the Screenshot then only the last class will add on the body tag.

@sanjay Chauhan

Yes you are right
it is my bad reviewing that.

Multiple Applications

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

Project 1: https://www.drupal.org/node/2771959

Project 2: https://www.drupal.org/node/2829650

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.

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

Hi Sanjay Chauhan,

Your account already granted as '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.

So You can directly promote your sandbox project.