[D7] Sphinx Search for Drupal

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

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.

Hey,

Switch your branch from master to something like 7.x. Also don't forget to delete the master branch.

Hello,

There are still some issues with you code that are picked up by the automated code reviewers.

Firstly, your working branch should be 7.x-1.x and not 7.x-1.0. Specific version numbers should be release tags and not branches. See: https://drupal.org/node/1015226

Secondly, all class names should be prefixed with your module name, so in lib/sphinxapi.php on line 445 you should probably rename SphinxClient to SphinxdrupalClient and rename all instances of it throughout your code.

There are also 2 undefined variable warnings in sphinxdrupal.pages.inc line 242 ($search_results) and line 300 ($search_options)

Report here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxivinco1197408git

Ah, yes, I see now that it's actually a 3rd party GPL library. I don't actually think you need to rename it in that case, sorry about that.

Manual examination of .module file:

1. Comment blocks for Drupal hooks should start with:

/**
* Implements hook_HOOK_NAME().
*/

I don't believe you are required to use @param and @return comments for hooks, but am not sure if those are actually discouraged or forbidden.


2. Don't require_once your class file.

This should be placed in your module.info file.
See: https://drupal.org/node/542202#files


3. Don't require_once your pages.inc file.

In your hook_menu you can use the 'file' option with a path to the specified file.
See: https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

In your hook_block_view(), just use module_load_include('inc', 'sphinxdrupal', 'sphinxdrupal.admin'); to load the file so that drupal_get_form can find it.
See: https://api.drupal.org/api/drupal/includes!module.inc/function/module_lo...

4. Move sphinxdrupal_admin() and sphinxdrupal_admin_validate() to an admin.inc file.

See my 3rd point for modifying the corresponding menu entry.

Missing sphinxdrupal.install:

You use a number of variables in your module, but you are missing a hook_uninstall() to remove those when the module is uninstalled. See: https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

Similar modules:

Here is a list of Sphinx modules that I came across:
Sphinx Search - https://drupal.org/project/sphinxsearch
Sphinx - https://drupal.org/project/sphinx
Search API Sphinx - https://drupal.org/project/search_api_sphinx
Sphinx API - https://drupal.org/project/sphinxapi

I feel like that is a lot of module duplication, and while I understand that none of those seem to have functional Drupal 7 versions, could you explain how else your module is different? (The person who is going to grant you full project access will probably want a solid answer to this question.)

I hope this post is clear enough, if you disagree with any of my suggested improvements, please let me know.

@ivinco: What do you think about reviving the sphinxsearch module and making your code the D7 branch?

1. I'm suggesting that it may be worthwhile to ask the maintainer of sphinxsearch if he intends to create a 7.x release, and if not to add you as a maintainer so you can find and replace sphinxdrupal with sphinxsearch and push that as the 7.x branch. The nice thing about that is the namespace is a perfect match for the integration, which helps discoverability, and there are already users there who may be able to upgrade.

2. It looks good to me. If you're opposed to #1, then I'll go ahead and approve it, but I think it's worth considering for building a user base.

Cheers!

If I am reading the request correctly, they are not asking you to maintain someone else's module. They are suggesting that it might be a good idea to make your module the D7 version of the existing module. You would be a co-maintainer, working on the D7 branch, and the other maintainer could still work on the D6 branch.

This would also achieve your other goal, since it would instantly provide you with a namespace that is already visible and discovered.

Maybe it's worth to take care about sphinx module. It has shorter name than sphinxsearch and it also doesn't have D7 version.

I agree with Chi. The Sphinx module would be really good name for this.

Duplicate modules for Sphinx are really bad already.

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.