[D7] User Auto Tag

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

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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 hiryu,

here are some things i found:

code

userautotag.module:

ok... last thing... what's this?

./userautotag.module: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.

Looks like your editor manually puts a bom to your file. Maybe you should take an other editor and create a new file.
- http://en.wikipedia.org/wiki/Byte_order_mark

There are some strings which are not translatable like messages to the user.

userautotag.css:

- remove line 11
-

}

in this line is unnecessary

in drupal

i get some warnings from your module:

Notice: Undefined variable: fields in userautotag_get_fields_list() (line 35 in ../sites/all/modules/user_auto_tag/userautotag.functions.php).

after saving in admin/config/workflow/userautotag:

Warning: Invalid argument supplied for foreach() in form_select_options() (line 2593 in ../includes/form.inc).

But your module works like written in the description.

Hi hiryu,

i don't get an notice anymore that's ok, but i still get this warning after saving. i will install your module in a clean drupal installation. hope i will get no warning after that. :)

As i can see there are still some strings which are not translatable. Please check that.

• Line 32
• Line 68
• Line 78
• Line 128
• Line 132
• Line 147
• Line 154

You are currently working in your master branch. Please create a "7.x-1.x" branch and push it to git. After that you can delete your master branch.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1978434

Project 2: http://drupal.org/node/1985214

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1978434

Project 2: http://drupal.org/node/1985214

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

Can you tell me how to create a "7.x-1.x" branch and push my code into it?

to push:
git push <remote-name> <branch-name>
to remove:
git push origin --delete <branch-name>
for more information see gitref.org

Manual review:
1) in Drupal is more common to use .inc files instead of .php files.

2) don't use $_POST global array in form submit. Use $form_state['values'] instead.
Here is part of your code:

if (!empty($_POST['userautotag_adm_exclude'])) {
    variable_set('userautotag_adm_exclude', $_POST['userautotag_adm_exclude']);
    $msg = t('Tagging for Administrators: Disabled');
  }

3)

Can you tell me how to create a "7.x-1.x" branch and push my code into it?

Also read this article http://drupal.org/node/1127732

read this article

Use Review bonus program

Have you already read this article?

Hi, manually reviewed:

• Please append link to git repository in current issue
• You still use $_POST instead $form_state_values in submit handler(lines 135-146)
• Error on line 45 and 50 in user_auto_tag.functions: if ($vocaboulary->machine_name == $form_stateDEFAULT_VOCABULARY) {
• unused variable $key on line 31 and $nestedkey on line 125 in user_auto_tag.functions

Added link

This project is a great intuition. I'm going to use it now... Thank you.

Please add some other description to readme file...

Ok, thank you!

Just some minor comments.

I don't think you should be using !important in css for contrib code.

// field_delete_field('field_user_auto_tags');

There's no reason to keep debug code lying around.

* Implements of hook_install().

"Implements hook_install()." not "Implements of hook_install().". Similar issue for most of the other documented hooks.

Avoid using module_load_include() in the global context, see the docs at https://api.drupal.org/api/drupal/includes%21module.inc/function/module_...

$user->field_user_auto_tags[LANGUAGE_NONE][]['tid'] = $applytag;

There are a few places where you're using LANGUAGE_NONE instead of the field API. This would make your module incompatible with internationalization/translation.

I'm wondering if it might be more efficient to convert your various, dynamic variable_set() calls (that use $type) that could end up representing many individual variables into a single array variable, unless there's a strong use-case for keeping them separate.

$path = realpath(dirname(__FILE__));

Can we not use drupal_get_path() here?

  // Adding CSS and JS just in the admin section.
  if (arg(0) == 'admin') {
    drupal_add_css(drupal_get_path('module', 'user_auto_tag') . '/user_auto_tag.css', array('group' => CSS_DEFAULT, 'every_page' => TRUE));
  }

Why are we not using path_is_admin() https://api.drupal.org/api/drupal/includes%21path.inc/function/path_is_a... ?

Why are we adding css to every admin path?

Hi,

It looks like you're calling an undefined function in user_auto_tag_install() (line 22 in user_auto_tag.install)
-> function user_auto_tag_check_vocaboulary()

If you don't mind about translation you may ignore the error below.

FILE: /var/www/drupal-7-pareview/pareview_temp/user_auto_tag.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 184 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

<hr> tags should technically be <hr/>.

I'm not sure why you're using check_plain() instead of t()? check_plain() just sanitizes the string - t() allows it to be translated. The comment in #32 just meant that when possible you should use t() like t('String you can translate"). When you do t($variable_to_translate) it doesn't allow a code parser to pre-determine all the translatable strings in your code. Not a major issue, and changing all the check_plains to t() is not the answer.

typo "variablke" should be "variable".

It would be nice to include a way to configure this module on a per-role basis. Currently it will either affect all users or all non-admins.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Definitely use something, I think using t() is better, but I guess check_plain() is ok too.

Thanks for your contribution, hiryu!

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.