[D7] Find Text

I'd recommend using http://pareview.sh to do an automated test of this project; there were quite a few warnings & errors. EDIT: removed long pareview.sh dump.

Setting status per https://www.drupal.org/node/532400

Mark,
You should check out the guidelines on https://www.drupal.org/node/997024 and add some of the items to your description here. Also, check out https://www.drupal.org/project/scanner. You'll probably want to have some good comparison of why your module is different enough from that.

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

Hi themarkahrens,

It seems in this line the apostrophe is missing for entity_id:
sites/all/modules/find_text/find_text.module, line 186:

$link_query->fields('f', array($field . '_url', entity_id));

In this case now it is an undefined constant, so it doesn't do that what you expect.

Hello there, nice one. I installed it and it is really fast and handy.

Pareview.sh finds some minor stuff, quite easy to fix for you.

FILE: /var/www/drupal-7-pareview/pareview_temp/find_text.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------
105 | WARNING | Only string literals should be passed to t() where
| | possible
236 | WARNING | Only string literals should be passed to t() where
| | possible
---------------------------------------------------------------------------

I also did some manual testing/thinking on your module.

• find_text_build_results seems to be a rather large function, that might be interesting to break up.
• Did you do some testing with large sets of content, I am kind of wondering how performance will be, as each search request will query the database.
• Highlighting found keywords might be a good one for the roadmap.

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/find_text.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------
105 | WARNING | Only string literals should be passed to t() where
| | possible
236 | WARNING | Only string literals should be passed to t() where
| | possible
You can Replace the line 105
($title->title ? l(t($title->title), "node/$title->nid") : t('Missing data for node @nid', array('@nid' => $title->nid))),
with
($title->title ? l(t("@title",array('@title' => $title->title)), "node/$title->nid") : t('Missing data for node @nid', array('@nid' => $title->nid))),

Manual Review

Individual user account

Yes: Follows

No duplication

Yes: Does not cause module duplication and/or fragmentation

Master Branch

[Yes: Follows / No: Does not follow] the guidelines for master branch.

README.txt/README.md

No: Does not follow the guidelines for README Template.
Rename README.md to README.txt also you Read Me.txt does not follow Template for README.txt https://www.drupal.org/node/2181737

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

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Your module does not search other fields than body,This module can be more helpfull if we can search the text in all fields
2. You can add hook_help in you module file https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
3. often we have to replace the text from multiple nodes,menu,terms so the module would be more beneficial if we can also replace the text with module.

t($item->link_title) is an XSS vulnerability. User provided text must be sanitized with placeholders in t(). Make sure to read https://www.drupal.org/node/28984 again. It also looks like you are printing vocabulary names and other user provided data unsanitized in your HTML tables, make sure to sanitize those as well. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hi,

I had downloaded module, good work on searching string.
Some issues with the module, as Follows :

1. Please provide more description for module. what is the use case of this module? What problem does it solve? See also https://www.drupal.org/node/997024
2. hook_help(), a module can make documentation available to the user for the module as a whole, or for specific paths. Help for developers should usually be provided via function header comments in the code.
3. Use db_like() in db_select query condition. @99, 138, 191, 230
$title_query->condition('title', '%' . db_like($search) . '%', 'LIKE'); Use if statement before using foreach looping@102.194, Leftjoin instead of Addjoin. Addjoin method is used in db_query .
4. Please optimises the complete code, Use child functions for clean & clear understanding of code.Before going into functional testing and negative testing, See also Coding standards Work on the module coding.
5. See also https://api.drupal.org/api/drupal/includes!database!database.inc/function/db_select/7.x

please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Okay klausi, by mistake i removed it. I won't do it 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.

Hi Mark Ahrens,

If you need any help complete this project then please let me know. I will help you complete your project.

Thanks

Hi,

I have checked your module on http://pareview.sh/ and found below errors:

FILE: /var/www/drupal-7-pareview/pareview_temp/find_text.module
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
116 | ERROR | [x] Expected one space after the comma, 0 found
246 | ERROR | [x] Expected one space after the comma, 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------

You need to use t() for below text lists:
Body Fields
Link Fields
Menu Items
Taxonomy Terms

minor coding standard errors are not application blockers, please do a real manual review. Anything else that you found or should this be RTBC instead?

hi @themarkahrens

Instead of checking textfield empty or not in submit hook , you can use validate hook for this.
check (if (isset($form_state['values']['search-string']) && $form_state['values']['search-string'] != '') ) this condition in validate hook.
Thankyou

hi @themarkahrens,

1) In menu hook there is no need of mention file as null .you can remove it from there.
2) In find_text_form hook else can condition can be written as elseif. because in else condition you have only if condition.
3)In comments you have mention "Module file for the Bongo module" , so what is Bongo module? No clear idea. i guess it is just copy paste problem

Thankyou

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.

Seems like the latest comments revolve around minor coding standards, which shouldn't block the application review.

Let's keep this closed until we hear back from the applicant.

FILE: find_text.module
Found 2 errors
line#116:- Expected one space after the comma,
line#246:- Expected one space after the comma,

@khurrami : Looks like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be "needs work"?

please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Security issues were resolved, as well as all the minor coding standard issues. Setting to RTBC as there have been several reviews without major issue. On a side note, I'm using this module on a client site without issue.

Thank you for your contribution!

I will update your account so you can opt into security advisory coverage. See Goodbye Project Applications, Hello Security Advisory Opt-in to understand what that means, and what is different from the previous project application purpose.

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!

Thank you, 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 go the dedicated reviewer(s) as well.

Assigning credits.