Problem/Motivation

The search protocol accepts a parameter named lr that allows to restrict search results by language.
e.g. When searching content from a Chinese interface, it allows to display only Chinese results.
Currently the google_appliance module does not seem to support such language filter. It could be nice, for instance on multillingual sites.

Proposed resolution

This can be implemented by altering queries sent to the GSA -- we just have to set the lr parameter appropriately.
Administrators would still have the option not to use language filtering.
Attached is a patch suggestion to provide such feature.

Remaining tasks

  • Opinions welcome.
  • The patch can be reviewed.

Interface changes

See screenshot -- new options provided on admin/config/search/google_appliance/settings.

CommentFileSizeAuthor
#4 google_appliance_language_filtering-4.patch5.81 KBfengtan
#2 google_appliance_language_filtering-2.patch5.31 KBfengtan
google_appliance_language_filtering_admin_ui.png70.22 KBfengtan
google_appliance_language_filtering.patch4.18 KBfengtan

Comments

iamEAP’s picture

iamEAP commented
Status: Needs review » Needs work

Thanks @Fengtan! This is definitely something I'd love to support in the module. Your patch looks pretty good, but I've found a few small issues in reviewing the code, found below.

Once those are taken care of, I'd like to manually test against my instance and provide further feedback and/or integrate the patch.

  1. +++ b/google_appliance.admin.inc
@@ -75,6 +75,28 @@ function google_appliance_admin_settings($form) {
+      '#title' => t('Restrict searches to pages in a specified language'),

    Small nitpick: Maybe just "Restrict searches to specified languages." 

  2. +++ b/google_appliance.admin.inc
@@ -75,6 +75,28 @@ function google_appliance_admin_settings($form) {
+        '***CURRENT_LANGUAGE***' => t('Current user\'s language'),
+        '***DEFAULT_LANGUAGE***' => t('Default site language'),

    I believe strings within t() functions should avoid using escape characters for the purposes of integration with localize.drupal.org.

    Should probably just wrap this in double quotes. 

  3. +++ b/google_appliance.module
@@ -470,6 +470,27 @@ function google_appliance_search_view($query = '', $sort = NULL) {
+    if (module_exists('locale') && variable_get('google_appliance_language_filter_toggle', FALSE)) {
+      $langcodes = array();
+      $options = array_filter(variable_get('google_appliance_language_filter_options', array('***CURRENT_LANGUAGE***')));
+      foreach ($options as $option) {

    Although antiquated... It'd be best to integrate these configurations into the _google_appliance_get_settings() system that's in place, rather than calling variable_get() directly.

fengtan’s picture

fengtan
Location Montreal, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new google_appliance_language_filtering-2.patch5.31 KB

Awesome. Thanks for the review.
Here is a new patch that should take the feedback into consideration.

Note that one variable uses array values, which cannot be used as PHP constants. I did this as a workaround:

+++ b/google_appliance.helpers.inc
@@ -60,6 +62,13 @@ function _google_appliance_get_settings($refresh = FALSE) {
+    // The default value of language_filter_options is an array, which
+    // cannot be used as a PHP constant. We set it here.
+    $settings['language_filter_options'] = variable_get(
+      'google_appliance_language_filter_options',
+      array('***CURRENT_LANGUAGE***')
+    );
iamEAP’s picture

iamEAP commented
Status: Needs review » Needs work

Thanks! You work quickly.

Changes look good and manual review went well. Just a couple more notes, after looking at the patch holistically, and I think it'll be ready to commit.

  1. Since we're adding some configs, we should also add them to the uninstall hook in google_appliance.install
  2. Seems a little silly to have duplicate chunks of logic in a couple different places (don't want them to get out of sync in the future). Would be nice to move it to a helper function in helpers.inc, so we could have something as simple as this in ...search_view and ...get_clusters:
        // Alter request according to language filter settings.
    if (module_exists('locale') && $settings['language_filter_toggle']) {
      $search_query_data['gsa_query_params']['lr'] = _google_appliance_get_lr($settings['language_filter_options']);
    }
fengtan’s picture

fengtan
Location Montreal, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new google_appliance_language_filtering-4.patch5.81 KB

Right, that makes sense. Here is a new patch.

  • Commit a572618 on 7.x-1.x authored by Fengtan, committed by iamEAP: 
    Adding the option to filter results by language. [#2244073]
iamEAP’s picture

iamEAP commented
Status: Needs review » Fixed

Thanks for your contribution, @Fengtan! Committed and pushed to dev (as noted above).

Status: Fixed » Closed (fixed)

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