[D7] shs search api

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

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.

Made some of the corrections suggested by robot.
The other corrections were not made because:

"Do not use the raw $form_state['input'], use $form_state['values'] instead where possible"
That was not corrected because there is not $form_state['values'] available.

Method names were not corrected because this class extends from a views filter handler.

Thanks for reviewing

Manual Review

Individual user account

Yes Follows the guidelines for individual user accounts.

No duplication

Yes : Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes Follows the licensing requirements

3rd party code

No

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template. But I find that README.txt is very limited. It only few lines of codes without any useful information. I will recommend to follow this README.txt Template (https://www.drupal.org/node/2181737)

Code long/complex enough for review

Yes Follows the guidelines for project length and complexity.

Secure code

Yes

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.

I have run your project on http://pareview.sh/pareview/ and I find few errors & warning.

You should consider fixing them http://pareview.sh/pareview/httpgitdrupalorgsandboxrigoucr2324723git

<p></p>
<p>FILE: ..._temp/includes/handlers/shs_handler_filter_term_node_tid_search_api.inc<br>
--------------------------------------------------------------------------------<br>
FOUND 4 ERRORS AFFECTING 4 LINES<br>
--------------------------------------------------------------------------------<br>
  18 | ERROR | Public method name<br>
     |       | "ShsSearchApiHandlerFilterTermNodeTid::option_definition" is not<br>
     |       | in lowerCamel format, it must not contain underscores<br>
  30 | ERROR | Public method name<br>
     |       | "ShsSearchApiHandlerFilterTermNodeTid::extra_options_form" is<br>
     |       | not in lowerCamel format, it must not contain underscores<br>
  56 | ERROR | Public method name<br>
     |       | "ShsSearchApiHandlerFilterTermNodeTid::value_form" is not in<br>
     |       | lowerCamel format, it must not contain underscores<br>
 176 | ERROR | Public method name<br>
     |       | "ShsSearchApiHandlerFilterTermNodeTid::admin_summary" is not in<br>
     |       | lowerCamel format, it must not contain underscores<br>
--------------------------------------------------------------------------------</p>

Time: 114ms; Memory: 7.25Mb<br>

<p></p>
<p>FILE: ..._temp/includes/handlers/shs_handler_filter_term_node_tid_search_api.inc<br>
--------------------------------------------------------------------------------<br>
FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 5 LINES<br>
--------------------------------------------------------------------------------<br>
  74 | WARNING | Do not use the raw $form_state['input'], use<br>
     |         | $form_state['values'] instead where possible<br>
  75 | WARNING | Do not use the raw $form_state['input'], use<br>
     |         | $form_state['values'] instead where possible<br>
  88 | WARNING | Unused variable $tid.<br>
 162 | WARNING | Do not use the raw $form_state['input'], use<br>
     |         | $form_state['values'] instead where possible<br>
 162 | WARNING | Do not use the raw $form_state['input'], use<br>
     |         | $form_state['values'] instead where possible<br>
 163 | WARNING | Do not use the raw $form_state['input'], use<br>
     |         | $form_state['values'] instead where possible<br>
--------------------------------------------------------------------------------</p>

Time: 52ms; Memory: 5.5Mb<br>

Thanks @darol100.

About what you commented:

I updated the project page. I think it looks better now.

The errors reported by CoderSniffer are because that file extends from views_handler_filter_term_node_tid; so I need to name the functions on that way.

The warnings reported by DrupalPractice are because in that point of the code I don't have those values in $form_state['values'].

Thanks again,

Automated Review

No. Please correct the issues reported at http://pareview.sh/pareview/httpgitdrupalorgsandboxrigoucr2324723git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes ,no master branch: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

No: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. It's a very small module to promote as stand alone project. I suggest you to contact either SHS or Search API maintainers to add your code as an additional functionality (preferably with SHS)

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.

Moving this to needs work, I would like you to think again before promoting it as stand alone module, else I don't see any blockers for this.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Really sorry for being so late. I contacted the shs mantainer; but I never got answer from him. So; I'd expect to promote this as an standalone project; and I'd hope to get help for this.

Thanks in advance

Assigning to myself for next review that might be in tonight.

First Thanks for your contribution!

Automated Review

There are lots of Best practice issues identified by pareview.sh / drupalcs / coder. http://pareview.sh/pareview/httpgitdrupalorgsandboxrigoucr2324723git

Manual Review

Individual user account

Maybe: Follows the guidelines for individual user accounts.There are two authors of this projects.So do let us know which of the user wants the git vetted role.I see there are two committers in this project https://www.drupal.org/node/2324723/committers

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

No: Does not follow the guidelines for in-project documentation and 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

1. A hook_help is missing in module.It would be nice it in the module.
2. As the module is implementing the views filter views_handler_filter_term_node_tid so I would suggest to add the dependency on the views module.
3. shs_handler_filter_term_node_tid_search_api.inc: You are adding js/css files using drupal_add_js/drupal_add_css rather use
   #attached to add the JS/CSS file
   with $form array.
4. Just a recommendation

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.

I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list, then you will come in admins radar :)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Hi there again;
we had totally forgotten about this :(

Taking this project again.

Thanks @naveenvalecha; I already applied the changes that you suggested.

I think this is ready to promote to full project; it would be great if someone could change status to RTBC and give full project status.

Thanks in advance,

manual review:

1. "module_exists('search_api')": you already depend on search_api_views which depends on search_api, so this check will always return TRUE and can be removed.
2. shs_search_api_taxonomy_get_all_descendants(): doc block is confusing, this function does not return anything?
3. shs_search_api_taxonomy_get_all_descendants(): why do you have to cast the tids to string? Please add a comment.
4. README.txt is a bit short, see also https://www.drupal.org/node/2181737

But otherwise looks good 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.

Thanks @klausi.

About your comments:
1) Removed
2) Updated comment
3) Removed unuseful cast and tested module functionality.
4) Updated README.txt

Really thanks for your review.

In ShsSearchApiHandlerFilterTermNodeTid->value_form() one small nit. Integer indexed array isn't really necessary:

        '#attached' => array(
          'js' => array(
            0 => array(
              'data' => $settings_js,
              'type' => 'setting',
            ),
            1 => array(
              'data' => drupal_get_path('module', 'shs') . '/js/shs.js',
              'type' => 'file',
            ),
          ),
        ),

Just nest these as array elements directly.

This is not even close to a blocker. Thanks for your contribution, Rodrigo! Maybe I'll even see you in a week at Drupal Camp CR?

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.