[D7] Get Total

Manual Review

Major coding standards / best practice issues

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at http://pareview.sh/pareview/httpgitdrupalorgsandboxpranavpathak2121169git.

Project Page

Please take a moment to make your project page follow tips for a great project page. There is currently no text in it and it does not give any indication as to what the module does, how to use it, or how to configure the module.

License

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

Code too short

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you. You can also implement hook_help() as well as adding some inline comments to help bring you over this threshold

Additional Notes

• Please remove the version from the .info file. It will automatically added by Drupal's packaging system
• I would suggest rewriting the "get_total_number()" function into a theme function so that its output may be easily overridden by other users. You can find more information about using Drupals theming system here.
• You have areas of your user displayed output that are not wrapped in the t() function. These lines are: 27, 30, 33, 49, 56.
• I'd suggest renaming all your non-hook functions to "_get_total_NAME_OF_FUNCTION" to prevent potential naming collisions with other modules
• Line 47 and 54 have typos. It should be "Drop Down" not "Droup Down"
• On the admin page, I'd suggest rewording the form labels so that they are more clear what the user is selecting. I'd also suggest adding a line of text explaining what the user is expected to do. For example, "Use the form below to select the content type and the published status to see the node count."
• You can replace your "get_all_content_type" function with the built in drupal function node_type_get_types()

Conclusion

Overall, I can see the benefit of this module. Thank you for your submission! There does not appear to be another module that achieves the same sort of functionality. One thing you may want to consider is to override the "Find Content" form and show the total there as well. You may also want to change the admin content type drop down to checkboxes so that users can see totals for multiple content types.

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.

Firstly have a look here for code review in regards to Coding Standards:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpranavpathak2121169git

Use of "drupal_get_total_form" seems unnecessary, just use "drupal_get_form" as page callback and page arguments.

A lot of stuff is misspelled which makes it harder to read the code.

Tried to find similar module seems like one exists for version 6:
https://drupal.org/project/count

Maybe its better to try and get in touch and create a 7 branch?

Hi Pranav,

Here is the review :-

1. Its for Drupal 7 and I think there is no hook_perm() in Drupal 7. Please change it to hook_permission().
2. Instead of defining your own method get_total_get_all_content_type(), you should use drupal provided method node_type_get_names() to get all content types.
3. You have defined your hook_menu form in the module file. You should define it in separate file by using file attribute of hook_menu().

(please use drupal apis / methods instead of defining yours if available.)

Also change your git url to :-
git clone --branch 7.x-1.0 http://git.drupal.org/sandbox/pranavpathak/2121169.git get_total

thanks.

As for https://drupal.org/node/1011698 in the 'Issue summary' please add:

• A detailed description of what your project does. You can copy/paste what you wrote in comment #3.
• I added a link to your project page. https://drupal.org/sandbox/pranavpathak/2121169 that is.
• I added a direct link to your git repository (git clone ...). git clone --branch master http://git.drupal.org/sandbox/pranavpathak/2121169.git

For application priority stick to https://drupal.org/node/894256#app-priority.

Thanks.

As for https://drupal.org/node/1011698 in the 'Issue summary' please add:

• A detailed description of what your project does. You can copy/paste what you wrote in comment #3.
• I added a link to your project page. https://drupal.org/sandbox/pranavpathak/2121169 that is.
• I added a direct link to your git repository (git clone ...). git clone --branch master http://git.drupal.org/sandbox/pranavpathak/2121169.git

For application priority stick to https://drupal.org/node/894256#app-priority.

Thanks.

Hi Pranav,

First - you haven't updated your git url.
Second - It is not necessary to use file attribute for hook_menu but should be used as module file is parsed on every page request and if we define methods in module file, then they will also parse on each request which cause performance blow.
So for performance, it is required to add file attribute in hook menu.

thanks

Hi pranavpathak, here is suggestions after manual review:

1) hook_menu doc
Implements hook_menu().
2) hook_permission doc
Implements hook_permission().
3) Please, don't document $form and $form_state in forms constructors. Please look here https://drupal.org/node/1354#forms.
4) Inside get_total_form you call $_GET['type'] directly, it is vulnerable for xss, you should use
check_plain($_GET['type') instead.
5) Please use 7.x-1.x branch now. Only after your module will become Rewieved and tested by community RTBC you will be able to create version branch.

That's all thank you!

Updated to 'Needs work' per #22

Hi pranavpathak. Please look here about moving to another git branch https://drupal.org/empty-git-master.

Hi pranavpathak

Right now your default branch is master but it should something like this *7.x-1.x *. Please change your project settings by following

1. Edit your project page.
2. Click on "Default Branch" tab.
3. Select your branch *7.x-1.x * rather than master.

Hi Pranav,

You still haven't updated you git url as it is asking for password. Please update it.

In your module file, comment is saying hook_perm() instead or hook_permission(). Correct this.

Method get_total_form_node_admin_content_alter() in your module file is implementation of hook_form_FORM_ID_alter(), so please mention it in the comment so that anyone who review this can get idea what this is.

In method _get_total_get_content() in your get_total.admin.inc file, try to use the static caching as it will help in performance.

Hi,

You still haven't updated your git url as it is asking for password :-
In description, you have given this :-

git clone --branch 7.x-1.0 pranavpathak@git.drupal.org:sandbox/pranavpathak/2121169.git get_total

If someone tries to clone your code from branch 7, it will ask for password.
Just try to clone with above url and you can see.
Now replace this with the url as I have mentioned in the earlier comment.

Is it necessary to use static caching?
Not really but if you do, it will help for performance and it is a good idea for code level caching.

thanks

#37

You used the incorrect command that's why system is asking the password.
In above command you used the "pranavpathak@git.drupal.org" as user name.

Use following command to clone the git repository
git clone --branch 7.x-1.0 http://git.drupal.org/sandbox/pranavpathak/2121169.git get_total

Above command works perfectly for me.

Regarding static cache
Can you please explain your thoughts about what is the use of static cache in this module.

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

• Also the branch 7.x-1.0 should be renamed 7.x-1.x
• You can make a 1.0 release once the project is a full module.
• You have some non-utf8 characters and a leading \feff in README.txt.
• In get_total_form_node_admin_content_alter() you are loading possibly a lot of nodes into memory at once. This could easily cause problems on large sites. Can you use a more direct method to count the nodes instead, perhaps a db_query?
• Don't concatenate t() strings, instead use variable substitution. For ex t('Total number of node is: @total', array('@total' => count($nodes));

----
Top Shelf Modules - Crafted, Curated, Contributed.

#39

I haven't used the wrong command. Wrong command was in the issue description. If you see the #10, you will see that I mentioned the correct git url.

Configure link in module listing page is not correct. Correct the path in .info file.

Please Implement hook_help and some line exceeds 80 character

That are surely not application blockers, any other major issues your found during your review?

Hey pranavpathak,

The admin config form and block functionality this module is providing can be easily obtained using views which is more simple and manageable. So i will highly vote for using views instead of using this module.

Hi pranavpathak,

#1, #3 Views solution is a more generic one and can often be altered though the Views UI and you are sure that the config is exportable with features and so on. If it works with Views then a lot of other modules also can hook in and change functionality if you need to change the out of the box func.

Manual Review:

The module is simple and in my opinion is really interesting.

My recommendations, because the module is so simple and probably people are not sure on what to expect:

1. use hook_install() to send user to the admin/config/content/get-total after module installation
2. and on the admin page put a note and say what exactly the module will do, eg.
"1. This module will provide a form at admin/config/get_total page.
2. This module also provides nodes count at /admin/content page."

• You should remove the branches you are not using from the repo if possible.
• There are still some windows characters in the README, the » on the last line.
• You don't need to use node_load_multiple() in get_total_form_node_admin_content_alter just to count the number of nodes. Use a db_select statement to just get a simple list of nids and then count() them. This way you don't have to load any extra nodes.
• PAReview found a couple minor errors at http://pareview.sh/pareview/httpgitdrupalorgsandboxpranavpathak2121169git.

None of those are blocking issues though.

Thanks for your contribution, pranavpathak!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Delete the master branch https://drupal.org/empty-git-master

I thought the guide was pretty self explanatory. But since you create your Drupal related branches the master branch is redundant.

Hi pranavpathak,

I am unable to get clone using.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/pranavpathak/2121169.git get_total
It is returning the following error.
fatal: http://git.drupal.org/sandbox/pranavpathak/2121169.git/info/refs not found: did you run git update-server-info on the server?.

That is your personal maintenance git clone command.