[D7] Popular Children

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

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.

Hello karoop,

Your module looks good. There are however a few things that should be double checked:

1. There are still a lot of pareview errors in popular_children.module:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxkaroop2744957git
You should check the code according to the warnings and errors displayed there.

2. In popular_children.install line 16 there is a mysql table field definition called 'date', which is a reserved word in both PHP and MySQL.
It could generate issues when using your module. You should rename this field and update any code that calls the field.

3. In popular_children.install line 39 there is a mysql table field definition called 'count', which is a reserved word in both PHP and MySQL.
It could generate issues when using your module. You should rename this field and update any code that calls the field.

4. In popular_children.install, for all tables declared, index names should begin with the name of the table they depend on.
See Drupal SQL coding conventions (https://www.drupal.org/node/2497).

5. In popular_children.module line 876 there is an SQL query that could/should be adjusted:
The $date_condition and $role_condition PHP variables should be moved out of the query body and passed in as separate parameters.
See Drupal SQL coding conventions (https://www.drupal.org/node/2497).

Thank you

Hi karoop,

I have done code review of your module, can you please do below fixes,

1) Use Drupal behaviors in your popular_children.js file.

Drupal.behaviors.myModule = function(context) {
   
}

2) Add $('[data-pc-external] a').click(linkHandler); in behaviors.

3) Invert condition if (count($menu_info) == 2) { for early return. and will reduce else section.

4) I have seen use of $_POST['form_token'] can we get this value from $form_state

Thanks,

Hello @karoop,

I have review your module and following are my findings,

File: popular_children.module

Line 95 and 100	: Doc comment short description must start with a capital letter

Line 634: Inline comments must start with a capital letter

Line 639: Do not concatenate strings to translatable strings, they should be part of the t() argument and you 	should use placeholders

Line 890: Parameter comment must end with a full stop

Line 1040: Array indentation error, expected 6 spaces but found 8
Same for Line 1041, 1043, 1044, 1045, 1046, 1047, 1048, 1049

Line 1073: Parameter comment must start with a capital letter

File: popular_children.test

Line 150:Doc comment long description must end with a full stop

Line 172: Type hint "function" missing for $fn

Line 780: Do not concatenate strings to translatable strings, they should be part of the t() argument and you should use placeholders

Line 883, 989, 1059: Parameter comment must start with a capitalletter

Thank you,

Hello @karoop,

Review which I posted here is showing by command line version of codesniffer which is slightly more up to date than pareview.sh.

Thank you,

Hi karoop,

I have reviewd your module. It is looking good for me.
Please check the below link and fix the issues :
http://pareview.sh/pareview/httpsgitdrupalorgsandboxkaroop2744957git.

Automated Review

You have addressed many of the Automated Review results in previous comments, but I have included a suggestion in the Coding Style section below.

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

No: Does not follow the guidelines for in-project documentation and/or the README Template.

Does not include all of the required sections of the README, e.g. Introduction, Requirements, etc

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. popular_children.test:172 - param type should be string
2. popular_children.test:780 - avoid concatenating the result of t(), see https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7....
3. popular_children.test:883 - Parameter comment must start with a capital letter. Perhaps prepend the word "The" at the beginning of the comment.
4. popular_children.test:989 - Parameter comment must start with a capital letter. Perhaps prepend the word "The" at the beginning of the comment.
5. popular_children.test:1059 - Parameter comment must start with a capital letter. Perhaps prepend the word "The" at the beginning of the comment.
6. popular_children.module:95 - Doc comment must start with a capital letter. Perhaps prepend the word "The" at the beginning of the comment.
7. popular_children.module:100 - Doc comment must start with a capital letter. Perhaps prepend the word "The" at the beginning of the comment.
8. popular_children.module:634 - Inline comment must start with a capital letter. Perhaps prepend the word "The" at the beginning of the comment.
9. popular_children.module:639 - Translatable strings must not begin or end with white spaces, see https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7....
10. popular_children.module:1073 - Parameter comment must start with a capital letter. Perhaps prepend the word "The" at the beginning of the comment.
11. Many examples of incorrect array indentation.

This review uses the Project Application Review Template.

Hi karoop,

I installed this module in my localhost. Please see my comments below.

For external links you are using ajax callback to increment the counter. But I found an issue in that, because anyone can increase the click count of a link in db using javascript. As you are setting access callback as 1 for ajax url, anyone can post data to that url and is getting entered in to the database. Using below script user can easily insert any random numbers in to the database.

jQuery.ajax({
  type: "POST",
  url: '/popular_children/click_logger',
  data: {mlid:999999999},
  success: function(data){console.log(data);},
  dataType: 'jSon'
});

Thanks,
ARUN AK

In case it helps find solutions to the problem that ARUN AK found: that sounds like a Cross Site Request Forgery and/or a Lack of Flood Control vulnerability.

I realize now that might also be a lack of input validation and access bypass.

Removing security tag because click tracking is hard to protect and usually open for anyone to create records. If you look at for example Drupal core's statistics.php file you will see that it also logs requests unconditionally. Even Google Analytics has this problem that it cannot distinguish fake page hits from some attacker vs. real page hits from human users.

Any other vulnerability you found ARUN AK?

Hey karoop,

I noticed a few things, I'll comment on...

- While using the define() function for constants, I feel like most Drupal developers use the variable_set() and variable_get() functions and do the initial setting in the install file/hook. I see you are using both ways in your module, but I feel switching to variable_set() and variable_get() aligns more with Drupal and allowing other modules to access the state of configuration. Also, if I'm looking through your code and trying to see what a constant is, I have to jump back and forth.

- I think you are using drupal_load() in the schema hook just to get the POPULAR_CHILDREN_BLOCK_TYPE_CHILDREN constant? If so, I've seen that setting default values is best done in hook_enable() and you can rely on the .module file being loaded in that hook. Reference

- In popular_children_add_block_form(), I think you can just call the actual form from hook_menu() rather than merging the $form into _popular_children_block_config_form(). Another IMHO, but using array_merge in your form definition can make it harder to read. You shouldn't really need to worry about overwriting your form keys in your own module.

- I tried going through the process of making a block but I could never view it. I tried stepping through the code but only got to the part in _popular_children_block_content() where there were no returned links. I guess I'm just no understanding how to use this module from poking around and using the readme. Could just be that I'm a dummy, but I guess my review will have to end here.

Overall, it looks like good quality code and what I pointed out are not huge deals, but since I couldn't fully use the module, I don't know how helpful this review will be.

Hi Karoop,

FILE NAME: popular_children.test
LINE NUMBER:3083
STATUS: WARNING
MESSAGE: Unused variable $edit_path.

Please, remove unused variable "$edit_path".

manual review:

1. popular_children_admin_form_submit(): why do you validate the form token here? The form API will already do that for you, so this can be removed?
2. popular_children_page_alter(): if you are just adding JS and not modifying $page here then you should use hook_page_build() instead.
3. popular_children_init(): hook_init() does not seem a good fit, since you only want to log clicks when an actual page is built. Also a case for hook_page_build()?
4. popular_children_boot(): why do you need to start a session for anonymous users? Probably because of Drupal.settings.popularChildren.token? I think that is bad because it breaks page caching. As I said: when counting statistics for anonymous users it is not really possible to prevent CSRF.

But otherwise looks good to me.

Thanks for your contribution, karoop!

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.