Search Modify

I think this module should be renamed to something a little less ambiguous.

Codesniffer

FILE: ...sites/all/modules/search_modify/search_modify.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 31 | WARNING | Avoid backslash escaping in translatable strings when possible,
    |         | use "" quotes instead
--------------------------------------------------------------------------------

Coder

search_modify.module

    severity: normalLine 40: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms

        $form['search_modify_redirect']['search_modify_redirect_'  . $id]  = array(

    severity: criticalLine 94: Using eval() or drupal_eval() in your module's code could have a security risk if the PHP input provided to the function contains malicious code. (Drupal Docs)

          $redirect = eval($redirect);

I had to clear my site cache before I could hit admin/config/content/search_modify for some reason..

Otherwise, seems to work as described.

making a suggestion and pasting some little automated results in is really no reason for throwing this issue back to needs work

Codesniffer scan comes up pretty clean, one warning which seems pretty minor.

The only issues that I can see are that there may be modules that duplicate this functionality. How does Search modify differ from Search Restrict? http://drupal.org/project/search_restrict

The other issue is that your project seems to be a little short for the application process. Your module should be at least 120 lines of code or contain at least 5 functions.

http://drupal.org/node/1587704

2.3 Ensure your project contains a minimum of handwritten code.
We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed, what means we can't make sure you're able to write secure code and use Drupal's APIs correctly.
If this is the case, we can't approve you as git vetted user (giving you the permission to create full projects). Either we can see that you have the needed knowledge by other contributions within the drupal community or we can promote it to a full project manually.

Please, remove master branch in your git and add valid default branch on Version Control tab.

.info file

Consider adding your configuration page in your info file to get that fancy icon on the module list.

.module file

Sometimes you are using single quotes, sometimes double quotes. Please read http://drupal.org/coding-standards#quotes.
What's more if you are not using any variables directly in your string consider using single quotes instead of double quotes (PHP parsed anything inside double quotes, so it results in slightly worse performance.

search_modify_preprocess_search_result()

if (variable_get('search_modify_no_info', FALSE) == TRUE) {
You do not need to == TRUE. If is good enough. Of course it is not a bug, just suggestion ;)
And what if I type test/<?php echo $node->nid; ?>

You should check if user has access to run PHP code (administer site configuration is not enough on your settings page).
User can have administer site configuration and do not have other permissions (as administer users - which gives control over permission system). I cannot imagine that someone can have administer site configuration and not using php code, but in theory it's allowed.
You can add your own permission to run php code or use existing one.

search_modify_preprocess_search_result()

Use drupal_substr() instead of substr().

I'm setting it to needs work because of php evaluation. I'm not experienced in reviewing Drupal modules, so if it should be needs review, please feel free to change.

Hi kelvincool,

Nice idea to add the ability to customise search links !

I ve just made a manual review of your module code. Here are my remarks/suggestions :

- in search_modify_query_alter the variable $exclude_node_type is not used. You should remove this variable from your code.

- in the advanced search form, exclude types are not removed in the checkboxes choices, when i exclude a node type. You should remove the unuseful checkboxes with a hook_form_alter implementation.

- I think the exclude content types part of your module is a duplication of Custom Search module. http://drupal.org/project/custom_search. Your module should extend the Custom Search module, to add the ability to choose links to nodes in search results.

- You should add the module "Php Filter" in your dependencies

Regards,

AmbikaFR.

- The Custom Search module has a different content types functionality, it doesn't exclude content types per se, it allows users of the site to search by content types whereas my module is for administrators to restrict what shows up in search results. They are really for two different audiences in terms of functionality.

Sorry about that, you are right !

- The PHP filter is not a required dependency, the module can function fine without it. The PHP filter simply enhances it.

You are right, but i think you should change the field description dynamically if PHP Filter module is not enabled (as you already do if the user has not the rights to edit Search Modify configuration). It could be confusing to talk about PHP Filter if it's not enabled.

I have noticed also : Instead of building a specific configuration form, you can use hook_search_admin, to add your specific configuration fields in a Search Modify fieldset. It should be confusing to have two different forms for search configuration.

Regards,

herve.

You need a hook_uninstall to clean up the variables that are being set in search_modify_form_search_admin_settings_alter(). This is a blocker.

Your project page needs to show how this is different from Custom Search and other similar modules, and why one would choose one or the other. You won't pass the final review w/o this.

Few very small code format issues: http://ventral.org/pareview/httpgitdrupalorgsandboxkelvincool1620970git

Inside your search-results.tpl.php, you are using $_GET['q'] directly. This isn't the best idea as there are Drupal functions for abstracting this (request_uri(), request_path(), etc): program to the interface not the implementation when you can.

I would also say that you have too much logic in your .tpl.php; at the least you should turn lines 27-28 into variables.

You need to indicate in the docs that nodes will still be indexed, even if they are excluded from search results. This is a serious performance impact for some sites. Long term, you may want to fold this logic in and give the user the choice between exclude content types from indexing or exclude types from the query. There are advantages and disadvantages to both.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.