Facet API Bonus
Drupal 7 module
http://drupal.org/sandbox/daniel.nolde/1387636
git clone http://git.drupal.org/sandbox/daniel.nolde/1387636.git facet_api_bonus

---------------

Facet API Bonus for Drupal 7 is a collection of additional Facet API plugins and functionality, foremost filter and dependency plugins – And a place to collect more additional Facet API extensions.

Currently Facet API Bonus includes:

  • Facet Dependency: Dependency plugin to make one facet (say "product category") to show up depending on other facets or specific facet items being active (say "content type" is "product" or "service"). Very flexible, supports multiple facets to be dependencies, as well as regexp for specifying facet item dependencies, as well as option how to behave if a dependency is being lost.
  • Exclude Items: Filter plugin to exclude certain facet items by their markup/title or internal value (say excluding "page" from "content types"). Regexp are also possible.
  • Rewrite Items: Filter plugin to rewrite labels or internals of the facet items via a admin-configurable callback function (in a structured array, before rendering). Very handy to rewrite list field values or totally custom encoded facet values for user friendly output.

Further additions are very welcome!

Facet API Bonus is written for Drupal 7, and is stable, tested, and ready to be used in production environments.

Requirements:

  • Facet API is obviously required, as well as a compatible search module (e.g. apachesolr, search_api).

Similar projects:

  • http://drupal.org/project/facetapi_extra, created at the same time, seems to aim for the same goal, containing but one smaller dependency plugin at the moment. Facet API Bonus offers more different plugins for more use cases, the plugins in Facet API Bonus are richer in function and number at the moment. Maintainers will later discuss further distinction or merge.

---------------

My earlier application for full project access had been stuck in the cvs>git transition, hopefully this one will work ;)

Comments

jinlong’s picture

Status: Needs review » Needs work

It appears 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.
Review of the master branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.


FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/facetapi_bonus.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 35 | ERROR | Expected 1 space(s) before asterisk; 0 found
 36 | ERROR | Expected 1 space(s) before asterisk; 0 found
--------------------------------------------------------------------------------


FILE: ...ules/pareview_temp/test_candidate/plugins/facetapi/dependency_facet.inc
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
 18 | ERROR | Class property $default_settings should use lowerCamel naming
    |       | without underscores
 26 | ERROR | Inline comments must start with a capital letter
 26 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
    |       | question marks
 31 | ERROR | Inline comments must start with a capital letter
 31 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
    |       | question marks
 37 | ERROR | Inline comments must start with a capital letter
 64 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
    |       | question marks
 93 | ERROR | Inline comments must start with a capital letter
 93 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
    |       | question marks
--------------------------------------------------------------------------------


FILE: ...temp/test_candidate/plugins/facetapi/filter_exclude_specified_items.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
 13 | ERROR | Expected 1 space(s) before asterisk; 0 found
 14 | ERROR | Expected 1 space(s) before asterisk; 0 found
 31 | ERROR | Expected 0 spaces after opening bracket; 1 found
 33 | ERROR | There must be a single space before an operator statement
 40 | ERROR | Inline control structures are not allowed
--------------------------------------------------------------------------------


FILE: .../pareview_temp/test_candidate/plugins/facetapi/filter_rewrite_items.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 10 | ERROR | Expected 1 space(s) before asterisk; 0 found
 11 | ERROR | Expected 1 space(s) before asterisk; 0 found
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

cpliakas’s picture

Status: Reviewed & tested by the community » Needs work

A point of clarification against the review in #1...

"plugins/facetapi/dependency_facet.inc" in facetapi_bonus.info: It's only necessary to declare files[] if they declare a class or interface.
"plugins/facetapi/filter_rewrite_items.inc" in facetapi_bonus.info: It's only necessary to declare files[] if they declare a class or interface.
"plugins/facetapi/filter_exclude_specified_items.inc" in facetapi_bonus.info: It's only necessary to declare files[] if they declare a class or interface.

All of these files DO contain classes that need to be autoloaded, so the API is implemented correctly in this instance and the module would be broken if these directives were removed.

I installed the module and played around with the internals for a while. I did find a couple of minor bugs that are posted against the issue queue and listed below:

#1388644: Do not explicitly return TRUE in the FacetapiDependencyFacet::execute() method
#1388666: Remove the FacetapiDependencyFacet::getActiveFacets() method in favor of the FacetapiDependency::activeItems property
#1388702: Facet labels are double-translated in FacetapiDependencyFacet::getEnabledFacets()
#1388750: Notice errors when "Exclude specified items" filter is active but the list is empty

Going down the How to review Full Project applications list for new modules, my comments are as follows:

Module description and uniqueness

The module's goals and description are clearly defined on the project page. This could be copied to the README.txt file as-is. In addition, it explains how it is different from the Facet API Extra module. My only comment here is that there is a piece of overlapping functionality in the "Another Facet" dependency that could be posted to Facet API Extra as a feature request against the "Parent" dependency where the two pieces could then be merged together.

Licensing

There are no violations of the GPL.

Security

The module correctly uses API functions such as check_plain() to sanitize user generated data. The only area of concern is the "Rewrite facet items via callback function" filter where you can specify a function though a textfield to alter the facet items. Ideally a list of available callbacks would be provided by a hook to whitelist which ones could be used to avoid giving site builders the ability to invoke any available function. See #1388804: Pull callbacks from a hook as opposed to allowing users to specify a function name through a textfield for more info.

Best practices and coding standards

The module adheres to programming best practices and also adheres to Drupal coding standards minus a few minor issues. Again, it is better than most of the modules on Drupal.org and shows an understanding of the guidelines. Reviewing the Module documentation guidelines page, there is no README.txt file as mentioned in #1 and there is also no hook_help() implementation. In this instance I would say this is OK and hook_help() would actually be overkill since most of the usage and UI is inherited from Facet API. There are good labels and descriptions in the UI that explain how to use the various filters and dependencies.

Conclusion

Overall I feel there is no major reason to deny this application. There are a few issues to resolve, mainly the security issue, but it shouldn't prevent this process from moving forward. I do think that the "Another Facet" dependency should be submitted to Facet API extra and removed from this project. Then to be more descriptive of what this module accomplished, I would propose renaming it to "Facet API Advanced Filters" to narrow the scope and not conflict with the purpose of Facet API Extra. Let's discuss, then I would support this issue being marked as RTBC.

danielnolde’s picture

Status: Needs work » Needs review

Thanks for the prompt and rich feedback, guys!!

Fixed:

  • README.TXT: Fixed. Now provided.
  • Code style issues: Fixed (coder.module coding standards review didn't show them - ventral's pareview is a bit anal, but great!).
  • Dependency logic/return value (#1388644): Fixed: Didn't know that, corrected via your patch - thanks, Chris!
  • Security with user-definable callback (#1388804): Fixed. Switched from insecure user definable callback function to dedicated hook_facet_items_alter (with documentation in the facet filter's user interface; implementing a hook for defining selectable callbacks and implementing the callback itself adds unnecessary overhead, when an alter hook does the job as well and the specific facet context is available for decisions)
  • Double translation in getEnabledFacets (#1388702): Fixed. thanks for the Hint!
  • Notices for empty exclude filter edge case (#1388750): Should be fixed.
  • Replacing getActiveFacets() (#1388666): Fixed, thanks for this hint!

No fix needed:

  • files[] declaration: No error, as Chris already pointed out. Pareview seem's broken here, or at least not blindly dependable.
  • hook_help(): no need for this at the moment, the usage is well documented within the plugins' form UI.

Needs discussion:

  • I'll do the move to a release specific branch once the module is in full project mode (since there are no releases for sandbox projects).
  • The distinction or the merge of the "Facet" dependency with facetapi_extra's "Parent" dependency. Yuri (the author of facetapi_extra) has to give feedback here, and he and me have to find the best way to do this.

Pareview clean.
Issue queue processed and clean.

Thanks again for the great feedback, as usual with Drupal community interaction, i learned a lot!
Looking forward to have this RTBC!

cpliakas’s picture

Status: Needs review » Reviewed & tested by the community

Based on the actions in #3, I am marking this application as RTBC. Judging by the attention to the issue queue and willingness to work with other developers, danielnolde will be a great maintainer and a valuable contributor. I look forward to this module being promoted to a full project so people can start downloading an official release. The functionality is very useful and a great add-on to Facet API.

klausi’s picture

Status: Needs work » Fixed
  • The files[] bug in pareview.sh was fixed and moved to drupalcs. Everthing ok on your side.
  • Module duplication is a big problem on drupal.org, so before you promote your own project you should really consider if this could live in facetapi_extra and coordinate you efforts with the maintainer.
  • FacetapiDependencyFacet::getDefaultSettings(): array indentation errors, you should only use 2 additional spaces per indentation level on the next line.
  • And I really recommend to abandon the master branch and create a version specific branch. This avoids confusion in the long term and should be followed from the start.

Anyway, there is no real blocker for this application, so ...

Thanks for your contribution, danielnolde! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jgladstone’s picture

Category: task » support
Status: Closed (fixed) » Active

Would like to see an example of a simple facet name ( #markup ) change via facet_items_alter function

klausi’s picture

Category: support » task
Status: Active » Closed (fixed)

This project application is closed. Please use the issue queue of the project.