Taxonomy Set Lineage

Some conding standards missing here
http://ventral.org/pareview/httpgitdrupalorgsandboxyareckon1744220git

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hello.

Little, nice and handy module.

• Coder's review - checked.
• Duplication - checked. Didn't found any similar modules.

Module works stable and I ain't got much troubles with it.

My suggestion is to move hook_menu's page argument implementation (taxonomy_set_lineage_config_form()) to a separate file. So it'll be loaded only on configuration page.

Functionality review:

When I go to the settings the first time, I get a the following warning:
Warning: Invalid argument supplied for foreach() in form_type_checkboxes_value() (line 2288 of */includes/form.inc).
This is caused by the default value of 0 on line 22 of the admin file. Change this to array() and the warning is gone:
http://drupalcode.org/sandbox/yareckon/1744220.git/blob/68bde42:/taxonom...

Taxonomy field single value: when I change the taxonomy field of the content type to single values, obviously your module doesn't work. Maybe you can add a warning that the term reference fields should be mutlivalued.

Apart from these findings, the module worked correct.

Code review:

http://drupalcode.org/sandbox/yareckon/1744220.git/blob/68bde42:/taxonom...
You define language_changed, but you don't use it. Either use it, or remove this line.

For the rest it looks ok. Nice little module!

I didn't read the Readme careful enough I think. I agree that a warning on the field instance configuration page is overkill.

Couple of comments:

1. The taxonomy_set_lineage_affected_taxonomies variable defaults to NULL in taxonomy_set_lineage_entity_presave() and to array() in taxonomy_set_lineage_config_form(). Ideally this would be consistent in both places.
2. In taxonomy_set_lineage_entity_presave line 39 checks

$wanted_vocabs = variable_get('taxonomy_set_lineage_affected_taxonomies', NULL);
  if (isset($wanted_vocabs)) {
    foreach ($wanted_vocabs as $wanted_vocab) {

but $wanted_vocabs may not be an array so should probably check !empty($wanted_vocabs) instead or is_array() of isset.
3. In _taxonomy_set_lineage_vocab_to_instance_field_name(), list is used on the result of entity_extract_ids(), but $id and $vid are never used. Could do:

$entity_ids = entity_extract_ids(etc);
foreach (field_info_instances($entity_type, $entity_ids['bundle']) as $instance) {

4. In _taxonomy_set_lineage_activate_parents() you have:
$tidsnew[] .= isset($term['tid']) ? $term['tid'] : NULL; - i don't think you need .= should just be = (should save a few php notices). Same goes for $tidsold a little further down.

Other than those minor things, looks a nice little module and can definitely see the use cases.

Nothing that should stop approval, but have you thought about what happens when a parent taxonomy term is removed from the entity - should that remove the child terms too? Might be a nice addition.

Hope this helps.

added third review

Deleted - duplicate post.

Deleted - duplicate post.

hi yareckon, no problem - glad it was helpful.

I've looked at the updates you've done and they look good. pareview is reporting all good too from a code standard POV (http://ventral.org/pareview/httpgitdrupalorgsandboxyareckon1744220git)

On the entity_extract_ids issue, that was my mistake on the returned array keys, and its minor style preference in the list vs array key so not something that should stop project approval.

Looks a good module to me, I'm going to mark as RTBC.

manual review:

1. The Git commits are not connected to your user account. You need to specify an email address. See http://drupal.org/node/1022156 and http://drupal.org/node/1051722
2. _taxonomy_set_lineage_activate_parents(): "$tidsnew[] = $parent->tid;": why do you set that, $tidsnew is never used later?
3. taxonomy_set_lineage_config_form(): no need to document the @params here, see http://drupal.org/node/1354#forms

but that are not major issues, so ...

Thanks for your contribution, yareckon!

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.

there is still no project created yet. or am i blind? i looked at his user profile.

thank you man
https://drupal.org/project/taxonomy_set_lineage