[D7] Account Settings Email Attachment

You should change the following in your issue description:
git clone --branch 7.x-1.x riju.srk@git.drupal.org:sandbox/riju.srk/2401475.git
to
git clone --branch 7.x-1.x http://git.drupal.org:sandbox/riju.srk/2401475.git account_settings_email_attachment

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxrijusrk2401475git you have some coding standars errors in your project and you should fix them. Please, fix them before put this issue at needs review state.

Manual Review

Individual user account

Yes: Follows

No duplication

Yes: Does not cause

Master Branch

Yes: Follows

Licensing

Yes: Follows

3rd party assets/code

Yes: Follows

README.txt/README.md

No: Does not follow the guidelines for in-project documentation You should improve your project page, you can do it following Project page template. Also you should implement hook_help in your module. Please, review all minimun requirements of Module requirement guidelines

Code long/complex enough for review

Yes: Follows

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) Major finding, needs work : Please, document the hooks your implements using
   

    /**
     * Implements hook_HOOK().
     */
   

   for example, change

   /**
    * Menu hook for module configuration.
    */
   function account_settings_email_attachment_menu() {
   

   to

   /**
    * Implements hook_menu().
    */
   function account_settings_email_attachment_menu() {
   

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.

This review uses the Project Application Review Template.

Regards!

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.

@arijits.drush

Thank you for contributing,

I have review this module and everything seem to be working fine. I did not see any blockers issue. Also, I have run coder review, Pareview.sh and I did not see any complain.

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

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.

manual review:

1. "variable_set('allowed_extension', 'gif png jpg jpeg pdf doc xls');": all variables defined by your module need to be prefixed with your module's name to avoid name clashes with others. Same for "variable_set('attachment_id', $attachment_value);".
2. account_settings_email_attachment_enable(): No need to set variables when enabling the module since you can make use of default values with variable_get() anyway.
3. account_settings_email_attachment_enable(): so when I disable and then re-enable the module my variable setting gets overridden?
4. account_settings_email_attachment_form_alter(): if you are only targeting one form you should use hook_form_FORM_ID_alter() instead, see https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
5. "t('Click "Browse..." to select a file to upload. Allowed extension :') . str_replace(' ', ',', variable_get('allowed_extension')),": do not concatenate variables into translatable strings, use placeholders with t() instead. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7
6. "'access arguments' => array('administer access control'),": that permission does not exist in Drupal core and it is also not defined by your module in hook_permission()? So the settings page will currently only work for user 1.

The variable naming is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

1. account_settings_email_attachment_enable(): I think this hook can be removed since you can make use of default values with variable_get() anyway, so no need to set any variables when the module is enabled.
2. account_settings_email_attachment_permission(): permissions are usually just all lower case words with spaces, no need to use underscores.

But otherwise looks RTBC to me.

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

2. account_settings_email_attachment_enable(): No need to set variables when enabling the module since you can make use of default values with variable_get() anyway.:I did not get this point, my motive is to set a variable at time of module installation and with default value.In allowed extension I need that value to set default file extension.

If you use

<?php
$allowed_extensions = variable_get('account_settings_email_attachment_allowed_extension', 'gif png jpg jpeg pdf doc xls');
?>

in your code the variable will contain the default value ('gif png jpg jpeg pdf doc xls') if the user hasn't configured anything different, yet.
See variable_get() for more information about this.

I'll have a deeper look into the code ...

Quick scan of code (in-depth review will follow):

* Use variable_get() with defaults and you can completely remove account_settings_email_attachment.install.
* Code style could be improved ;) (missing blank lines between functions, indentation, ...).
* account_settings_email_attachment_help(): You don't need to use all those if-statements here. Look at the Features module for a clean implementation.
* account_settings_email_attachment_form_user_admin_settings_alter() (and other): The variable $account_settings_email_attachment_attach_id is empty on first visit, so $account_settings_email_attachment_attach_id[$counter] does not exist (in "#default_value"). Use default values in variable_get().
* account_settings_email_attachment_form_user_admin_settings_alter() (and other): The variable $counter is not named very well. Please use a name matching the meaning of the variable.
* account_settings_email_attachment_form_user_admin_settings_alter(): make the upload path for the attachments configurable. Personally I would never store this in the public-files directory.
* account_settings_email_attachment_mail_alter(): Sending emails without saving the email settings first will cause an error because $account_settings_email_attachment_attach_id[$counter] does not exist.
* account_settings_email_attachment_set_attachment_config(): Why do you replace spaces with comma here? This is very uncommon. Additionally you need to replace it back in account_settings_email_attachment_set_attachment_config_submit() (which could be omitted completely).

account_settings_email_attachment_help(): do not use filter_xss_admin() and check_plain() here since the README content is trusted, not user provided input.

Back to stBorchert to conclude his review.

When calling account_settings_email_attachment_mail_attachment() method, there might be a chance that user has not uploaded any file. I this case, variable

$account_settings_email_attachment_attach_id[$mail_element]

contains value 0 and

$file = file_load($fid);
$filepath = drupal_realpath($file->uri);

these two lines will cause the problem.

Also I don't understand why system_settings_form() is not used for account_settings_email_attachment_set_attachment_config() and so there will be no need to have submit handler.

system_settings_form() can handle the variable_set() calls for you. Check out https://api.drupal.org/api/drupal/modules!system!system.module/function/... and examples that use it such as https://api.drupal.org/api/drupal/modules!book!book.admin.inc/function/b...

So there were no major objections here and this was RTBC already ...

Thanks for your contribution, arijits.drush!

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.