This module extends the Apache Solr module. It lets users limit their search to a specific field instead of searching the full node text. For example, a library site may have a node type called book with a field for the book author and another for a plot summary, this module creates a configurable block where the user can choose to search only the author field without getting hits in the plot summary.

This module will currently work with the 6.x-2.x branch of Apache Solr Search.

http://drupal.org/sandbox/AndreasRadloff/1474818
git clone --branch 6.x-2.x AndreasRadloff@git.drupal.org:sandbox/AndreasRadloff/1474818.git apachesolr_field_search

Thanks for taking the time to review it!

Comments

Andreas Radloff’s picture

Andreas Radloff commented
Status: Active » Needs review
patrickd’s picture

patrickd commented
Status: Needs review » Needs work

welcome,

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
(At least this has to be done before switching back to needs review)

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxandreasradloff1474818git

You can also get a review bonus and we will come back to your application sooner.

regards

Andreas Radloff’s picture

Andreas Radloff commented
Status: Needs work » Needs review

Thank you friendly bot for pointing me to ventral.org, it caught some stuff that Coder didn't!

All issues but a 80+ chars line have been fixed.
Repo cleaned up.

anilbhatt’s picture

anilbhatt commented

Manual Review:
1. Indentation errors in hook_menu() and many other places.
2. Line #54 not readable, try to break it multiple lines.
3. On line #104 why you have created case 'configure' and what's the significance of $form?
4. $available_fields array(), IMO should be declare before the 'foreach' loop.
5. Use t() function on line 99 or line 159.

anilbhatt’s picture

anilbhatt commented
Status: Needs review » Needs work
Andreas Radloff’s picture

Andreas Radloff commented

Thanks for the review anilbhatt!

3, 4 and 5 have been fixed and comitted, thanks for spotting them!

I hope you don't think I'm obnoxious now but I would like some input on these two:

1
Could you give an example of the indentation errors? Coder and pareview didn't find any errors, I did a lot of indentation fixes after I put it through there actually, I thought I had it cleaned up nice and by the rules?

2
I agree, it's awful, but it's a copy of apachesolr_cck_text_indexing_callback() in apachesolr module, which I have now documented in my code. Is it not better to leave it as the original function is written in this case?

Andreas Radloff’s picture

Andreas Radloff commented
Status: Needs work » Needs review
Andreas Radloff’s picture

Andreas Radloff commented
Issue summary: View changes

fixed fauly git clone url

c-logemann’s picture

c-logemann
he/him
Primary language German
Location Frankfurt/M, Germany
commented
Assigned: Unassigned » c-logemann

Assigned to myself, to show that I am working on review.

c-logemann’s picture

c-logemann
he/him
Primary language German
Location Frankfurt/M, Germany
commented
Assigned: c-logemann » Unassigned
Status: Needs review » Needs work

Module duplication?
http://drupal.org/node/894256#anchor-goals
What is the difference betwenn this module and "apachesolr_custom_fields"? Why not merge?
http://drupal.org/project/apachesolr_custom_fields

Risk of naming conflict?
The module name is "apachesolr_field_search". I think there can be a naming conflict in future with the module "apachesolr" and maybe a new module "apachesolr_field". Ok, you are not the only one who is naming projects like this for example apachesolr_custom_fields

Dependency on apachesolr-6.x-2.x
Yout module is based on "apachesolr-6.x-2.x" which is marked as Previous experimental branch. Unsupported and deprecated except for looking at code examples.
There ist an 6.x-3.0-alpha1 Version.

Manual code review: Security Issue

function apachesolr_field_search_apachesolr_modify_query(&$query, &$params) {
  if ($fields = $_REQUEST['fields']) {
    $params['fl'] .= ',' . $fields;
    $params['qf'] = array($fields . '^40');
  }
}

@todo Check if input sanitation is needed
I think it's strongly recommended. I don't see ad hoc what sanitation is done by apachesolr. But I think it's your liability if your module is receiving data from $_REQUEST. There can be violating code and to much data pushed to your module even by guests/robots.

Functionality
Because the module is based on apache solr and I have currently no testing system for this. So I can't test the functionality. Maybe somebody with an apachesolr setup can test this?

Machine review
There are two minor code standard issues left:
http://ventral.org/pareview/httpgitdrupalorgsandboxandreasradloff1474818git

Edit/add:
Maybe change git link in iussue summary to a gust version:
git clone --branch 6.x-2.x http://git.drupal.org/sandbox/AndreasRadloff/1474818.git apache_solr_field_search

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

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

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

fixed git clone url