[D7] Custom Cookie Compliance

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

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.

Hi,
I have found the following issues from your module.
1. Write properly the git details in project applications issues

 Project git : git clone --branch 7.x-1.x    http://git.drupal.org/sandbox/pathirakaliappan/2301783.git page_guide   
          cd page_guide

2. Use proper package name in the .info file Instead of Package = Custom
3. If you adding the css and js from .info file, it will load on all pages, best to add necessary pages from .module file by using drupal_add_js()

Other things are ok for me.

@leopathu,
The above ones are not blocker.Is there anything else that you found that stopped you to set this to RTBC
@jzasnake,
Regarding point #3 You are adding js/css files in .info file.Is is needed at all pages.Please add a comment.If not use hook_page_alter or its friends to render it on specific pages.Use #attached to add the JS/CSS file
with page rendered array where these actually required.

I have a question concerning the second item, what do you mean by using a proper package name? Are you saying that I should change the project name completely? And if so, why?

No Packagename is used distinguish the modules on the modules listing page on the basis of their categories, If you will not define the package name then module will display in default category 'Other'

As all reviewers 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 :-)

Manual review :-

As per the http://php.net/manual/en/reserved.variables.server.php,

The address of the page (if any) which referred the user agent to the current page. This is set by the user agent. Not all user agents will set this, and some provide the ability to modify HTTP_REFERER as a feature. In short, it cannot really be trusted.

!empty($_SERVER['HTTP_REFERER']) && strpos($_SERVER['HTTP_REFERER']

may throw notice, if HTTP_REFERER is not available. So instead of checking for empty, use isset().

Apart from this I am not seeing any issue.

Also one more thing - Why haven't used behaviors in JS ?

Thanks and Regards
Rohit Joshi

empty() does not throw PHP notices: "No warning is generated if the variable does not exist. That means empty() is essentially the concise equivalent to !isset($var) || $var == false." from http://php.net/manual/en/function.empty.php

@klausi - What my point here was that if $_SERVER['HTTP_REFERER'] is not set, then it will throw notice.

My Apology for this.

Module authors can't directly set it RTBC.See the workflow https://www.drupal.org/node/532400

I've tested the module and it works pretty fine for me.

Assigning to myself for next review, which will hopefully be tonight.

Automated Review

Review of the 7.x-1.x branch (commit cefbcbf):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: ...custom_cookie_compliance/pareview_temp/js/custom_cookie_compliance.js
  ---------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   9 | ERROR | [x] Expected newline after closing brace
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  
  Time: 225ms; Memory: 6.5Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Minor; non-blocker.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

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

(*) I think this technically has a CSRF in it. You rely on a GET parameter which sets a cookie in custom_cookie_compliance_preprocess_html().
Any site can spoof this link. Use the Form API instead.

Coding style & Drupal API usage

(+) Your JS should be a Drupal behavior and not use the jQuery .ready() method.

You can move your config form to a separate include. This can help reduce memory footprint.

custom_cookie_compliance_preprocess_html(): use REQUEST_TIME instead of time;

The cookie expiration should be a configuration variable.

(+) You need a hook_uninstall() to get rid of the variables you are setting in _custom_cookie_compliance_settings_form(). Also keep in mind what cleanup may be needed as languages get added and deleted.

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.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Review of the 7.x-1.x branch (commit 1f344ae):

• ESLint has found some issues with your code (please check the JavaScript coding standards).

js/custom_cookie_compliance.js: line 7, col 10, Error - Missing "use strict" statement. (strict)
js/custom_cookie_compliance.js: line 7, col 18, Error - Missing space before function parentheses. (space-before-function-paren)
js/custom_cookie_compliance.js: line 8, col 78, Error - Expected '!==' and instead saw '!='. (eqeqeq)
js/custom_cookie_compliance.js: line 15, col 57, Error - Missing space before function parentheses. (space-before-function-paren)
js/custom_cookie_compliance.js: line 15, col 60, Error - Irregular whitespace not allowed (no-irregular-whitespace)

5 problems

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. project page: please add the differences to existing projects such as https://www.drupal.org/project/cookiecontrol and https://www.drupal.org/project/eu_cookie_compliance to the project page so that user can make an educated decision which module to use.
2. custom_cookie_compliance_page_alter(): since you are not altering existing stuff on the page you should use hook_page_build() instead.
3. custom_cookie_compliance_dismiss_cookie_message(): doc block should improved that this is a page callback, see https://www.drupal.org/coding-standards/docs#functions
4. _custom_cookie_compliance_settings_form(): "Custom form function." is not a useful comment. What is this form used for? See https://www.drupal.org/coding-standards/docs#forms
5. _custom_cookie_compliance_settings_form(): why do you need the hidden form element? Please add a comment.

But otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

No commit after 'commit 1f344ae', so same Automated Review.

I read through `git diff 1f344ae..HEAD` and also did a manual scan, and didn't see anything major. In addition to Klausi's recommendations, adding few more recommendations for you.

You should also really have a hook_help() with some basic info about the module.

You should move your setting form to a separate admin.inc/.inc file that is useful to keep all administration stuff like menu callbacks and forms etc. It's also useful for performance as well because all the functions from .module files get loaded at every initialization.

Otherwise it looks ready to go, so...

Thanks for your contribution, Jake Stanich!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.