[D7] Age Calculator

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

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
Please find my review below:

age_calculator.install
You don't need to set variables on age_calculator_install() functions, Instead wherever you are using variable_get() you can pass default value as a second parameter. e.g for 'age_calculator_years_months_days' you can use variable_get('age_calculator_years_months_days', 'age_calculator_years_months_days'). @see: https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/vari...

age_calculator.module
Use sanitize functions. Use t() function for form #title property for multilingual in line#81,88....

In age_calculator_permission()
'warning' => t('Warning: Give to trusted roles only; this permission has security implications.'),
What type of security implications this permission has?

age_calculator.settings.inc
In age_calculator_output_options() all the options value(strings) should be sanitize with t().
In age_calculator_default_output_options() use default value in variable_get(), once you removed variable_set in hook_install.

age_calculator.helper_functions.inc
In age_calculator_get_results() all the strings should be sanitize with t() like 'Your age is' in "<span class=''age_calculator_helper_text>Your age is </span>".

Hi Kunal,

The git clone command that you have mentioned is asking for a password.

Hi ,
I tried to clone this on my local machine with your updated URL mentioned in comment#4
Issue:
siddharth@siddharth:~$ git clone git.drupal.org/sandbox/kunal.kursija/2452777.git age_calculator
fatal: repository 'git.drupal.org/sandbox/kunal.kursija/2452777.git' does not exist

Instructions:
You can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".

Reference : https://www.drupal.org/node/1011698.

When I select 'Age on date' < 'Date of birth' it shows error message:
"Invalid date of birth."
Error message should be change, something like: 'Age on date should not be lesser than date of birth'.

That sounds like a minor improvement but not like an application blocker, anything else that you found or should this be RTBC instead?

Hi! :) Good job :)

Fix in age_calculator.module Line 40.

$items['admin/config/people/age-calculator/settings'] = array(
40 'title' => 'Age calculator',
41 'description' => '',

Line 40 add function t()

Yes @klausi
For now I did not find any kind of blocker.

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
Looks good to me
Just one error message improvement:
Remove full stop from "Select age calculator output format. field is required." error message after 'output format.'

Hi @nguerrero

In D7 you should NOT be using t() in your menu declarations. Menu automatically pushes title and description through t().
@see: https://api.drupal.org/comment/51103#comment-51103

Yes @guptas89, @Klausi write me for the same. Sorry.

Removed review comments where you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

manual review:

1. what is the use case of this module? Why would I add that to my site? Please explain on the project page.
2. age_calculator_install(): no need to set variables upon installation since you can make use of default values with variable_get() anyway.
3. age_calculator_form(): doc block is wrong, this is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms on how to document form building functions. Same for age_calculator_output_configuration_form_submit(), please check all your form doc blocks.
4. "'#value' => 'Calculate',": all user facing text must run through t() for translation.
5. age_calculator_alter_year_range(): why do you need this after build callback? You can just set the #options in age_calculator_form()?
6. "t("Or") . " " . $formatted_output;": do not concatenate variables to translatable strings, use placeholders with t() instead. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7 . Same for "$months . " " . t("Months") . " " . $date_difference->d . " " . t("Days");", please check all your translatable strings.
7. age_calculator_output_configuration_form_submit(): You don't need this function if you use system_settings_form() in age_calculator_output_configuration_form(). See https://www.drupal.org/node/206761
8. age_calculator_block_view(): do not call render() here, just use the nested render array. Drupal core will render it later for you and other modules can alter that data more easily.

Although you should definitely fix those issues they are not critical application blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

1. Not critical application blocking, but the inline comment blocks (even the multiline) should use
   // Comment here
   instead of

   /*
     * Comment here
     */

2. (*) Instead of age_calculator_get_results(), which basically returns themed markup, use hook_theme. That way folks can modify the markup in the block fairly easily.
3. hook_hook_info_alter is a little unorthodox for a simple module like this.

The first two items should probably be fixed before a full release. The last just makes it a little more difficult for folks to contribute patches and improvements to the project. It doesn't need to be changed, but keeping the entry point low is always good.

Thanks for your contribution, Kunal!

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.

I recommend using tpl when possible. It is more clear for folks to override later. However, you can also use a function instead of a tpl.
https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
See the difference between 'function' & 'template'?

Also some blog posts that came to the top in google:
http://www.nicklewis.org/drupal-hackers-cookbook/theming/hook_theme-and-...
http://www.stephanemartinw.com/articles/25-theme-drupal-hooks-templates-...

It looks about right. Does it work?